On Tue, Nov 23, 2021 at 09:42:12PM +0100, Oliver Hartkopp wrote:

Hello Dan,

On 22.11.21 08:56, Dan Carpenter wrote:

quoted

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 <redacted>

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