netdev - Re: [PATCH 1/2] virtio-net: Verify page list size before fitting into skb

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: <20110926184445.GA22278@redhat.com>
Date:	Mon, 26 Sep 2011 21:44:45 +0300
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 1/2] virtio-net: Verify page list size before fitting
 into skb

On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote:
> This patch verifies that the length of a buffer stored in a linked list
> of pages is small enough to fit into a skb.
> 
> If the size is larger than a max size of a skb, it means that we shouldn't
> go ahead building skbs anyway since we won't be able to send the buffer as
> the user requested.
> 
> 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>

Interesting.  This is a theoretical issue, correct?
Not a crash you actually see.

This crash would mean device is giving us packets
that are way too large. Avoiding crashes even in the face of
a misbehaved device is a good idea, but should
we print a diagnostic to a system log?
Maybe rate-limited or print once to avoid filling
up the disk. Other places in driver print with pr_debug
I'm not sure that's right but better than nothing.

> ---
>  drivers/net/virtio_net.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..64e0717 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	unsigned int copy, hdr_len, offset;
>  	char *p;
>  
> +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)

unlikely()?

Also, this seems too aggressive: at this point len includes the header
and the linear part. The right place for this
test is probably where we fill in the frags, just before
while (len)

The whole can only happen when mergeable buffers
are disabled, right?


> +		return NULL;
> +
>  	p = page_address(page);
>  
>  	/* copy small packet so we can reuse these pages for small data */
> -- 
> 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