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
| ||
|
Message-ID: <f05f9dd9b39f42d18df0018c3596d866@AcuMS.aculab.com> Date: Wed, 12 Oct 2022 21:41:25 +0000 From: David Laight <David.Laight@...LAB.COM> To: 'Sergei Antonov' <saproj@...il.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 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. So if the sender is generating longer packets it is buggy! ... > > > Since 1518 is a standard Ethernet maximum frame size, and it can > > > easily be encountered (in SSH for example), fix this behavior: > > > > > > * Set FTMAC100_MACCR_RX_FTL in the MAC Control Register. > > > > What does that do? > > If FTMAC100_MACCR_RX_FTL is not set: > the driver does not receive the "long" packet at all. Looks like the > controller discards the packet without bothering the driver. Right so the existing check for the flag being set could never happen. > 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. ... > > 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). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Powered by blists - more mailing lists