diff mbox series

[V2,net] net: fec: Coverity issue: Dereference null return value

Message ID 20221219022755.1047573-1-wei.fang@nxp.com
State New
Headers
Series [V2,net] net: fec: Coverity issue: Dereference null return value |

Commit Message

Wei Fang Dec. 19, 2022, 2:27 a.m. UTC 
  From: Wei Fang <wei.fang@nxp.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@nxp.com>
Reviewed-by: Shenwei Wang <Shenwei.wang@nxp.com>
---
V2 changes:
1. Remove rx_packets and rx_bytes counters.
2. Use netdev_err_once instead of netdev_err.
---
 drivers/net/ethernet/freescale/fec_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Alexander Duyck Dec. 19, 2022, 3:42 p.m. UTC | #1 
On Sun, Dec 18, 2022 at 6:31 PM <wei.fang@nxp.com> wrote:
>
> From: Wei Fang <wei.fang@nxp.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@nxp.com>
> Reviewed-by: Shenwei Wang <Shenwei.wang@nxp.com>
> ---
> V2 changes:
> 1. Remove rx_packets and rx_bytes counters.
> 2. Use netdev_err_once instead of netdev_err.
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 5528b0af82ae..644f3c963730 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1674,6 +1674,14 @@ 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_dropped++;
> +
> +                       netdev_err_once(ndev, "build_skb failed!\n");
> +                       goto rx_processing_done;
> +               }
> +
>                 skb_reserve(skb, data_start);
>                 skb_put(skb, pkt_len - sub_len);
>                 skb_mark_for_recycle(skb);


Looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
Jakub Kicinski Dec. 20, 2022, 7:36 p.m. UTC | #2 
On Mon, 19 Dec 2022 10:27:55 +0800 wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.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.

Applied but I had to change the subject because the subject should
describe the change. Mentioning the tool which found the problem
belongs in the body of the message.
Wei Fang Dec. 21, 2022, 1:39 a.m. UTC | #3 
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2022年12月21日 3:36
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; pabeni@redhat.com;
> Clark Wang <xiaoning.wang@nxp.com>; Shenwei Wang
> <shenwei.wang@nxp.com>; alexander.duyck@gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2 net] net: fec: Coverity issue: Dereference null return
> value
> 
> On Mon, 19 Dec 2022 10:27:55 +0800 wei.fang@nxp.com wrote:
> > From: Wei Fang <wei.fang@nxp.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.
> 
> Applied but I had to change the subject because the subject should describe
> the change. Mentioning the tool which found the problem belongs in the body
> of the message.

Thanks for taking the time to make these changes, I'll keep these things in mind
next time.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 5528b0af82ae..644f3c963730 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1674,6 +1674,14 @@  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_dropped++;
+
+			netdev_err_once(ndev, "build_skb failed!\n");
+			goto rx_processing_done;
+		}
+
 		skb_reserve(skb, data_start);
 		skb_put(skb, pkt_len - sub_len);
 		skb_mark_for_recycle(skb);