netdev - Re: [PATCH net] net: fec: Coverity issue: Dereference null return value

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: <be98552a061f6249de558b210ff25de45e80d690.camel@gmail.com>
Date:   Fri, 16 Dec 2022 07:34:21 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     wei.fang@....com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, xiaoning.wang@....com,
        shenwei.wang@....com, linux-imx@....com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: fec: Coverity issue: Dereference null return
 value

On Thu, 2022-12-15 at 17:11 +0800, wei.fang@....com wrote:
> From: Wei Fang <wei.fang@....com>
> 
> The build_skb might return a null pointer but there is no check on the
> return value in the fec_enet_rx_queue(). So a null pointer dereference
> might occur. To avoid this, we check the return value of build_skb. If
> the return value is a null pointer, the driver will recycle the page and
> update the statistic of ndev. Then jump to rx_processing_done to clear
> the status flags of the BD so that the hardware can recycle the BD.
> 
> Signed-off-by: Wei Fang <wei.fang@....com>
> Reviewed-by: Shenwei Wang <Shenwei.wang@....com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 5528b0af82ae..c78aaa780983 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1674,6 +1674,16 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>  		 * bridging applications.
>  		 */
>  		skb = build_skb(page_address(page), PAGE_SIZE);
> +		if (unlikely(!skb)) {
> +			page_pool_recycle_direct(rxq->page_pool, page);
> +			ndev->stats.rx_packets--;
> +			ndev->stats.rx_bytes -= pkt_len;
> +			ndev->stats.rx_dropped++;

I'm not sure you really need to bother with rewinding the rx_packets
and rx_bytes counters. I know that the rx_dropped statistic will get
incremented in the network stack in the event of a packet failing to
enqueue to the backlog, so it might be better to just leave the
rx_packets counter as is and assume the actual packet count is
rx_packets - rx_dropped.

> +
> +			netdev_err(ndev, "build_skb failed!\n");

Instead of netdev_err you may want to consider netdev_err_once for
this. Generally speaking when we start seeing memory allocation error
issues they can get very noisy very quickly as you are likely to fail
the allocation for every packet in a given polling session, and
sessions to follow.

> +			goto rx_processing_done;
> +		}
> +
>  		skb_reserve(skb, data_start);
>  		skb_put(skb, pkt_len - sub_len);
>  		skb_mark_for_recycle(skb);