[email protected] wrote: > - [Critical] Using `bytes_recvd` for `memcpy()` without checking > `vmbus_recvpacket()` return value leads to a massive heap buffer > overflow.
This one is bounded on this channel. hyperv_connect_vsp() calls vmbus_open() without setting max_pkt_size, so the inbound ring uses VMBUS_DEFAULT_MAX_PKT_SIZE (4096) and hv_pkt_iter_first() clamps the packet length to pkt_buffer_size. bytes_recvd therefore cannot exceed 4096, well under the 16 KiB recv_buf and init_buf, and vmbus_recvpacket() does not return -ENOBUFS here, so the memcpy length stays bounded. I will still gate the dispatch on a successful vmbus_recvpacket() return in the next revision, as defense in depth, so the bound is local instead of relying on the ring clamp. > - [High] Strict sizeof() validation incorrectly rejects > dynamically-sized SYNTHVID_RESOLUTION_RESPONSE packets. Agreed. The response carries resolution_count entries, not the full SYNTHVID_MAX_RESOLUTION_COUNT array, so checking against sizeof(struct synthvid_supported_resolution_resp) is too strict. The next revision validates the fixed prefix, reads and bounds resolution_count, then requires only the count-sized array. > - [High] Concurrent lockless write to `hv->init_buf` from VMBus > callback allows a malicious host to overwrite data while the guest > is validating it. > - [High] Missing `reinit_completion()` before reusing the shared > `hv->wait` completion object. Both pre-existing. On v2 Michael Kelley suggested splitting the completion reinit into a separate patch on the resume path. The init_buf reuse sits in the same area, so I plan to send the reinit and the related response-type handling as a separate follow-up rather than fold them into this size-validation change. Thanks for the review. Berkant
