[PATCH net-next] can: sja1000: fix use after free in ems_pcmcia_add_card()

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>
---
 drivers/net/can/sja1000/ems_pcmcia.c | 44 +++++++++++++++-------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/sja1000/ems_pcmcia.c b/drivers/net/can/sja1000/ems_pcmcia.c
index e21b169c14c0..271fe9444827 100644
--- a/drivers/net/can/sja1000/ems_pcmcia.c
+++ b/drivers/net/can/sja1000/ems_pcmcia.c

@@ -210,28 +210,30 @@ static int ems_pcmcia_add_card(struct pcmcia_device *pdev, unsigned long base)
 			(i * EMS_PCMCIA_CAN_CTRL_SIZE);
 
 		/* Check if channel is present */
-		if (ems_pcmcia_check_chan(priv)) {
-			priv->read_reg  = ems_pcmcia_read_reg;
-			priv->write_reg = ems_pcmcia_write_reg;
-			priv->can.clock.freq = EMS_PCMCIA_CAN_CLOCK;
-			priv->ocr = EMS_PCMCIA_OCR;
-			priv->cdr = EMS_PCMCIA_CDR;
-			priv->flags |= SJA1000_CUSTOM_IRQ_HANDLER;
-
-			/* Register SJA1000 device */
-			err = register_sja1000dev(dev);
-			if (err) {
-				free_sja1000dev(dev);
-				goto failure_cleanup;
-			}
-
-			card->channels++;
-
-			printk(KERN_INFO "%s: registered %s on channel "
-			       "#%d at 0x%p, irq %d\n", DRV_NAME, dev->name,
-			       i, priv->reg_base, dev->irq);
-		} else
+		if (!ems_pcmcia_check_chan(priv)) {
+			err = -EINVAL;
 			free_sja1000dev(dev);
+			goto failure_cleanup;
+		}
+		priv->read_reg  = ems_pcmcia_read_reg;
+		priv->write_reg = ems_pcmcia_write_reg;
+		priv->can.clock.freq = EMS_PCMCIA_CAN_CLOCK;
+		priv->ocr = EMS_PCMCIA_OCR;
+		priv->cdr = EMS_PCMCIA_CDR;
+		priv->flags |= SJA1000_CUSTOM_IRQ_HANDLER;
+
+		/* Register SJA1000 device */
+		err = register_sja1000dev(dev);
+		if (err) {
+			free_sja1000dev(dev);
+			goto failure_cleanup;
+		}
+
+		card->channels++;
+
+		printk(KERN_INFO "%s: registered %s on channel "
+		       "#%d at 0x%p, irq %d\n", DRV_NAME, dev->name,
+		       i, priv->reg_base, dev->irq);
 	}
 
 	err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED,

-- 
2.20.1

Sorry about I meant [PATCH net]...  I was in the process of editing the
subject and got distracted and hit send by mistake.  :/

regards,
dan carpenter

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

Best regards,
Oliver

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