[RFC PATCH-for-5.2 0/2] net: Do not accept packets with invalid huge size

[RFC PATCH-for-5.2 0/2] net: Do not accept packets with invalid huge size

[Philippe Mathieu-Daudé]

Hi,

This is a simple attempt to avoid the following pattern:

  ssize_t pkt_size = get_pkt_size(); // returns errno

  // no check

  send_packet(size_t size=pkt_size); // size casted to unsigned
                                     // -> overflow

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  net: Do not accept packets bigger then NET_BUFSIZE
  net: Assert no packet bigger than NET_BUFSIZE is queued

 net/net.c   | 4 ++++
 net/queue.c | 7 +++++++
 2 files changed, 11 insertions(+)

-- 
2.26.2

[RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE

[Philippe Mathieu-Daudé]

Do not allow qemu_send_packet*() and qemu_net_queue_send()
functions to accept packets bigger then NET_BUFSIZE.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
We have to put a limit somewhere. NET_BUFSIZE is defined as:

 /* Maximum GSO packet size (64k) plus plenty of room for
  * the ethernet and virtio_net headers
  */
 #define NET_BUFSIZE (4096 + 65536)

If we do want to accept bigger packets (i.e. multiple GSO packets
in a IOV), we could use INT32_MAX as limit...
---
 net/net.c   | 4 ++++
 net/queue.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/net/net.c b/net/net.c
index 6a2c3d95670..f29bfac2b11 100644
--- a/net/net.c
+++ b/net/net.c
@@ -644,6 +644,10 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
     qemu_hexdump(stdout, "net", buf, size);
 #endif
 
+    if (size > NET_BUFSIZE) {
+        return -1;
+    }
+
     if (sender->link_down || !sender->peer) {
         return size;
     }
diff --git a/net/queue.c b/net/queue.c
index 19e32c80fda..221a1c87961 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -191,6 +191,10 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
 {
     ssize_t ret;
 
+    if (size > NET_BUFSIZE) {
+        return -1;
+    }
+
     if (queue->delivering || !qemu_can_send_packet(sender)) {
         qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
         return 0;
-- 
2.26.2

Re: [RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE

[Jason Wang]

On 2020/11/27 下午11:45, Philippe Mathieu-Daudé wrote:
> Do not allow qemu_send_packet*() and qemu_net_queue_send()
> functions to accept packets bigger then NET_BUFSIZE.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> We have to put a limit somewhere. NET_BUFSIZE is defined as:
>
>   /* Maximum GSO packet size (64k) plus plenty of room for
>    * the ethernet and virtio_net headers
>    */
>   #define NET_BUFSIZE (4096 + 65536)
>
> If we do want to accept bigger packets (i.e. multiple GSO packets
> in a IOV), we could use INT32_MAX as limit...


This looks like a complaint for:

commit 25c01bd19d0e4b66f357618aeefda1ef7a41e21a
Author: Jason Wang <jasowang@redhat.com>
Date:   Tue Dec 4 11:53:43 2018 +0800

     net: drop too large packet early

which only fixes the iov version of the function.

If you don't see any real bug, I suggest to merge the fix in next release.

Thanks


> ---
>   net/net.c   | 4 ++++
>   net/queue.c | 4 ++++
>   2 files changed, 8 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 6a2c3d95670..f29bfac2b11 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -644,6 +644,10 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
>       qemu_hexdump(stdout, "net", buf, size);
>   #endif
>   
> +    if (size > NET_BUFSIZE) {
> +        return -1;
> +    }
> +
>       if (sender->link_down || !sender->peer) {
>           return size;
>       }
> diff --git a/net/queue.c b/net/queue.c
> index 19e32c80fda..221a1c87961 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -191,6 +191,10 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
>   {
>       ssize_t ret;
>   
> +    if (size > NET_BUFSIZE) {
> +        return -1;
> +    }
> +
>       if (queue->delivering || !qemu_can_send_packet(sender)) {
>           qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
>           return 0;

Re: [RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE

[Mauro Matteo Cascella]

Hello,

On Mon, Nov 30, 2020 at 3:36 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/27 下午11:45, Philippe Mathieu-Daudé wrote:
> > Do not allow qemu_send_packet*() and qemu_net_queue_send()
> > functions to accept packets bigger then NET_BUFSIZE.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > We have to put a limit somewhere. NET_BUFSIZE is defined as:
> >
> >   /* Maximum GSO packet size (64k) plus plenty of room for
> >    * the ethernet and virtio_net headers
> >    */
> >   #define NET_BUFSIZE (4096 + 65536)
> >
> > If we do want to accept bigger packets (i.e. multiple GSO packets
> > in a IOV), we could use INT32_MAX as limit...
>
>
> This looks like a complaint for:
>
> commit 25c01bd19d0e4b66f357618aeefda1ef7a41e21a
> Author: Jason Wang <jasowang@redhat.com>
> Date:   Tue Dec 4 11:53:43 2018 +0800
>
>      net: drop too large packet early
>
> which only fixes the iov version of the function.
>
> If you don't see any real bug, I suggest to merge the fix in next release.
>
> Thanks
>
>

Following is the reference bug along with a proposed patch, although I
guess the patch [2] is not strictly required once this patchset is
merged.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1899722
[2] https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05935.html

Thank you,
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE

[Philippe Mathieu-Daudé]

On 11/30/20 10:20 AM, Mauro Matteo Cascella wrote:
> Hello,
> 
> On Mon, Nov 30, 2020 at 3:36 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2020/11/27 下午11:45, Philippe Mathieu-Daudé wrote:
>>> Do not allow qemu_send_packet*() and qemu_net_queue_send()
>>> functions to accept packets bigger then NET_BUFSIZE.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> We have to put a limit somewhere. NET_BUFSIZE is defined as:
>>>
>>>   /* Maximum GSO packet size (64k) plus plenty of room for
>>>    * the ethernet and virtio_net headers
>>>    */
>>>   #define NET_BUFSIZE (4096 + 65536)
>>>
>>> If we do want to accept bigger packets (i.e. multiple GSO packets
>>> in a IOV), we could use INT32_MAX as limit...
>>
>>
>> This looks like a complaint for:
>>
>> commit 25c01bd19d0e4b66f357618aeefda1ef7a41e21a
>> Author: Jason Wang <jasowang@redhat.com>
>> Date:   Tue Dec 4 11:53:43 2018 +0800
>>
>>      net: drop too large packet early
>>
>> which only fixes the iov version of the function.
>>
>> If you don't see any real bug, I suggest to merge the fix in next release.

Fine by me, I don't have access to the big picture.

> Following is the reference bug along with a proposed patch, although I
> guess the patch [2] is not strictly required once this patchset is
> merged.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1899722
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05935.html

I didn't noticed your patch. While it prevents this kind of
error on a particular device, it doesn't for the rest.

Re: [RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE

[P J P]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

+-- On Fri, 27 Nov 2020, Philippe Mathieu-Daudé wrote --+
| Do not allow qemu_send_packet*() and qemu_net_queue_send()
| functions to accept packets bigger then NET_BUFSIZE.
| 
| We have to put a limit somewhere. NET_BUFSIZE is defined as:
|  /* Maximum GSO packet size (64k) plus plenty of room for
|   * the ethernet and virtio_net headers
|   */
|  #define NET_BUFSIZE (4096 + 65536)
| 
| +    if (size > NET_BUFSIZE) {
| +        return -1;
| +    }
| +

/usr/include/linux/if_ether.h
  #define ETH_MIN_MTU 68        /* Min IPv4 MTU per RFC791  */                      
  #define ETH_MAX_MTU 0xFFFFU   /* 65535, same as IP_MAX_MTU    */

  if (size < ETH_MIN_MTU || size > ETH_MAX_MTU) {
      return -1;
  }

Should there be similar check for minimum packet size?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [RFC PATCH-for-5.2 1/2] net: Do not accept packets bigger then NET_BUFSIZE

[Philippe Mathieu-Daudé]

On 12/4/20 11:03 AM, P J P wrote:
> +-- On Fri, 27 Nov 2020, Philippe Mathieu-Daudé wrote --+
> | Do not allow qemu_send_packet*() and qemu_net_queue_send()
> | functions to accept packets bigger then NET_BUFSIZE.
> | 
> | We have to put a limit somewhere. NET_BUFSIZE is defined as:
> |  /* Maximum GSO packet size (64k) plus plenty of room for
> |   * the ethernet and virtio_net headers
> |   */
> |  #define NET_BUFSIZE (4096 + 65536)
> | 
> | +    if (size > NET_BUFSIZE) {
> | +        return -1;
> | +    }
> | +
> 
> /usr/include/linux/if_ether.h
>   #define ETH_MIN_MTU 68        /* Min IPv4 MTU per RFC791  */                      
>   #define ETH_MAX_MTU 0xFFFFU   /* 65535, same as IP_MAX_MTU    */
> 
>   if (size < ETH_MIN_MTU || size > ETH_MAX_MTU) {
>       return -1;
>   }
> 
> Should there be similar check for minimum packet size?

I don't think so, because this API is not restricted to Ethernet
frames (i.e. CAN frames are much shorter).
We also want developers be able to experiment with new protocols.

I'd rather not relate this with any protocol. Using an upper limit
doesn't hurt. Maybe not accepting frames bigger than 1 GiB is enough
from a security point of view.

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
> 

[RFC PATCH-for-5.2 2/2] net: Assert no packet bigger than NET_BUFSIZE is queued

[Philippe Mathieu-Daudé]

Ensure no packet bigger then NET_BUFSIZE is queued via
qemu_net_queue_append*() by adding assertions.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/queue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/queue.c b/net/queue.c
index 221a1c87961..94b98b19ef9 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -102,6 +102,8 @@ static void qemu_net_queue_append(NetQueue *queue,
     if (queue->nq_count >= queue->nq_maxlen && !sent_cb) {
         return; /* drop if queue full and no callback */
     }
+
+    assert(size <= NET_BUFSIZE);
     packet = g_malloc(sizeof(NetPacket) + size);
     packet->sender = sender;
     packet->flags = flags;
@@ -131,6 +133,7 @@ void qemu_net_queue_append_iov(NetQueue *queue,
         max_len += iov[i].iov_len;
     }
 
+    assert(max_len <= NET_BUFSIZE);
     packet = g_malloc(sizeof(NetPacket) + max_len);
     packet->sender = sender;
     packet->sent_cb = sent_cb;
-- 
2.26.2

Re: [RFC PATCH-for-5.2 2/2] net: Assert no packet bigger than NET_BUFSIZE is queued

[Jason Wang]

On 2020/11/27 下午11:45, Philippe Mathieu-Daudé wrote:
> Ensure no packet bigger then NET_BUFSIZE is queued via
> qemu_net_queue_append*() by adding assertions.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   net/queue.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/net/queue.c b/net/queue.c
> index 221a1c87961..94b98b19ef9 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -102,6 +102,8 @@ static void qemu_net_queue_append(NetQueue *queue,
>       if (queue->nq_count >= queue->nq_maxlen && !sent_cb) {
>           return; /* drop if queue full and no callback */
>       }
> +
> +    assert(size <= NET_BUFSIZE);
>       packet = g_malloc(sizeof(NetPacket) + size);
>       packet->sender = sender;
>       packet->flags = flags;
> @@ -131,6 +133,7 @@ void qemu_net_queue_append_iov(NetQueue *queue,
>           max_len += iov[i].iov_len;
>       }
>   
> +    assert(max_len <= NET_BUFSIZE);
>       packet = g_malloc(sizeof(NetPacket) + max_len);
>       packet->sender = sender;
>       packet->sent_cb = sent_cb;


Anyway to avoid the assert here?

Thanks

Re: [RFC PATCH-for-5.2 0/2] net: Do not accept packets with invalid huge size

[Philippe Mathieu-Daudé]

On 11/27/20 4:45 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This is a simple attempt to avoid the following pattern:
> 
>   ssize_t pkt_size = get_pkt_size(); // returns errno

Sorry, I meant: returns "-errno" (< 0).

> 
>   // no check
> 
>   send_packet(size_t size=pkt_size); // size casted to unsigned
>                                      // -> overflow
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (2):
>   net: Do not accept packets bigger then NET_BUFSIZE
>   net: Assert no packet bigger than NET_BUFSIZE is queued
> 
>  net/net.c   | 4 ++++
>  net/queue.c | 7 +++++++
>  2 files changed, 11 insertions(+)
> 

Re: [RFC PATCH-for-5.2 0/2] net: Do not accept packets with invalid huge size

[Peter Maydell]

On Fri, 27 Nov 2020 at 15:45, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi,
>
> This is a simple attempt to avoid the following pattern:
>
>   ssize_t pkt_size = get_pkt_size(); // returns errno
>
>   // no check
>
>   send_packet(size_t size=pkt_size); // size casted to unsigned
>                                      // -> overflow

"RFC" and "for-5.2" are not a great combination at this point :-(
What are the consequences if we don't put this patchset in 5.2?

thanks
-- PMM

Re: [RFC PATCH-for-5.2 0/2] net: Do not accept packets with invalid huge size

[Philippe Mathieu-Daudé]

On 11/28/20 9:59 PM, Peter Maydell wrote:
> On Fri, 27 Nov 2020 at 15:45, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi,
>>
>> This is a simple attempt to avoid the following pattern:
>>
>>   ssize_t pkt_size = get_pkt_size(); // returns errno
>>
>>   // no check
>>
>>   send_packet(size_t size=pkt_size); // size casted to unsigned
>>                                      // -> overflow
> 
> "RFC" and "for-5.2" are not a great combination at this point :-(

"RFC" because I don't understand all the effects this assert
can have. "for-5.2" because it was raised as a security bug,
but I don't have access to the information, so I can not see
the big picture.

> What are the consequences if we don't put this patchset in 5.2?

Jason suggested to postpone this. If this is security important,
we can release a 5.2.1-stable tag early I suppose.

Regards,

Phil.