Re: [PATCH v2 09/14] hv: use owned VMBus buffers in NetVSC and UIO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hv_netvsc: host bugcheck DoS due to GPADL teardown after failed revoke
--

commit 418182ccc1e62bfbe8d59179d5fb1baf3aaa6d53
Author: Emerson Busson <emersonbusson@gmail.com>

    hv: use owned VMBus buffers in NetVSC and UIO

Move the remaining buffer consumers to the allocation, GPADL and release
entry points that carry one descriptor throughout their lifetime. Preserve
established exported signatures throughout the series.

> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index ffba1443396ab..fd8aa7a3dcb32 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -243,7 +243,6 @@ static void netvsc_revoke_recv_buf(struct hv_device *device,
>  		if (ret != 0) {
>  			netdev_err(ndev, "unable to send "
>  				"revoke receive buffer to netvsp\n");
> -			net_device->recv_buffer.leak = true;
>  			return;
>  		}
>  		net_device->recv_section_cnt = 0;
> @@ -295,7 +294,6 @@ static void netvsc_revoke_send_buf(struct hv_device *device,
>  		if (ret != 0) {
>  			netdev_err(ndev, "unable to send "
>  				   "revoke send buffer to netvsp\n");
> -			net_device->send_buffer.leak = true;
>  			return;
>  		}
>  		net_device->send_section_cnt = 0;
> @@ -308,18 +306,14 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
>  {
>  	int ret;
>  
> -	if (net_device->recv_buffer.leak)
> -		return;
> -
>  	if (net_device->recv_buffer.gpadl.gpadl_handle) {
> -		ret = vmbus_teardown_gpadl(device->channel,
> -					   &net_device->recv_buffer.gpadl);
> +		ret = vmbus_teardown_gpadl_owned(device->channel,
> +						 &net_device->recv_buffer);

[Severity: High]
With the removal of the leak check here and the manual assignment in
netvsc_revoke_recv_buf(), is there a risk of a host bugcheck if the
protocol-level revoke message fails?

If vmbus_sendpacket() returns an error (like -EAGAIN from a full ring) in
netvsc_revoke_recv_buf(), the NetVSP layer won't receive the revoke message
and might still actively map the GPADL. Unconditionally sending a VMBus GPADL
teardown request in this scenario violates the protocol invariant. Could
this lead to a Denial of Service via a host bugcheck or guest termination?

>  
>  		/* If we failed here, we might as well return and have a leak
>  		 * rather than continue and a bugchk
>  		 */
>  		if (ret != 0) {
> -			net_device->recv_buffer.leak = true;
>  			netdev_err(ndev,
>  				   "unable to teardown receive buffer's gpadl\n");
>  			return;
> @@ -333,18 +327,14 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
>  {
>  	int ret;
>  
> -	if (net_device->send_buffer.leak)
> -		return;
> -
>  	if (net_device->send_buffer.gpadl.gpadl_handle) {
> -		ret = vmbus_teardown_gpadl(device->channel,
> -					   &net_device->send_buffer.gpadl);
> +		ret = vmbus_teardown_gpadl_owned(device->channel,
> +						 &net_device->send_buffer);

[Severity: High]
Similarly for the send buffer, does removing the leak check and flag tracking
in netvsc_revoke_send_buf() expose the host to the same bugcheck risk if
the revoke message fails to send?

>  
>  		/* If we failed here, we might as well return and have a leak
>  		 * rather than continue and a bugchk
>  		 */
>  		if (ret != 0) {
> -			net_device->send_buffer.leak = true;
>  			netdev_err(ndev,
>  				   "unable to teardown send buffer's gpadl\n");
>  			return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007190752.336426-1-emersonbusson@gmail.com?part=9