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 19:37:10 +1000 Message-ID: In-Reply-To: <6e5d1d57a3afc4c5ea0d2a3d62be58c90741a869.1779396074.git.me@berkoc.com> References: <6e5d1d57a3afc4c5ea0d2a3d62be58c90741a869.1779396074.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 **Verdict: Good. Well-reasoned defense-in-depth.** **Header size check** =E2=80=94 Correct. The minimum `hdr_size` of `sizeof(= struct pipe_msg_hdr) + sizeof(struct synthvid_msg_hdr)` =3D 16 bytes ensure= s `msg->vid_hdr.type` is safe to read before the type dispatch: ```c hdr_size =3D sizeof(struct pipe_msg_hdr) + sizeof(struct synthvid_msg_hdr); if (bytes_recvd < hdr_size) { drm_err_ratelimited(&hv->dev, "synthvid packet too small for header: %u\n", bytes_recvd); return; } ``` **Per-type payload validation** =E2=80=94 The switch inside the completion = block is the right structure. Each response type gets its own minimum size: - `SYNTHVID_VERSION_RESPONSE`: `hdr_size + sizeof(struct synthvid_version_r= esp)` =3D 22 bytes - `SYNTHVID_VRAM_LOCATION_ACK`: `hdr_size + sizeof(struct synthvid_vram_loc= ation_ack)` =3D 24 bytes - `SYNTHVID_FEATURE_CHANGE`: `hdr_size + sizeof(struct synthvid_feature_cha= nge)` =3D 20 bytes **Variable-length SYNTHVID_RESOLUTION_RESPONSE** =E2=80=94 This is the most= complex case and it's handled well. The two-step validation is correct: ```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(...); return; } need +=3D msg->resolution_resp.resolution_count * sizeof(struct hvd_screen_info); break; ``` First validates the fixed prefix (EDID block + count/index/is_standard =3D = 131 bytes), then reads `resolution_count`, bounds it, and requires exactly = the count-sized array. This avoids the v3 bug where requiring the full `siz= eof(struct synthvid_supported_resolution_resp)` (=3D 387 bytes) would rejec= t the shorter responses the host legitimately sends when fewer than 64 reso= lutions are reported. **memcpy size change** =E2=80=94 Critical improvement: ```c // old: copies full 16 KiB regardless of actual data memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE); // new: copies only received data memcpy(hv->init_buf, msg, bytes_recvd); ``` This eliminates the stale-data leak where `hyperv_get_supported_resolution(= )` and other consumers could read residual data from a prior message in `in= it_buf` beyond `bytes_recvd`. Combined with patch 1's bounds check on `reso= lution_count`, this closes the window where a short response combined with = stale `init_buf` content could produce corrupt resolution data. **`!ret` gating** =E2=80=94 Essential with the new `memcpy(... bytes_recvd)= `: ```c if (!ret && bytes_recvd > 0 && recv_buf->pipe_hdr.type =3D=3D PIPE_MSG_DATA) hyperv_receive_sub(hdev, bytes_recvd); ``` On `vmbus_recvpacket()` returning `-ENOBUFS`, `bytes_recvd` contains the *r= equired* buffer size, which can exceed `VMBUS_MAX_PACKET_SIZE`. Without the= `!ret` gate, the old `VMBUS_MAX_PACKET_SIZE` memcpy was safe (both buffers= are that size), but the new `bytes_recvd` memcpy would overflow both `recv= _buf` and `init_buf`. The cover letter's analysis of this is accurate. **Observation on completion timeout behavior:** When `hyperv_receive_sub()`= rejects a packet (e.g., resolution count too large), it returns without ca= lling `complete()`. The corresponding `wait_for_completion_timeout()` in `h= yperv_get_supported_resolution()` / `hyperv_negotiate_version()` / `hyperv_= update_vram_location()` will eventually time out and return `-ETIMEDOUT`. T= his is the correct degraded behavior =E2=80=94 a corrupt response should no= t be treated as valid. Patch 1's fallback ensures the driver still gets usa= ble WIN8 defaults. **One subtlety worth noting:** The `need` variable is declared as `size_t` = inside the `if` block, and `bytes_recvd` is `u32`. The comparison `bytes_re= cvd < need` promotes `bytes_recvd` to `size_t`, which is safe since `u32` = =E2=86=92 `size_t` is a widening conversion. No issue here. Overall, this is a solid hardening series that follows good defensive codin= g patterns. Both patches are ready for merge. --- Generated by Claude Code Patch Reviewer