Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] hv_netvsc: Double-free and UAF on duplicate RNDIS response - [Medium] selftests: rndis_request_size_premise_test fails unconditionally on architectures with >4KB pages - [Medium] netvsc: netvsc_dma_map() leaks earlier DMA mappings when dma_map_single() fails - [Medium] hyperv: netvsc: DMA mapping leak in RNDIS_MSG_HALT control message - [Low] kconfig: HYPERV_NET_KUNIT_TEST cannot be enabled when driver is built as a module --
commit 91156bed3a78e0ac932d5c6bf8e50ef88aa52fa1 Author: Emerson Busson <[email protected]> hv: netvsc: allocate RNDIS request descriptors with kvzalloc_obj() This commit changes the allocation of RNDIS control requests from kzalloc_obj() to kvzalloc_obj() to avoid order-2 page allocation failures under buddy fragmentation. It resolves the vmalloc-backed descriptor into an array of page buffers for the DMA layout and includes KUnit test cases to cover the allocation and layout requirements. > diff --git a/drivers/net/hyperv/Kconfig b/drivers/net/hyperv/Kconfig > index 982964c1a9fb9..226728fd77d71 100644 > --- a/drivers/net/hyperv/Kconfig > +++ b/drivers/net/hyperv/Kconfig > @@ -6,3 +6,15 @@ config HYPERV_NET > select NLS > help > Select this option to enable the Hyper-V virtual network driver. > + > +config HYPERV_NET_KUNIT_TEST > + bool "Build Hyper-V netvsc KUnit tests" > + depends on HYPERV_NET && KUNIT [Severity: Low] Will this test be unselectable for modular builds? Because HYPERV_NET_KUNIT_TEST is a bool depending on the tristate HYPERV_NET, if HYPERV_NET is built as a module (=m), this boolean symbol will be clamped to n. Should this use depends on HYPERV_NET != n && KUNIT instead? > + default KUNIT_ALL_TESTS > + help > + Build the netvsc KUnit test suites into the hv_netvsc object. [ ... ] > diff --git a/drivers/net/hyperv/rndis_filter.c > b/drivers/net/hyperv/rndis_filter.c > index 9b6c44979b4ea..1ce40baa2d239 100644 > --- a/drivers/net/hyperv/rndis_filter.c > +++ b/drivers/net/hyperv/rndis_filter.c [ ... ] > @@ -115,8 +102,8 @@ static struct rndis_request *get_rndis_request(struct > rndis_device *dev, > return request; > } > > -static void put_rndis_request(struct rndis_device *dev, > - struct rndis_request *req) > +void put_rndis_request(struct rndis_device *dev, > + struct rndis_request *req) > { > unsigned long flags; > > @@ -124,7 +111,8 @@ static void put_rndis_request(struct rndis_device *dev, > list_del(&req->list_ent); > spin_unlock_irqrestore(&dev->request_lock, flags); > > - kfree(req); > + /* Paired with the kvzalloc_obj() in get_rndis_request(). */ > + kvfree(req); > } [Severity: Medium] Does freeing the request here permanently leak its DMA mapping when the device is halted? When rndis_filter_halt_device() sends an RNDIS_MSG_HALT request, it gets mapped via netvsc_dma_map(). Because it expects no response, rndis_filter_receive_response() is never executed to unmap it. Additionally, the TX completion path in netvsc_send_tx_complete() skips unmapping for control messages since skb is NULL. When rndis_filter_halt_device() subsequently calls put_rndis_request(), the request and its req->pkt.dma_range array are freed, but dma_unmap_single() is never called, leaking the SWIOTLB mappings. [Severity: Critical] Can a duplicate response from the host cause a double-free and use-after-free here? When rndis_filter_receive_response() processes a response, it looks up the request in dev->req_list but does not remove it: rndis_filter_receive_response() { spin_lock_irqsave(&dev->request_lock, flags); list_for_each_entry(request, &dev->req_list, list_ent) { if (request->request_msg.msg.init_req.req_id == *req_id) { found = true; break; } } spin_unlock_irqrestore(&dev->request_lock, flags); ... } If the host sends a duplicate RNDIS response with the same req_id, and the first response has already woken the waiting thread, the waiting thread can call put_rndis_request() to remove and free the request. The duplicate response RX thread will then use the freed request pointer and call netvsc_dma_unmap(), which calls kfree(packet->dma_range) without checking for NULL. For isolated VMs (like AMD SEV-SNP or Intel TDX), could an untrusted Hyper-V host use this to compromise the guest? > static void dump_rndis_message(struct net_device *netdev, [ ... ] > @@ -221,27 +209,78 @@ static void dump_rndis_message(struct net_device > *netdev, > } > } > > +int rndis_build_page_buffers(const void *data, u32 len, > + struct hv_page_buffer *page_bufs, > + u32 *page_buf_cnt) [ ... ] > static int rndis_filter_send_request(struct rndis_device *dev, > struct rndis_request *req) > { > struct hv_netvsc_packet *packet; > - struct hv_page_buffer pb; > + struct hv_page_buffer page_bufs[MAX_PAGE_BUFFER_COUNT]; > + u32 page_buf_cnt; > int ret; > > /* Setup the packet to send it */ > packet = &req->pkt; > > packet->total_data_buflen = req->request_msg.msg_len; > - packet->page_buf_cnt = 1; > - > - pb.pfn = virt_to_phys(&req->request_msg) >> HV_HYP_PAGE_SHIFT; > - pb.len = req->request_msg.msg_len; > - pb.offset = offset_in_hvpage(&req->request_msg); > + ret = rndis_build_page_buffers(&req->request_msg, > + req->request_msg.msg_len, > + page_bufs, &page_buf_cnt); > + if (ret) > + return ret; > + packet->page_buf_cnt = page_buf_cnt; > > trace_rndis_send(dev->ndev, 0, &req->request_msg); > > rcu_read_lock_bh(); > - ret = netvsc_send(dev->ndev, packet, NULL, &pb, NULL, false); > + ret = netvsc_send(dev->ndev, packet, NULL, page_bufs, NULL, false); > rcu_read_unlock_bh(); [Severity: Medium] Because page_buf_cnt can now be greater than 1, does this introduce a resource leak if DMA mapping partially fails? Inside netvsc_dma_map(), it iterates over page_buf_cnt to map the pages: netvsc_dma_map() { ... for (i = 0; i < page_count; i++) { ... dma = dma_map_single(&hv_dev->device, src, len, DMA_TO_DEVICE); if (dma_mapping_error(&hv_dev->device, dma)) { kfree(packet->dma_range); return -ENOMEM; } } ... } If dma_map_single() fails on a later iteration (when i > 0), it returns -ENOMEM but neglects to call dma_unmap_single() for the pages 0 to i - 1 that were successfully mapped in previous loop iterations. > diff --git a/drivers/net/hyperv/rndis_request_test.c > b/drivers/net/hyperv/rndis_request_test.c > new file mode 100644 > index 0000000000000..6affa916e74e3 > --- /dev/null > +++ b/drivers/net/hyperv/rndis_request_test.c [ ... ] > +static void rndis_request_size_premise_test(struct kunit *test) > +{ > + KUNIT_EXPECT_GT(test, sizeof(struct rndis_request), > + (size_t)KMALLOC_MAX_CACHE_SIZE); [Severity: Medium] Will this assertion unconditionally fail on architectures with page sizes larger than 4KB? KMALLOC_MAX_CACHE_SIZE scales with PAGE_SIZE (typically PAGE_SIZE * 2). On architectures like ARM64 or PPC64 configured with 64KB pages, KMALLOC_MAX_CACHE_SIZE is at least 64 KiB. Since sizeof(struct rndis_request) is slightly larger than 8 KiB (because RNDIS_EXT_LEN is fixed at 4096), this check will evaluate to roughly 8272 > 65536 on those platforms, causing the test suite to spuriously fail. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
