linux-kernel - Re: [PATCH 1/2] vhost_net: correct error hanlding in vhost_net_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121227131442.GF20595@redhat.com>
Date:	Thu, 27 Dec 2012 15:14:42 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] vhost_net: correct error hanlding in
 vhost_net_set_backend()

On Thu, Dec 27, 2012 at 02:39:19PM +0800, Jason Wang wrote:
> Fix the leaking of oldubufs and fd refcnt when fail to initialized used ring.
> 
> Signed-off-by: Jason Wang <jasowang@...hat.com>
> ---
>  drivers/vhost/net.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index ebd08b2..629d6b5 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -834,8 +834,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>  		vhost_net_enable_vq(n, vq);
>  
>  		r = vhost_init_used(vq);
> -		if (r)
> -			goto err_vq;
> +		if (r) {
> +			sock = NULL;
> +			goto err_used;
> +		}
>  
>  		n->tx_packets = 0;
>  		n->tx_zcopy_err = 0;
> @@ -859,8 +861,14 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>  	mutex_unlock(&n->dev.mutex);
>  	return 0;
>  
> +err_used:
> +	if (oldubufs)
> +		vhost_ubuf_put_and_wait(oldubufs);
> +	if (oldsock)
> +		fput(oldsock->file);
>  err_ubufs:
> -	fput(sock->file);
> +	if (sock)
> +		fput(sock->file);
>  err_vq:
>  	mutex_unlock(&vq->mutex);
>  err:

I think it's a real bug, but I don't see how the fix
makes sense.
We are returning an error, so we ideally
revert to the state before the faulty
operation. So this should put sock and ubufs,
not oldsock/oldubufs.


The best way is probably to change
vhost_init_used so that it gets private data
pointer as a parameter.

We can then call it before ubuf alloc.
You can then add err_used right after err_ubufs
with no extra logic.




-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/