| 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
| ||
|
Message-ID: <0bf0811e-cdb8-4410-9b69-1c38b06bbadf@gmail.com>
Date: Thu, 26 Jun 2025 22:34:35 +0700
From: Bui Quang Minh <minhquangbui99@...il.com>
To: Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
<eperezma@...hat.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Stanislav Fomichev <sdf@...ichev.me>, virtualization@...ts.linux.dev,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net 1/4] virtio-net: ensure the received length does not
exceed allocated size
On 6/26/25 09:34, Jason Wang wrote:
> On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> <minhquangbui99@...il.com> wrote:
>> In xdp_linearize_page, when reading the following buffers from the ring,
>> we forget to check the received length with the true allocate size. This
>> can lead to an out-of-bound read. This commit adds that missing check.
>>
>> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> I think we should cc stable.
Okay, I'll do that in next version.
>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@...il.com>
>> ---
>> drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index e53ba600605a..2a130a3e50ac 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1797,7 +1797,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
>> * across multiple buffers (num_buf > 1), and we make sure buffers
>> * have enough headroom.
>> */
>> -static struct page *xdp_linearize_page(struct receive_queue *rq,
>> +static struct page *xdp_linearize_page(struct net_device *dev,
>> + struct receive_queue *rq,
>> int *num_buf,
>> struct page *p,
>> int offset,
>> @@ -1818,17 +1819,33 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
>> page_off += *len;
>>
>> while (--*num_buf) {
>> - unsigned int buflen;
>> + unsigned int headroom, tailroom, room;
>> + unsigned int truesize, buflen;
>> void *buf;
>> + void *ctx;
>> int off;
>>
>> - buf = virtnet_rq_get_buf(rq, &buflen, NULL);
>> + buf = virtnet_rq_get_buf(rq, &buflen, &ctx);
>> if (unlikely(!buf))
>> goto err_buf;
>>
>> p = virt_to_head_page(buf);
>> off = buf - page_address(p);
>>
>> + truesize = mergeable_ctx_to_truesize(ctx);
> This won't work for receive_small_xdp().
If it is small mode, the num_buf == 1 and we don't get into the while loop.
>
>> + headroom = mergeable_ctx_to_headroom(ctx);
>> + tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
>> + room = SKB_DATA_ALIGN(headroom + tailroom);
>> +
>> + if (unlikely(buflen > truesize - room)) {
>> + put_page(p);
>> + pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>> + dev->name, buflen,
>> + (unsigned long)(truesize - room));
>> + DEV_STATS_INC(dev, rx_length_errors);
>> + goto err_buf;
>> + }
> I wonder if this issue only affect XDP should we check other places?
In small mode, we check the len with GOOD_PACKET_LEN in receive_small.
In mergeable mode, we have some checks over the place and this is the
only one I see we miss. In xsk, we check inside buf_to_xdp. However, in
the big mode, I feel like there is a bug.
In add_recvbuf_big, 1 first page + vi->big_packets_num_skbfrags pages.
The pages are managed by a linked list. The vi->big_packets_num_skbfrags
is set in virtnet_set_big_packets
vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS :
DIV_ROUND_UP(mtu, PAGE_SIZE);
So the vi->big_packets_num_skbfrags can be fewer than MAX_SKB_FRAGS.
In receive_big, we call to page_to_skb, there is a check
if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
/* error case */
}
But because the number of allocated buffer is
vi->big_packets_num_skbfrags + 1 and vi->big_packets_num_skbfrags can be
fewer than MAX_SKB_FRAGS, the check seems not enough
while (len) {
unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
frag_size, truesize);
len -= frag_size;
page = (struct page *)page->private;
offset = 0;
}
In the following while loop, we keep running based on len without NULL
check the pages linked list, so it may result into NULL pointer dereference.
What do you think?
Thanks,
Quang Minh.
>
>> +
>> /* guard against a misconfigured or uncooperative backend that
>> * is sending packet larger than the MTU.
>> */
>> @@ -1917,7 +1934,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
>> headroom = vi->hdr_len + header_offset;
>> buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
>> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> - xdp_page = xdp_linearize_page(rq, &num_buf, page,
>> + xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
>> offset, header_offset,
>> &tlen);
>> if (!xdp_page)
>> @@ -2252,7 +2269,7 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
>> */
>> if (!xdp_prog->aux->xdp_has_frags) {
>> /* linearize data for XDP */
>> - xdp_page = xdp_linearize_page(rq, num_buf,
>> + xdp_page = xdp_linearize_page(vi->dev, rq, num_buf,
>> *page, offset,
>> XDP_PACKET_HEADROOM,
>> len);
>> --
>> 2.43.0
>>
Powered by blists - more mailing lists