diff mbox series

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

Message ID 20201127154524.1902024-2-philmd@redhat.com
State New
Headers show
Series net: Do not accept packets with invalid huge size | expand

Commit Message

Philippe Mathieu-Daudé Nov. 27, 2020, 3:45 p.m. UTC 
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(+)

Comments

Jason Wang Nov. 30, 2020, 2:36 a.m. UTC | #1 
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;
Mauro Matteo Cascella Nov. 30, 2020, 9:20 a.m. UTC | #2 
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
Philippe Mathieu-Daudé Nov. 30, 2020, 9:59 a.m. UTC | #3 
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.
Prasad Pandit Dec. 4, 2020, 10:03 a.m. UTC | #4 
+-- 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
Philippe Mathieu-Daudé Dec. 4, 2020, 2:01 p.m. UTC | #5 
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
>
diff mbox series

Patch

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;