netdev - Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Oct 2022 13:29:00 +0300
From:   Sergei Antonov <saproj@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

On Thu, 13 Oct 2022 at 00:41, David Laight <David.Laight@...lab.com> wrote:
>
> From: Sergei Antonov
> > Sent: 12 October 2022 17:43
> >
> > On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@...lab.com> wrote:
> > >
> > > From: Sergei Antonov
> > > > Sent: 12 October 2022 16:38
> > > >
> > > > Despite the datasheet [1] saying the controller should allow incoming
> > > > packets of length >=1518, it only allows packets of length <=1514.
> > >
> > > Shouldn't that be <=1518 and <1518 ??
> >
> > Oh, thanks for noticing. But still it should be slightly different:
> > <= 1518 and <=1514
> > Here is my test results of different packet sizes:
> > packets of 1518 / 1517 / 1516 / 1515 bytes did not come to the driver
> > (before my patch)
> > packets of 1514 and less bytes did come
>
> I had to double check the frames sizes, not written an ethernet driver
> for nearly 30 years! There is a nice description that is 90% accurate
> at https://en.wikipedia.org/wiki/Ethernet_frame
>
> Without an 802.1Q tag (probably a VLAN tag?) the max frame has
> 1514 data bytes (inc mac addresses, but excl crc).
> Unless you are using VLANs that should be the frame limit.
> The IP+TCP is limited to the 1500 byte payload.

Exactly! Incoming packets first go through a switch chip (Marvell
88E6060), so packets should get tagged.

> So if the sender is generating longer packets it is buggy!

Looking into it.

On the sender computer:
sudo ifconfig eno1 mtu 1500 up
ssh receiver_computer

On the receiver computer:
in ftmac100_rx_packet_error() I call
ftmac100_rxdes_frame_length(rxdes) and it returns 1518. I suppose, it
is 1500 + 18(ethernet overhead) + 4(switch tag) - 4(crc).

Would you like me to dump the entire packet and verify?

> > If FTMAC100_MACCR_RX_FTL is set:
> >   the driver receives the "long" packet marked by the
> > FTMAC100_RXDES0_FTL flag. And these packets were discarded by the
> > driver (before my patch).
> >
> > > Looks like it might cause 'Frame Too Long' packets be returned.
> > > In which case should the code just have ignored it since
> > > longer frames would be discarded completely??
> >
> > Is there such a thing as a response packet which is sent in return to
> > FTL packet? Did not know that. My testcases were SSH and SCP programs
> > on Ubuntu 22 and they simply hang trying to connect to the ftmac100
> > device - no retransmissions or retries with smaller frames happened.
>
> Overlong frames should be discarded.
> The sender might choose to do PMTU (path MTU) detection,
> but probably doesn't unless a router is involved.

I am afraid the developers of ftmac100 controller did not take into
account the possibility of VAN tagging. So my patch is an attempt to
solve the issue. However, I am now doubtful about it. After my patch
the driver will not be correct for the case without a switch. Should I
instead of simple checking for "length > 1518" check a packet for VLAN
tag presence and then, depending on the result, for "length > 1514" or
"length > 1518"?

> ...
> > > Do you need to read this value this early in the function?
> > > Looks like it is only used when overlong packets are reported.
> >
> > I decided to make a variable in order to use it twice:
> > in the condition: "length > 1518"
> > in logging: "netdev_info(netdev, "rx frame too long (%u)\n", length);"
> > You are right saying it is not needed in most cases. Can we hope for
> > the optimizer to postpone the initialization of 'length' till it is
> > accessed?
>
> Unlikely unless there are no function calls and no volatile
> memory accesses.
> IMHO just because you can assign a value on the declaration
> (of a local) doesn't mean it is a good idea.
> Better to move it nearer the use (unless it is used throughout
> the function).

OK. I will rewrite this part in future versions of the patch.