Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] vmbus_teardown_gpadl masks vmbus_post_msg failures, leading to Use-After-Free/Memory Corruption in guest. - [High] Missing synchronization between `netvsc_device_remove` and concurrent TX path, leading to use-after-free or NULL pointer dereference in the VMBus ring buffer. -- --- Patch [3]: [PATCH v2 3/6] hv_netvsc: Remove GPADL teardown special case for old Hyper-V hosts --- commit 25bcea31eec6bb56b8730b7c31834e06d41d6a61 Author: Michael Kelley <[email protected]> hv_netvsc: Remove GPADL teardown special case for old Hyper-V hosts This commit removes the special case handling of GPADL teardown for Hyper-V hosts earlier than Windows Server 2016 and Windows 10. The receive and send GPADL teardowns are now always performed after the VMBus channel is closed. > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 4d319c50955ed..4295b96b43127 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -695,17 +695,8 @@ void netvsc_device_remove(struct hv_device *device) > = rtnl_dereference(net_device_ctx->nvdev); > int i; > > - /* > - * Revoke receive buffer. If host is pre-Win2016 then tear down > - * receive buffer GPADL. Do the same for send buffer. > - */ > netvsc_revoke_recv_buf(device, net_device, ndev); > - if (vmbus_proto_version < VERSION_WIN10) > - netvsc_teardown_recv_gpadl(device, net_device, ndev); > - > netvsc_revoke_send_buf(device, net_device, ndev); > - if (vmbus_proto_version < VERSION_WIN10) > - netvsc_teardown_send_gpadl(device, net_device, ndev); > > RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); > [ ... ] > @@ -733,14 +724,9 @@ void netvsc_device_remove(struct hv_device *device) > /* Now, we can close the channel safely */ > vmbus_close(device->channel); [Severity: High] This isn't a bug introduced by this patch, but is it possible for a concurrent TX path to race here and cause a use-after-free or NULL pointer dereference? In drivers/net/hyperv/netvsc_drv.c:netvsc_remove(), rndis_filter_device_remove() is called before unregister_netdevice(), which stops the TX queues: netvsc_remove() { if (nvdev) rndis_filter_device_remove(dev, nvdev); unregister_netdevice(net); } When netvsc_device_remove() (called from rndis_filter_device_remove) clears the net_device_ctx->nvdev pointer and immediately calls vmbus_close(), it doesn't wait for concurrent RCU readers like netvsc_start_xmit() to finish. If a concurrent TX thread has already read the pointer, couldn't it proceed to write to the VMBus ring buffer, which has just been freed and unmapped by vmbus_close()? > - /* > - * If host is Win2016 or higher then we do the GPADL tear down > - * here after VMBus is closed. > - */ > - if (vmbus_proto_version >= VERSION_WIN10) { > - netvsc_teardown_recv_gpadl(device, net_device, ndev); > - netvsc_teardown_send_gpadl(device, net_device, ndev); > - } > + /* Must do the GPADL teardown after channel is closed */ > + netvsc_teardown_recv_gpadl(device, net_device, ndev); [Severity: Critical] This is a pre-existing issue, but does this teardown sequence handle vmbus_post_msg() failures correctly? Looking at drivers/hv/channel.c:vmbus_teardown_gpadl(), it appears to mask the failure code for non-confidential VMs: vmbus_teardown_gpadl() { post_msg_err: /* ... */ if (gpadl->decrypted) ret = set_memory_encrypted((unsigned long)gpadl->buffer, PFN_UP(gpadl->size)); else ret = 0; } If vmbus_post_msg() fails (e.g., returning -ENOBUFS), this overwrites the error with 0. The caller, netvsc_teardown_recv_gpadl(), assumes a return value of 0 means the host successfully tore down the GPADL: netvsc_teardown_recv_gpadl() { if (net_device->recv_buf_gpadl_handle.gpadl_handle) { ret = vmbus_teardown_gpadl(device->channel, &net_device->recv_buf_gpadl_handle); /* If we failed here, we might as well return and have a leak * rather than continue and a bugchk */ if (ret != 0) { ... } } } Since vmbus_teardown_gpadl() silently returns 0 on failure, the network driver will proceed to free the receive/send buffers. Could the host still consider these buffers active and overwrite the newly freed memory with incoming network packets, leading to guest memory corruption? > + netvsc_teardown_send_gpadl(device, net_device, ndev); > > /* Release all resources */ > free_netvsc_device_rcu(net_device); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
