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);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ