From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/hyperv: validate VMBus packet size in receive callback Date: Mon, 25 May 2026 17:45:31 +1000 Message-ID: In-Reply-To: <8200dbc199c7a9b75ac7e8af6c748d2189b5ebd5.1779542874.git.me@berkoc.com> References: <8200dbc199c7a9b75ac7e8af6c748d2189b5ebd5.1779542874.git.me@berkoc.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Status: Good =E2=80=94 minor observations, no blocking issues.** **Header minimum check:** Correct. The header size computation matches the = offset to the union payload in `struct synthvid_msg`: ```c + hdr_size =3D sizeof(struct pipe_msg_hdr) + + sizeof(struct synthvid_msg_hdr); + if (bytes_recvd < hdr_size) { ``` `pipe_msg_hdr` is 8 bytes, `synthvid_msg_hdr` is 8 bytes, so `hdr_size =3D = 16`. The cast to `struct synthvid_msg *` is correctly moved after this vali= dation, ensuring `msg->vid_hdr.type` isn't read from an undersized buffer. **SYNTHVID_VERSION_RESPONSE and SYNTHVID_VRAM_LOCATION_ACK:** Straightforwa= rd fixed-size checks. Both correctly add the inner struct size to `need` an= d fall through to the shared completion path. **SYNTHVID_RESOLUTION_RESPONSE:** The two-stage validation is well thought = out: ```c + case SYNTHVID_RESOLUTION_RESPONSE: + need +=3D offsetof(struct synthvid_supported_resolution_resp, + supported_resolution); + if (bytes_recvd < need) + break; + if (msg->resolution_resp.resolution_count > + SYNTHVID_MAX_RESOLUTION_COUNT) { + drm_err_ratelimited(&hv->dev, ...); + return; + } + need +=3D msg->resolution_resp.resolution_count * + sizeof(struct hvd_screen_info); + break; ``` The `offsetof` correctly gives 131 bytes (128 EDID + 1 `resolution_count` += 1 `default_resolution_index` + 1 `is_standard`). The early `break` on unde= rsized packets falls through to the shared path where `bytes_recvd < need` = catches and logs it. The `return` on oversized `resolution_count` prevents = the completion from firing, which is correct =E2=80=94 the waiter will time= out and report the failure. The duplicate bounds check with Patch 1's cons= umer-side check provides defense-in-depth. **SYNTHVID_FEATURE_CHANGE:** Correctly handled as a self-contained case tha= t validates and returns without falling through to the completion path: ```c + case SYNTHVID_FEATURE_CHANGE: + if (bytes_recvd < need + + sizeof(struct synthvid_feature_change)) { ``` At this point `need =3D=3D hdr_size` (16 bytes), plus `sizeof(synthvid_feat= ure_change)` =3D 4 bytes, for a total of 20 bytes minimum. Correct. **memcpy change:** The switch from `memcpy(hv->init_buf, msg, VMBUS_MAX_PAC= KET_SIZE)` to `memcpy(hv->init_buf, msg, bytes_recvd)` is a good improvemen= t. Both buffers are `u8[VMBUS_MAX_PACKET_SIZE]` (0x4000 =3D 16 KiB), and `b= ytes_recvd <=3D VMBUS_MAX_PACKET_SIZE` is guaranteed on a successful `vmbus= _recvpacket()` return since that's the buffer size passed. The consumers (`= hyperv_negotiate_version`, `hyperv_get_supported_resolution`, `hyperv_updat= e_vram_location`) only access fields within the validated payload extent, s= o stale bytes beyond `bytes_recvd` in `init_buf` are not a concern. **vmbus_recvpacket error handling in hyperv_receive():** ```c + if (ret) { + drm_err_ratelimited(&hv->dev, + "vmbus_recvpacket failed: %d (need %u)\n", + ret, bytes_recvd); + } else if (bytes_recvd > 0 && + recv_buf->pipe_hdr.type =3D=3D PIPE_MSG_DATA) { + hyperv_receive_sub(hdev, bytes_recvd); + } ``` This fixes a real bug in the original code: the `do { } while` loop body wo= uld execute before the loop condition `ret =3D=3D 0` is tested, so on an er= ror return (e.g., `-ENOBUFS`), the original code would still enter the `if = (bytes_recvd > 0 ...)` body, potentially calling `hyperv_receive_sub()` wit= h stale buffer contents and a `bytes_recvd` value representing the *require= d* length (which could exceed the 16 KiB buffer). The new gating on `ret` p= revents this. **Minor observations (non-blocking):** 1. The `default:` case silently drops unknown message types. A `drm_dbg_rat= elimited()` could aid debugging during development, but silent drop is reas= onable for production to avoid log noise from potential future message type= s. 2. The error message `"vmbus_recvpacket failed: %d (need %u)\n"` uses "need= " to describe `bytes_recvd` =E2=80=94 this is correct on `-ENOBUFS` where `= bytes_recvd` reports the required length, but could be slightly confusing f= or other error codes. Minor nit; the context makes it clear enough. 3. The loop exit on error means any valid packets queued behind a bad one w= on't be processed in this callback invocation. This is acceptable =E2=80=94= on `-ENOBUFS` the oversized packet is stuck in the ring and retrying would= just fail again; the callback will be re-triggered by the next vmbus inter= rupt. Overall both patches are correct, well-documented, and ready to merge. The = commit messages are thorough and the changelog clearly tracks the evolution= across versions. --- Generated by Claude Code Patch Reviewer