[PATCH v2 0/3] hv_netvsc: Fix leaking of send/receive buffers after GPADL teardown error

[PATCH v2 0/3] hv_netvsc: Fix leaking of send/receive buffers after GPADL teardown error

[Michael Kelley]

When a Hyper-V netvsc device is shutdown, the GPADLs for the device's
large send and receive buffers are torn down. If the teardown fails,
Hyper-V retain access to the buffers and may continue to read or write
them. Consequently the buffers should be leaked after such an error.

Leaking the buffers used to work correctly, but was broken by a
commit applied in the 4.16 kernel. This patch series restores the
correct leaking behavior.

Because the large buffers are allocated with wrapper functions
that allow allocations larger than MAX_ORDER_NR_PAGES, leaking the
memory is not as simple as setting the memory pointer to NULL so
vfree() or kfree() does nothing. A new function is introduced
to specify that the large buffers should be leaked.

Patch 1: Fix a bug vmbus_teardown_gpadl() where an error status is lost.

Patch 2: Introduce vmbus_leak_buffer() that causes vmbus_free_buffer()
to leak the buffers.

Patch 3: Use vmbus_leak_buffer() in the netvsc driver.

Tested by hacking in code to return errors from vmbus_post_msg()
and set_memory_encrypted() as called in vmbus_teardown_gpadl().
Then did unbind/rebind cycles on hv_netvsc and hv_balloon devices
in a normal VM and in an SEV-SNP CoCo VM. Verified that the expected
errors are generated in dmesg and that memory and vmalloc/vmap
resources are freed or leaked as expected.

Changes in v2:
* Add a new patch as Patch 1 to correct error return from
  vmbus_teardown_gpadl()
* Patch 3: Tweak the commit message
* Patch 3: Remove now superfluous return statements

Michael Kelley (3):
  Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl()
  Drivers: hv: Add vmbus_leak_buffer()
  hv_netvsc: Leak send/recv buffers if GPADL teardown fails

 drivers/hv/channel.c        | 36 +++++++++++++++++++++++++++++++-----
 drivers/net/hyperv/netvsc.c |  8 ++++++--
 include/linux/hyperv.h      |  4 ++++
 3 files changed, 41 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl()

[Michael Kelley]

In vmbus_teardown_gpadl(), the return value from vmbus_post_msg() is
overwritten by the logic that decides if set_memory_encrypted() should
run. A failure from vmbus_post_msg() is lost and vmbus_teardown_gpadl()
may incorrectly report success. Furthermore, if vmbus_post_msg() fails,
the GPADL remains active on the Hyper-V side, yet in a CoCo VM the
buffer will be re-encrypted anyway.

Fix this by gating buffer re-encryption on success from vmbus_post_msg().
And if either function fails, mark the buffer as decrypted so the memory
will be leaked. The decrypted flag does double-duty: in a CoCo VM it
indicates the decryption status, but at buffer cleanup time in all VMs
it is a "should be leaked due to error" flag.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hyperv/20260904160724.82AB51F00A3D@smtp.kernel.org/
Fixes: d4dccf353db8 ("Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/hv/channel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index f4370617deac..cc86e8505ad0 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -1059,14 +1059,14 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
 
 	kfree(info);
 
-	if (gpadl->decrypted)
+	if (!ret && gpadl->decrypted) {
 		ret = set_memory_encrypted((unsigned long)gpadl->buffer,
 					PFN_UP(gpadl->size));
-	else
-		ret = 0;
-	if (ret)
-		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
+		if (ret)
+			pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
+	}
 
+	/* If error in ret, mark buffer decrypted so it is leaked */
 	gpadl->decrypted = ret;
 
 	return ret;
-- 
2.25.1

Re: [PATCH v2 1/3] Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl()

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260907214902.9046-2-mhklinux@outlook.com?part=1
---
    Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl()
    
    In vmbus_teardown_gpadl(), the return value from vmbus_post_msg() is
    overwritten by the logic that decides if set_memory_encrypted() should
    run. A failure from vmbus_post_msg() is lost and vmbus_teardown_gpadl()
    may incorrectly report success. Furthermore, if vmbus_post_msg() fails,
    the GPADL remains active on the Hyper-V side, yet in a CoCo VM the
    buffer will be re-encrypted anyway.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hyperv/20260904160724.82AB51F00A3D@smtp.kernel.org/
Fixes: d4dccf353db8 ("Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM")

> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index f4370617deacb..cc86e8505ad0b 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -1059,14 +1059,14 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
>  
>  	kfree(info);
>  
> -	if (gpadl->decrypted)
> +	if (!ret && gpadl->decrypted) {
>  		ret = set_memory_encrypted((unsigned long)gpadl->buffer,
>  					PFN_UP(gpadl->size));
> -	else
> -		ret = 0;
> -	if (ret)
> -		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
> +		if (ret)
> +			pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
> +	}
>  
> +	/* If error in ret, mark buffer decrypted so it is leaked */
>  	gpadl->decrypted = ret;

[Severity: High]
This is a pre-existing issue, but does this early return in
vmbus_teardown_gpadl() bypass the leak mechanism?

drivers/hv/channel.c:vmbus_teardown_gpadl() {
	...
	info = kzalloc(sizeof(*info) +
		       sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
	if (!info)
		return -ENOMEM;
	...
}

If this kzalloc() fails, gpadl->decrypted is not updated.

In a CoCo VM, __vmbus_open() establishes a GPADL for the ring buffer and
initializes gpadl->decrypted to false. If a subsequent step in
__vmbus_open() fails, it calls vmbus_teardown_gpadl() to roll back:

drivers/hv/channel.c:__vmbus_open() {
	...
error_free_gpadl:
	vmbus_teardown_gpadl(newchannel, &newchannel->ringbuffer_gpadlhandle);
error_clean_ring:
	...
	return err;
}

Since __vmbus_open() ignores the teardown failure, vmbus_open() will then
proceed to call vmbus_free_ring():

drivers/hv/channel.c:vmbus_free_ring() {
	...
		/* In a CoCo VM leak the memory if it didn't get re-encrypted */
		if (!channel->ringbuffer_gpadlhandle.decrypted)
			__free_pages(channel->ringbuffer_page,
			...
}

Because gpadl->decrypted remains false instead of being set to -ENOMEM,
vmbus_free_ring() will erroneously free the memory back to the kernel page
allocator. Could this lead to guest memory corruption if the host hypervisor
retains an active GPADL mapping and overwrites it?

>  
>  	return ret;

[PATCH v2 2/3] Drivers: hv: Add vmbus_leak_buffer()

[Michael Kelley]

If an error case needs to leak the buffer memory allocated by
vmbus_alloc_buffer(), doing so requires knowledge of how
vmbus_free_buffer() works. In a CoCo VM buffers are allocated
differently from a normal VM, and vmbus_free_buffer() handles
the difference.

Encapsulate this knowledge in a new function, vmbus_leak_buffer(),
that error paths can call. After calling vmbus_leak_buffer(), a
subsequent call to vmbus_free_buffer() frees the additional
resources used in the CoCo VM case but does not free the actual
buffer memory. As such, vmbus_leak_buffer() is callable in a
context where accesses to the buffer memory may be in flight.

Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/hv/channel.c   | 26 ++++++++++++++++++++++++++
 include/linux/hyperv.h |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index cc86e8505ad0..389b27cb038d 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -648,6 +648,32 @@ void vmbus_free_buffer(void *addr, struct page **chunks, u32 chunk_cnt)
 }
 EXPORT_SYMBOL_GPL(vmbus_free_buffer);
 
+/**
+ * vmbus_leak_buffer - set up a buffer to be leaked by vmbus_free_buffer().
+ *
+ * @addr: buffer address
+ * @chunks: chunks array from vmbus_alloc_buffer()
+ * @chunk_cnt: number of entries in @chunks
+ *
+ * When @chunks is NULL the buffer is a plain vzalloc() allocation and
+ * the buffer is leaked by setting @addr to NULL. Otherwise set
+ * @chunk_cnt to 0 so that vmbus_free_buffer() does not try to re-encrypt
+ * or free the buffer memory, but still releases the vmap address and
+ * the chunks memory.
+ *
+ * This function may be called in a context where the buffer is still
+ * being accessed. It must not remove any kernel virtual addresses of
+ * the buffer or change its encryption status.
+ */
+void vmbus_leak_buffer(void **addr, struct page ***chunks, u32 *chunk_cnt)
+{
+	if (*chunks)
+		*chunk_cnt = 0;
+	else
+		*addr = NULL;
+}
+EXPORT_SYMBOL_GPL(vmbus_leak_buffer);
+
 /**
  * vmbus_alloc_buffer - allocate a host-visible, virtually-contiguous buffer.
  *
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 9e109d91aa14..6f60ce5924ba 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1220,6 +1220,10 @@ extern void *vmbus_alloc_buffer(struct vmbus_channel *channel,
 
 extern void vmbus_free_buffer(void *addr, struct page **chunks, u32 chunk_cnt);
 
+extern void vmbus_leak_buffer(void **addr,
+			      struct page ***chunks,
+			      u32 *chunk_cnt);
+
 void vmbus_reset_channel_cb(struct vmbus_channel *channel);
 
 extern int vmbus_recvpacket(struct vmbus_channel *channel,
-- 
2.25.1

[PATCH v2 3/3] hv_netvsc: Leak send/recv buffers if GPADL teardown fails

[Michael Kelley]

If GPADL teardown fails for the send or receive buffers, either
the Hyper-V host retains access to the buffers, or re-encryption of
the buffers failed. In either case, the intent is to be safe by
leaking the buffers instead of freeing them.

The intended behavior existed prior to commit 02400fcee254 ("hv_netvsc:
use RCU to fix concurrent rx and queue changes") because freeing
the buffers was done in the same function as the GPADL teardown.
The "return" statement in the error path effectively skipped freeing
the memory. But commit 02400fcee254 moved the freeing to a separate
function that is called later. It has no knowledge of the GPADL teardown
error, and so frees the memory regardless.

Fix this by calling vmbus_leak_buffer() if the respective GPADL
teardown fails. The later call to vmbus_free_buffer() then skips
freeing of the actual buffer, including any re-encryption required
in a CoCo VM.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hyperv/20260731201210.3653C1F00AC4@smtp.kernel.org/
Fixes: 02400fcee254 ("hv_netvsc: use RCU to fix concurrent rx and queue changes")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/net/hyperv/netvsc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 5cd084e5696c..e9292c3fac92 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -316,9 +316,11 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
 		 * rather than continue and a bugchk
 		 */
 		if (ret != 0) {
+			vmbus_leak_buffer(&net_device->recv_buf,
+					  &net_device->recv_buf_chunks,
+					  &net_device->recv_buf_chunk_cnt);
 			netdev_err(ndev,
 				   "unable to teardown receive buffer's gpadl\n");
-			return;
 		}
 	}
 }
@@ -337,9 +339,11 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
 		 * rather than continue and a bugchk
 		 */
 		if (ret != 0) {
+			vmbus_leak_buffer(&net_device->send_buf,
+					  &net_device->send_buf_chunks,
+					  &net_device->send_buf_chunk_cnt);
 			netdev_err(ndev,
 				   "unable to teardown send buffer's gpadl\n");
-			return;
 		}
 	}
 }
-- 
2.25.1

Re: [PATCH v2 3/3] hv_netvsc: Leak send/recv buffers if GPADL teardown fails

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260907214902.9046-2-mhklinux@outlook.com?part=3
---
hv_netvsc: Leak send/recv buffers if GPADL teardown fails

This commit addresses a bug where send/recv buffers were improperly freed if
GPADL teardown failed. It introduces calls to vmbus_leak_buffer() to safely
leak the buffers instead of freeing them when they might still be accessed by
the host.

Closes: https://lore.kernel.org/linux-hyperv/20260731201210.3653C1F00AC4@smtp.kernel.org/

> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 5cd084e5696cd..e9292c3fac925 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -316,9 +316,11 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
>  		 * rather than continue and a bugchk
>  		 */
>  		if (ret != 0) {
> +			vmbus_leak_buffer(&net_device->recv_buf,
> +					  &net_device->recv_buf_chunks,
> +					  &net_device->recv_buf_chunk_cnt);

[Severity: High]
Could this lead to a NULL pointer dereference if NAPI is still polling
concurrently?

In netvsc_device_add(), NAPI is enabled and the VMBus channel is opened. If a
subsequent initialization step fails (like send_buf setup), the error path
calls netvsc_teardown_recv_gpadl().

If the GPADL teardown fails, vmbus_leak_buffer() will set
net_device->recv_buf to NULL. Because NAPI is not synchronized with this
teardown and is disabled later in the error path, a concurrent softirq could
execute this path in netvsc_receive():

    char *recv_buf = net_device->recv_buf;
    ...
    data = recv_buf + offset;
    ...
    ret = rndis_filter_receive(ndev, net_device, nvchan, data, buflen);

Since the code does not check for NULL, it passes the faulty data pointer
(NULL + offset) to rndis_filter_receive().

This would result in a kernel panic when trying to read from the pointer:

    memcpy(rndis_msg, data, RNDIS_HEADER_SIZE);

Is there a way to defer setting recv_buf to NULL until after NAPI has been
disabled?

>  			netdev_err(ndev,
>  				   "unable to teardown receive buffer's gpadl\n");
> -			return;
>  		}
>  	}
>  }