netdev - Re: [PATCH net-next] virtio-net: fix use-after-free in page_to

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:   Tue, 20 Apr 2021 20:11:49 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        syzbot <syzkaller@...glegroups.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH net-next] virtio-net: fix use-after-free in page_to_skb()



On 4/20/21 7:51 PM, Guenter Roeck wrote:

> 
> sh does indeed fail, with the same symptoms as before, but so far I was not
> able to track it down to a specific commit. The alpha failure is different,
> though. It is a NULL pointer access.
> 
> Anyway, testing ...
> 
> The patch below does indeed fix the problem I am seeing on sh.
> 
> ... and it does fix the alpha problem as well. Neat, though I don't really understand
> what a NULL pointer access and an unaligned access have to do with each other.
> 
> Great catch, thanks!
> 
> Guenter
> 

Note that build_skb(), without an additional skb_reserve(skb, NET_IP_ALIGN)
can not possibly work on arches that care about alignments.

That is because we can not both align skb->data and skb_shinfo(skb)

So unless we change build_skb() to make sure to align skb_shinfo(),
a fix could be simply :

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8cd76037c72481200ea3e8429e9fdfec005dad85..9cbe9c1737649450e451e3c65f59f794d1bf34b0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -415,7 +415,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 
        shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-       if (len > GOOD_COPY_LEN && tailroom >= shinfo_size) {
+       if (!_NET_IP_ALIGN && len > GOOD_COPY_LEN && tailroom >= shinfo_size) {
                skb = build_skb(p, truesize);
                if (unlikely(!skb))
                        return NULL;