netdev - Re: [PATCH net-next] can: sja1000: fix use after free in ems_pcmcia_add

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

Message-ID: <20211124065605.GE6514@kadam>
Date:   Wed, 24 Nov 2021 09:56:06 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Leon Romanovsky <leon@...nel.org>, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net-next] can: sja1000: fix use after free in
 ems_pcmcia_add_card()

On Tue, Nov 23, 2021 at 09:42:12PM +0100, Oliver Hartkopp wrote:
> Hello Dan,
> 
> On 22.11.21 08:56, Dan Carpenter wrote:
> > In the original code if ems_pcmcia_check_chan() returned false then
> > it called free_sja1000dev(dev) but did not set the error code or jump
> > to the clean up code.  This frees "dev" and leads to a use after free.
> > 
> > I flipped the ems_pcmcia_check_chan() check around to make the error
> > handling more consistent and readable.  That lets us pull the rest of
> > the code in one tab.
> > 
> > Fixes: fd734c6f25ae ("can/sja1000: add driver for EMS PCMCIA card")
> > Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> 
> I do not think, that you are fixing something here.
> 
> The loop
> 
> for (i = 0; i < EMS_PCMCIA_MAX_CHAN; i++) { ...
> 
> checks with
> 
> if (ems_pcmcia_check_chan(priv))
> 
> whether this channel is 'available' or not.
> 
> As this hardware could come with only ONE channel it is just wrong to tag a
> missing channel as error and finally kill the entire setup process
> (including the potentially working channel we already initialized).
> 
> So thanks for the patch but NACK ;-)

There is definitely a use after free bug with the "dev" pointer, but
you're right that it would only affect things if it were the last
channel.  The easy solution would be to do something like:

-	err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED,
+	err = request_irq(pdev->irq, &ems_pcmcia_interrupt, IRQF_SHARED,

I'll send a patch for that.

If we were super paranoid, we could add a check for if of the channels
were available.

regards,
dan carpenter