Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Mon, 6 Nov 2023 14:40:19 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Hans Ulli Kroll <ulli.kroll@googlemail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <mirq-linux@rere.qmqm.pl>,
        Andrew Lunn <andrew@lunn.ch>,
        linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net v2 3/4] net: ethernet: cortina: Protect against
 oversized frames
Message-ID: <20231106124019.ethagifngefgcihm@skbuf>
References: <20231105-gemini-largeframe-fix-v2-0-cd3a5aa6c496@linaro.org>
 <20231105-gemini-largeframe-fix-v2-3-cd3a5aa6c496@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231105-gemini-largeframe-fix-v2-3-cd3a5aa6c496@linaro.org>
Precedence: bulk

On Sun, Nov 05, 2023 at 09:57:25PM +0100, Linus Walleij wrote:
> The max size of a transfer no matter the MTU is 64KB-1 so immediately
> bail out if the skb exceeds that.
> 
> The calling site tries to linearize the skbuff on error so return a
> special error code -E2BIG to indicate that this will not work in
> any way and bail out immediately if this happens.
> 
> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index b21a94b4ab5c..576174a862a9 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1151,6 +1151,12 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>  	if (skb->protocol == htons(ETH_P_8021Q))
>  		mtu += VLAN_HLEN;
>  
> +	if (skb->len > 65535) {
> +		/* The field for length is only 16 bits */
> +		netdev_err(netdev, "%s: frame too big, max size 65535 bytes\n", __func__);
> +		return -E2BIG;
> +	}
> +

Prints in the packet data path are extremely discouraged, since if they
trigger, they will spam your serial console and make it unusable.

I see that the out_drop label already bumps a counter. That should be
enough to signal there is a problem.

>  	word1 = skb->len;
>  	word3 = SOF_BIT;
>  
> @@ -1232,6 +1238,7 @@ static netdev_tx_t gmac_start_xmit(struct sk_buff *skb,
>  	struct gmac_txq *txq;
>  	int txq_num, nfrags;
>  	union dma_rwptr rw;
> +	int ret;
>  
>  	if (skb->len >= 0x10000)
>  		goto out_drop_free;

Since you already have this test, does the newly introduced one make
this redundant? Why not just change the limit here?

> @@ -1269,7 +1276,11 @@ static netdev_tx_t gmac_start_xmit(struct sk_buff *skb,
>  		}
>  	}
>  
> -	if (gmac_map_tx_bufs(netdev, skb, txq, &w)) {
> +	ret = gmac_map_tx_bufs(netdev, skb, txq, &w);
> +	if (ret == -E2BIG)
> +		goto out_drop;

Why out_drop and not out_drop_free? This handling will eventually cause an OOM.

The fact that it didn't makes me suspect that you never actually hit this condition,
because the network stack isn't delivering skbs larger than dev->mtu.
Maybe net/core/pktgen.c doesn't take the MTU into consideration, I'm not
completely sure there...

> +	if (ret) {
> +		/* Linearize and retry */
>  		if (skb_linearize(skb))
>  			goto out_drop;
>  
> 
> -- 
> 2.34.1
>