linux-kernel - Re: [PATCH] can: rcar_canfd: Make sure we free CAN network device

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>] [day] [month] [year] [list]

Date:   Thu, 6 Jan 2022 17:46:25 +0000
From:   "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To:     Kieran Bingham <kieran.bingham@...asonboard.com>
Cc:     Biju Das <biju.das.jz@...renesas.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Pavel Machek <pavel@...x.de>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        linux-can@...r.kernel.org, netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] can: rcar_canfd: Make sure we free CAN network device

Hi Kieran,

Thank you for the  review.

On Thu, Jan 6, 2022 at 3:46 PM Kieran Bingham
<kieran.bingham@...asonboard.com> wrote:
>
> Quoting Lad Prabhakar (2022-01-06 11:48:00)
> > Make sure we free CAN network device in the error path. There are several
> > jumps to fail label after allocating the CAN network device successfully.
> > This patch places the free_candev() under fail label so that in failure
> > path a jump to fail label frees the CAN network device.
> >
> > Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family")
> > Reported-by: Pavel Machek <pavel@...x.de>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index ff9d0f5ae0dd..388521e70837 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -1640,8 +1640,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> >         ndev = alloc_candev(sizeof(*priv), RCANFD_FIFO_DEPTH);
> >         if (!ndev) {
> >                 dev_err(&pdev->dev, "alloc_candev() failed\n");
> > -               err = -ENOMEM;
> > -               goto fail;
> > +               return -ENOMEM;
>
> Aha good - so we don't try to call free_candev() on a null pointer.
> (which doesn't look null-safe, in free_netdev).
>
Yep.

> >         }
> >         priv = netdev_priv(ndev);
> >
> > @@ -1735,8 +1734,8 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
> >
> >  fail_candev:
>
> Is this label still appropriately named now that the free_candev is
> moved out of it? I wonder if it should be fail_netif:
>
I was tempted for this change, but wanted to keep the changes minimal.
Maybe I'll do it anyway to improve the readability.

> So aside from potential naming, the !ndev case is safely handled, so it
> looks fine to me.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
>
> >         netif_napi_del(&priv->napi);
> > -       free_candev(ndev);
> >  fail:
> > +       free_candev(ndev);
>
>
>
> >         return err;
> >  }
> >
> > --
> > 2.17.1
> >

Cheers,
Prabhakar