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]

Message-ID: <1b919195757c49769bde19c59a846789@AcuMS.aculab.com>
Date:   Wed, 12 Oct 2022 16:13:40 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Sergei Antonov' <saproj@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "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 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 ??

Although traditionally it was 1514+crc.
An extra 4 byte header is now allowed.
There is also the usefulness of supporting full length frames
with a PPPoE header.

Whether it actually makes sense to round up the receive buffer
size and associated max frame length to 1536 (cache line aligned)
is another matter (probably 1534 for 4n+2 alignment).

> 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?
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??

> * Check for packet size > 1518 in ftmac100_rx_packet_error().
> 
> [1]
> https://bitbucket.org/Kasreyn/mkrom-uc7112lx/src/master/documents/FIC8120_DS_v1.2.pdf
> 
> Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")
> Signed-off-by: Sergei Antonov <saproj@...il.com>
> ---
> 
> v1 -> v2:
> * Typos in description fixed.
> 
>  drivers/net/ethernet/faraday/ftmac100.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
> index d95d78230828..34d0284079ff 100644
> --- a/drivers/net/ethernet/faraday/ftmac100.c
> +++ b/drivers/net/ethernet/faraday/ftmac100.c
> @@ -154,6 +154,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
>  				 FTMAC100_MACCR_CRC_APD	| \
>  				 FTMAC100_MACCR_FULLDUP	| \
>  				 FTMAC100_MACCR_RX_RUNT	| \
> +				 FTMAC100_MACCR_RX_FTL	| \
>  				 FTMAC100_MACCR_RX_BROADPKT)
> 
>  static int ftmac100_start_hw(struct ftmac100 *priv)
> @@ -320,6 +321,7 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
>  {
>  	struct net_device *netdev = priv->netdev;
>  	bool error = false;
> +	const unsigned int length = ftmac100_rxdes_frame_length(rxdes);

Do you need to read this value this early in the function?
Looks like it is only used when overlong packets are reported.

	David

> 
>  	if (unlikely(ftmac100_rxdes_rx_error(rxdes))) {
>  		if (net_ratelimit())
> @@ -337,9 +339,16 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
>  		error = true;
>  	}
> 
> -	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
> +	/* The frame-too-long flag 'FTMAC100_RXDES0_FTL' is described in the
> +	 * datasheet as: "When set, it indicates that the received packet
> +	 * length exceeds 1518 bytes." But testing shows that it is also set
> +	 * when packet length is equal to 1518.
> +	 * Since 1518 is a standard Ethernet maximum frame size, let it pass
> +	 * and only trigger an error when packet length really exceeds it.
> +	 */
> +	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes) && length > 1518)) {
>  		if (net_ratelimit())
> -			netdev_info(netdev, "rx frame too long\n");
> +			netdev_info(netdev, "rx frame too long (%u)\n", length);
> 
>  		netdev->stats.rx_length_errors++;
>  		error = true;
> --
> 2.34.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)