Re: [PATCH v2 3/6] hv_netvsc: Remove GPADL teardown special case for old Hyper-V hosts

[sashiko-bot@kernel.org]

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 <mhkelley58@gmail.com>

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/20260805203751.2678-1-mhklinux@outlook.com?part=3