* RE: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-08 14:42 [PATCH net v2] virtio-net: fix received length check in big packets Bui Quang Minh
@ 2025-07-08 14:52 ` Parav Pandit
2025-07-08 14:58 ` Bui Quang Minh
2025-07-10 9:57 ` Paolo Abeni
2025-07-16 13:59 ` Michael S. Tsirkin
2 siblings, 1 reply; 8+ messages in thread
From: Parav Pandit @ 2025-07-08 14:52 UTC (permalink / raw)
To: Bui Quang Minh, netdev@vger.kernel.org
Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Minggang(Gavin) Li, Gavi Teitz,
virtualization@lists.linux.dev, linux-kernel@vger.kernel.org
> From: Bui Quang Minh <minhquangbui99@gmail.com>
> Sent: 08 July 2025 08:12 PM
>
> Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big
> packets"), the allocated size for big packets is not MAX_SKB_FRAGS *
> PAGE_SIZE anymore but depends on negotiated MTU. The number of
> allocated frags for big packets is stored in
> vi->big_packets_num_skbfrags. This commit fixes the received length
> check corresponding to that change. The current incorrect check can lead to
> NULL page pointer dereference in the below while loop when erroneous
> length is received.
Do you mean a device has copied X bytes in receive buffer but device reports X+Y bytes in the ring?
>
> Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big
> packets")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
> Changes in v2:
> - Remove incorrect give_pages call
> ---
> drivers/net/virtio_net.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> 5d674eb9a0f2..3a7f435c95ae 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info
> *vi, {
> struct sk_buff *skb;
> struct virtio_net_common_hdr *hdr;
> - unsigned int copy, hdr_len, hdr_padded_len;
> + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
> struct page *page_to_free = NULL;
> int tailroom, shinfo_size;
> char *p, *hdr_p, *buf;
> @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct
> virtnet_info *vi,
> * tries to receive more than is possible. This is usually
> * the case of a broken device.
> */
> - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> + BUG_ON(offset >= PAGE_SIZE);
> + max_remaining_len = (unsigned int)PAGE_SIZE - offset;
> + max_remaining_len += vi->big_packets_num_skbfrags * PAGE_SIZE;
> + if (unlikely(len > max_remaining_len)) {
> net_dbg_ratelimited("%s: too much data\n", skb->dev-
> >name);
> dev_kfree_skb(skb);
> return NULL;
> }
> - BUG_ON(offset >= PAGE_SIZE);
> +
> while (len) {
> unsigned int frag_size = min((unsigned)PAGE_SIZE - offset,
> len);
> skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
> --
> 2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-08 14:52 ` Parav Pandit
@ 2025-07-08 14:58 ` Bui Quang Minh
0 siblings, 0 replies; 8+ messages in thread
From: Bui Quang Minh @ 2025-07-08 14:58 UTC (permalink / raw)
To: Parav Pandit, netdev@vger.kernel.org
Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Minggang(Gavin) Li, Gavi Teitz,
virtualization@lists.linux.dev, linux-kernel@vger.kernel.org
On 7/8/25 21:52, Parav Pandit wrote:
>> From: Bui Quang Minh <minhquangbui99@gmail.com>
>> Sent: 08 July 2025 08:12 PM
>>
>> Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big
>> packets"), the allocated size for big packets is not MAX_SKB_FRAGS *
>> PAGE_SIZE anymore but depends on negotiated MTU. The number of
>> allocated frags for big packets is stored in
>> vi->big_packets_num_skbfrags. This commit fixes the received length
>> check corresponding to that change. The current incorrect check can lead to
>> NULL page pointer dereference in the below while loop when erroneous
>> length is received.
> Do you mean a device has copied X bytes in receive buffer but device reports X+Y bytes in the ring?
Yes, that's what I mean. AFAIK, it's not checked in the ring level.
Thanks,
Quang Minh.
>
>> Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big
>> packets")
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>> Changes in v2:
>> - Remove incorrect give_pages call
>> ---
>> drivers/net/virtio_net.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
>> 5d674eb9a0f2..3a7f435c95ae 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info
>> *vi, {
>> struct sk_buff *skb;
>> struct virtio_net_common_hdr *hdr;
>> - unsigned int copy, hdr_len, hdr_padded_len;
>> + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
>> struct page *page_to_free = NULL;
>> int tailroom, shinfo_size;
>> char *p, *hdr_p, *buf;
>> @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct
>> virtnet_info *vi,
>> * tries to receive more than is possible. This is usually
>> * the case of a broken device.
>> */
>> - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
>> + BUG_ON(offset >= PAGE_SIZE);
>> + max_remaining_len = (unsigned int)PAGE_SIZE - offset;
>> + max_remaining_len += vi->big_packets_num_skbfrags * PAGE_SIZE;
>> + if (unlikely(len > max_remaining_len)) {
>> net_dbg_ratelimited("%s: too much data\n", skb->dev-
>>> name);
>> dev_kfree_skb(skb);
>> return NULL;
>> }
>> - BUG_ON(offset >= PAGE_SIZE);
>> +
>> while (len) {
>> unsigned int frag_size = min((unsigned)PAGE_SIZE - offset,
>> len);
>> skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
>> --
>> 2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-08 14:42 [PATCH net v2] virtio-net: fix received length check in big packets Bui Quang Minh
2025-07-08 14:52 ` Parav Pandit
@ 2025-07-10 9:57 ` Paolo Abeni
2025-07-10 10:44 ` Jason Wang
2025-07-16 13:59 ` Michael S. Tsirkin
2 siblings, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2025-07-10 9:57 UTC (permalink / raw)
To: Bui Quang Minh, netdev
Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Gavin Li, Gavi Teitz, Parav Pandit, virtualization, linux-kernel
On 7/8/25 4:42 PM, Bui Quang Minh wrote:
> Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length
> for big packets"), the allocated size for big packets is not
> MAX_SKB_FRAGS * PAGE_SIZE anymore but depends on negotiated MTU. The
> number of allocated frags for big packets is stored in
> vi->big_packets_num_skbfrags. This commit fixes the received length
> check corresponding to that change. The current incorrect check can lead
> to NULL page pointer dereference in the below while loop when erroneous
> length is received.
>
> Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big packets")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
> Changes in v2:
> - Remove incorrect give_pages call
> ---
> drivers/net/virtio_net.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5d674eb9a0f2..3a7f435c95ae 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> {
> struct sk_buff *skb;
> struct virtio_net_common_hdr *hdr;
> - unsigned int copy, hdr_len, hdr_padded_len;
> + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
> struct page *page_to_free = NULL;
> int tailroom, shinfo_size;
> char *p, *hdr_p, *buf;
> @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> * tries to receive more than is possible. This is usually
> * the case of a broken device.
> */
> - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> + BUG_ON(offset >= PAGE_SIZE);
Minor nit (not intended to block this patch): since you are touching
this, you could consider replacing the BUG_ON() with a:
if (WARN_ON_ONCE()) <goto error path>.
/P
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-10 9:57 ` Paolo Abeni
@ 2025-07-10 10:44 ` Jason Wang
2025-07-15 15:21 ` Lei Yang
2025-07-16 14:00 ` Michael S. Tsirkin
0 siblings, 2 replies; 8+ messages in thread
From: Jason Wang @ 2025-07-10 10:44 UTC (permalink / raw)
To: Paolo Abeni
Cc: Bui Quang Minh, netdev, Michael S. Tsirkin, Xuan Zhuo,
Eugenio Pérez, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Gavin Li, Gavi Teitz, Parav Pandit,
virtualization, linux-kernel
On Thu, Jul 10, 2025 at 5:57 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 7/8/25 4:42 PM, Bui Quang Minh wrote:
> > Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length
> > for big packets"), the allocated size for big packets is not
> > MAX_SKB_FRAGS * PAGE_SIZE anymore but depends on negotiated MTU. The
> > number of allocated frags for big packets is stored in
> > vi->big_packets_num_skbfrags. This commit fixes the received length
> > check corresponding to that change. The current incorrect check can lead
> > to NULL page pointer dereference in the below while loop when erroneous
> > length is received.
> >
> > Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big packets")
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> > Changes in v2:
> > - Remove incorrect give_pages call
> > ---
> > drivers/net/virtio_net.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 5d674eb9a0f2..3a7f435c95ae 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > {
> > struct sk_buff *skb;
> > struct virtio_net_common_hdr *hdr;
> > - unsigned int copy, hdr_len, hdr_padded_len;
> > + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
> > struct page *page_to_free = NULL;
> > int tailroom, shinfo_size;
> > char *p, *hdr_p, *buf;
> > @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > * tries to receive more than is possible. This is usually
> > * the case of a broken device.
> > */
> > - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> > + BUG_ON(offset >= PAGE_SIZE);
>
> Minor nit (not intended to block this patch): since you are touching
> this, you could consider replacing the BUG_ON() with a:
>
> if (WARN_ON_ONCE()) <goto error path>.
I'm not sure I get this, but using BUG_ON() can help to prevent bugs
from being explored.
Thanks
>
> /P
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-10 10:44 ` Jason Wang
@ 2025-07-15 15:21 ` Lei Yang
2025-07-16 14:00 ` Michael S. Tsirkin
1 sibling, 0 replies; 8+ messages in thread
From: Lei Yang @ 2025-07-15 15:21 UTC (permalink / raw)
To: Bui Quang Minh
Cc: Paolo Abeni, netdev, Michael S. Tsirkin, Xuan Zhuo,
Eugenio Pérez, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Jason Wang, Gavin Li, Gavi Teitz, Parav Pandit,
virtualization, linux-kernel
Tested this patch with virtio-net regression tests, everything works fine.
Tested-by: Lei Yang <leiyang@redhat.com>
On Thu, Jul 10, 2025 at 6:44 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jul 10, 2025 at 5:57 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On 7/8/25 4:42 PM, Bui Quang Minh wrote:
> > > Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length
> > > for big packets"), the allocated size for big packets is not
> > > MAX_SKB_FRAGS * PAGE_SIZE anymore but depends on negotiated MTU. The
> > > number of allocated frags for big packets is stored in
> > > vi->big_packets_num_skbfrags. This commit fixes the received length
> > > check corresponding to that change. The current incorrect check can lead
> > > to NULL page pointer dereference in the below while loop when erroneous
> > > length is received.
> > >
> > > Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big packets")
> > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Remove incorrect give_pages call
> > > ---
> > > drivers/net/virtio_net.c | 9 ++++++---
> > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 5d674eb9a0f2..3a7f435c95ae 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > {
> > > struct sk_buff *skb;
> > > struct virtio_net_common_hdr *hdr;
> > > - unsigned int copy, hdr_len, hdr_padded_len;
> > > + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
> > > struct page *page_to_free = NULL;
> > > int tailroom, shinfo_size;
> > > char *p, *hdr_p, *buf;
> > > @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > * tries to receive more than is possible. This is usually
> > > * the case of a broken device.
> > > */
> > > - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> > > + BUG_ON(offset >= PAGE_SIZE);
> >
> > Minor nit (not intended to block this patch): since you are touching
> > this, you could consider replacing the BUG_ON() with a:
> >
> > if (WARN_ON_ONCE()) <goto error path>.
>
> I'm not sure I get this, but using BUG_ON() can help to prevent bugs
> from being explored.
>
> Thanks
>
> >
> > /P
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-10 10:44 ` Jason Wang
2025-07-15 15:21 ` Lei Yang
@ 2025-07-16 14:00 ` Michael S. Tsirkin
1 sibling, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2025-07-16 14:00 UTC (permalink / raw)
To: Jason Wang
Cc: Paolo Abeni, Bui Quang Minh, netdev, Xuan Zhuo,
Eugenio Pérez, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Gavin Li, Gavi Teitz, Parav Pandit,
virtualization, linux-kernel
On Thu, Jul 10, 2025 at 06:44:03PM +0800, Jason Wang wrote:
> On Thu, Jul 10, 2025 at 5:57 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On 7/8/25 4:42 PM, Bui Quang Minh wrote:
> > > Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length
> > > for big packets"), the allocated size for big packets is not
> > > MAX_SKB_FRAGS * PAGE_SIZE anymore but depends on negotiated MTU. The
> > > number of allocated frags for big packets is stored in
> > > vi->big_packets_num_skbfrags. This commit fixes the received length
> > > check corresponding to that change. The current incorrect check can lead
> > > to NULL page pointer dereference in the below while loop when erroneous
> > > length is received.
> > >
> > > Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big packets")
> > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Remove incorrect give_pages call
> > > ---
> > > drivers/net/virtio_net.c | 9 ++++++---
> > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 5d674eb9a0f2..3a7f435c95ae 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > {
> > > struct sk_buff *skb;
> > > struct virtio_net_common_hdr *hdr;
> > > - unsigned int copy, hdr_len, hdr_padded_len;
> > > + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
> > > struct page *page_to_free = NULL;
> > > int tailroom, shinfo_size;
> > > char *p, *hdr_p, *buf;
> > > @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > * tries to receive more than is possible. This is usually
> > > * the case of a broken device.
> > > */
> > > - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> > > + BUG_ON(offset >= PAGE_SIZE);
> >
> > Minor nit (not intended to block this patch): since you are touching
> > this, you could consider replacing the BUG_ON() with a:
> >
> > if (WARN_ON_ONCE()) <goto error path>.
>
> I'm not sure I get this, but using BUG_ON() can help to prevent bugs
> from being explored.
>
> Thanks
You mean exploited.
Paolo what's your thought here? Why do you want to work around this
one, specifically? I don't see how we can get offset >= PAGE_SIZE.
> >
> > /P
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v2] virtio-net: fix received length check in big packets
2025-07-08 14:42 [PATCH net v2] virtio-net: fix received length check in big packets Bui Quang Minh
2025-07-08 14:52 ` Parav Pandit
2025-07-10 9:57 ` Paolo Abeni
@ 2025-07-16 13:59 ` Michael S. Tsirkin
2 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2025-07-16 13:59 UTC (permalink / raw)
To: Bui Quang Minh
Cc: netdev, Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Gavin Li, Gavi Teitz, Parav Pandit, virtualization, linux-kernel
On Tue, Jul 08, 2025 at 09:42:06PM +0700, Bui Quang Minh wrote:
> Since commit 4959aebba8c0 ("virtio-net: use mtu size as buffer length
> for big packets"), the allocated size for big packets is not
> MAX_SKB_FRAGS * PAGE_SIZE anymore but depends on negotiated MTU. The
> number of allocated frags for big packets is stored in
> vi->big_packets_num_skbfrags. This commit fixes the received length
> check corresponding to that change. The current incorrect check can lead
> to NULL page pointer dereference in the below while loop when erroneous
> length is received.
>
> Fixes: 4959aebba8c0 ("virtio-net: use mtu size as buffer length for big packets")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Changes in v2:
> - Remove incorrect give_pages call
> ---
> drivers/net/virtio_net.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5d674eb9a0f2..3a7f435c95ae 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -823,7 +823,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> {
> struct sk_buff *skb;
> struct virtio_net_common_hdr *hdr;
> - unsigned int copy, hdr_len, hdr_padded_len;
> + unsigned int copy, hdr_len, hdr_padded_len, max_remaining_len;
> struct page *page_to_free = NULL;
> int tailroom, shinfo_size;
> char *p, *hdr_p, *buf;
> @@ -887,12 +887,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> * tries to receive more than is possible. This is usually
> * the case of a broken device.
> */
> - if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> + BUG_ON(offset >= PAGE_SIZE);
> + max_remaining_len = (unsigned int)PAGE_SIZE - offset;
> + max_remaining_len += vi->big_packets_num_skbfrags * PAGE_SIZE;
> + if (unlikely(len > max_remaining_len)) {
> net_dbg_ratelimited("%s: too much data\n", skb->dev->name);
> dev_kfree_skb(skb);
> return NULL;
> }
> - BUG_ON(offset >= PAGE_SIZE);
> +
> while (len) {
> unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
> skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
> --
> 2.43.0
^ permalink raw reply [flat|nested] 8+ messages in thread