linux-kernel - Re: [PATCH net] net: stmmac: Guard against txfifosz=0

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: <20200412112756.687ff227@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sun, 12 Apr 2020 11:27:56 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, olteanv@...il.com, mripard@...nel.org,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        linux-stm32@...md-mailman.stormreply.com (moderated list:ARM/STM32 
        ARCHITECTURE),
        linux-arm-kernel@...ts.infradead.org (moderated list:ARM/STM32 
        ARCHITECTURE), linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH net] net: stmmac: Guard against txfifosz=0

On Sat, 11 Apr 2020 20:49:31 -0700 Florian Fainelli wrote:
> After commit bfcb813203e619a8960a819bf533ad2a108d8105 ("net: dsa:
> configure the MTU for switch ports") my Lamobo R1 platform which uses
> an allwinner,sun7i-a20-gmac compatible Ethernet MAC started to fail
> by rejecting a MTU of 1536. The reason for that is that the DMA
> capabilities are not readable on this version of the IP, and there is
> also no 'tx-fifo-depth' property being provided in Device Tree. The
> property is documented as optional, and is not provided.
> 
> The minimum MTU that the network device accepts is ETH_ZLEN - ETH_HLEN,
> so rejecting the new MTU based on the txfifosz value unchecked seems a
> bit too heavy handed here.

OTOH is it safe to assume MTUs up to 16k are valid if device tree lacks
the optional property? Is this change purely to preserve backward
(bug-ward?) compatibility, even if it's not entirely correct to allow
high MTU values? (I think that'd be worth stating in the commit message
more explicitly.) Is there no "reasonable default" we could select for
txfifosz if property is missing?

> Fixes: eaf4fac47807 ("net: stmmac: Do not accept invalid MTU values")
> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e6898fd5223f..9c63ba6f86a9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3993,7 +3993,7 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  	new_mtu = STMMAC_ALIGN(new_mtu);
>  
>  	/* If condition true, FIFO is too small or MTU too large */
> -	if ((txfifosz < new_mtu) || (new_mtu > BUF_SIZE_16KiB))
> +	if ((txfifosz < new_mtu && txfifosz) || (new_mtu > BUF_SIZE_16KiB))
>  		return -EINVAL;
>  
>  	dev->mtu = new_mtu;