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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CABikg9zdg-WW+C-46Cy=gcgsd8ZEborOJkXOPUfxy9TmNEz_wg@mail.gmail.com>
Date:   Wed, 12 Oct 2022 19:42:31 +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 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

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

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

> > * 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.

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?