netdev - Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

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]

Date:	Mon, 3 Oct 2011 21:05:17 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Sasha Levin <levinsasha928@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Rusty Russell <rusty@...tcorp.com.au>,
	virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
	kvm@...r.kernel.org
Subject: Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:
> This patch prevents a NULL dereference when the user has passed a length
> longer than an actual buffer to virtio-net.
> 
> Cc: Rusty Russell <rusty@...tcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@...hat.com>
> Cc: virtualization@...ts.linux-foundation.org
> Cc: netdev@...r.kernel.org
> Cc: kvm@...r.kernel.org
> Signed-off-by: Sasha Levin <levinsasha928@...il.com>
> ---
>  drivers/net/virtio_net.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index bde0dec..4a53d2a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -208,12 +208,22 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  		return NULL;
>  	}
>  
> -	while (len) {
> +	while (len && page) {

I think if (unlikely(!page))
		goto err_page;

would make the logic clearer. But see below

>  		set_skb_frag(skb, page, offset, &len);
>  		page = (struct page *)page->private;
>  		offset = 0;
>  	}
>  
> +	/*
> +	 * This is the case where we ran out of pages in our linked list, but
> +	 * supposedly have more data to read.
> +	 */	
> +	if (len > 0) {
> +		pr_debug("%s: missing data to assemble skb\n", skb->dev->name);
> +		dev_kfree_skb(skb);
> +		return NULL;
> +	}
> +
>  	if (page)
>  		give_pages(vi, page);
>

I thought about this some more. If length was too large host is
right now writing into pages that we have freed.
That is very bad, and I don't know what do do with it,
really not worth prettifying that IMO, NULL pointer is the least of our
worries.

But, with mergeable buffers at least, and that is the main mode anyway,
there is always a single page per buf, right?
So I think we should change the code,
and for mergeable buffers we shall only verify that
length <= PAGE_SIZE.

IOW copy a bit of code from page_to_skb(vi, page, len)
to receive_mergeable, maybe add a common function
if we need to avoid duplication.

  
> -- 
> 1.7.6.1
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html