netdev - Re: [PATCH net-next 1/2] virtio_net: Fix short frame length check

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:   Fri, 13 Jan 2023 16:36:50 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Parav Pandit <parav@...dia.com>
Cc:     "mst@...hat.com" <mst@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH net-next 1/2] virtio_net: Fix short frame length check

On Fri, Jan 13, 2023 at 4:23 PM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Fri, Jan 13, 2023 at 3:37 PM Parav Pandit <parav@...dia.com> wrote:
> >
> >
> > > From: Alexander H Duyck <alexander.duyck@...il.com>
> > > Sent: Friday, January 13, 2023 6:24 PM
> > >
> > > On Sat, 2023-01-14 at 00:36 +0200, Parav Pandit wrote:
> > > > A smallest Ethernet frame defined by IEEE 802.3 is 60 bytes without
> > > > any preemble and CRC.
> > > >
> > > > Current code only checks for minimal 14 bytes of Ethernet header length.
> > > > Correct it to consider the minimum Ethernet frame length.
> > > >
> > > > Fixes: 296f96fcfc16 ("Net driver using virtio")
> > > > Signed-off-by: Parav Pandit <parav@...dia.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> > > > 7723b2a49d8e..d45e140b6852 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -1248,7 +1248,7 @@ static void receive_buf(struct virtnet_info *vi,
> > > struct receive_queue *rq,
> > > >     struct sk_buff *skb;
> > > >     struct virtio_net_hdr_mrg_rxbuf *hdr;
> > > >
> > > > -   if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > > > +   if (unlikely(len < vi->hdr_len + ETH_ZLEN)) {
> > > >             pr_debug("%s: short packet %i\n", dev->name, len);
> > > >             dev->stats.rx_length_errors++;
> > > >             if (vi->mergeable_rx_bufs) {
> > >
> > > I'm not sure I agree with this change as packets are only 60B if they have gone
> > > across the wire as they are usually padded out on the transmit side. There may
> > > be cases where software routed packets may not be 60B.
> > >
> > Do you mean Linux kernel software? Any link to it would be helpful.
>
> The problem is there are several software paths involved and that is
> why I am wanting to be cautious. As I recall this would impact Qemu
> itself, DPDK, the Linux Kernel and several others if I am not
> mistaken. That is why I am tending to err on the side of caution as
> this is a pretty significant change.
>
> > > As such rather than changing out ETH_HLEN for ETH_ZLEN I wonder if we
> > > should look at maybe making this a "<=" comparison instead since that is the
> > > only case I can think of where the packet would end up being entirely empty
> > > after eth_type_trans is called and we would be passing an skb with length 0.
> >
> > I likely didn’t understand your comment.
> > This driver check is before creating the skb for the received packet.
> > So, purpose is to not even process the packet header or prepare the skb if it not an Ethernet frame.
> >
> > It is interesting to know when we get < 60B frame.
>
> If I recall, a UDPv4 frame can easily do it since Ethernet is 14B, IP
> header is 20, and UDP is only 8 so that only comes to 42B if I recall
> correctly. Similarly I think a TCPv4 Frame can be as small as 54B if
> you disable all the option headers.
>
> A quick and dirty test would be to run something like a netperf UDP_RR
> test. I know in the case of the network stack we see the transmits
> that go out are less than 60B until they are padded on xmit, usually
> by the device. My concern is wanting to make sure all those paths are
> covered before we assume that all the packets will be padded.

I was curious so I decided to try verifying things with a qemu w/ user
networking and virtio-net. From what I can tell it looks like it is
definitely not padding them out.

19:34:38.331376 IP (tos 0x0, ttl 64, id 31799, offset 0, flags [DF],
proto UDP (17), length 29)
    localhost.localdomain.59579 > _gateway.52701: [udp sum ok] UDP, length 1
        0x0000:  5255 0a00 0202 5254 0012 3456 0800 4500
        0x0010:  001d 7c37 4000 4011 a688 0a00 020f 0a00
        0x0020:  0202 e8bb cddd 0009 c331 6e
19:34:38.331431 IP (tos 0x0, ttl 64, id 45459, offset 0, flags [none],
proto UDP (17), length 29)
    _gateway.52701 > localhost.localdomain.59579: [udp sum ok] UDP, length 1
        0x0000:  5254 0012 3456 5255 0a00 0202 0800 4500
        0x0010:  001d b193 0000 4011 b12c 0a00 0202 0a00
        0x0020:  020f cddd e8bb 0009 c331 6e