Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference
flat view
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2011-10-03 19:04:41
Also in:
kvm, lkml
On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:
quoted hunk ↗ jump to hunk
This patch prevents a NULL dereference when the user has passed a length longer than an actual buffer to virtio-net. Cc: Rusty Russell <redacted> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: virtualization@lists.linux-foundation.org Cc: netdev@vger.kernel.org Cc: kvm@vger.kernel.org Signed-off-by: Sasha Levin <redacted> --- 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
quoted hunk ↗ jump to hunk
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