| Message ID | 20211122075614.GB6581@kili (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] can: sja1000: fix use after free in ems_pcmcia_add_card() | expand |
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 <dan.carpenter@oracle.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 ;-) Best regards, Oliver > --- > 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, >
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@oracle.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
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,
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@oracle.com> --- drivers/net/can/sja1000/ems_pcmcia.c | 44 +++++++++++++++------------- 1 file changed, 23 insertions(+), 21 deletions(-)