| Message ID | 20211124065618.GA3970@kili (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] can: sja1000: fix use after free in ems_pcmcia_add_card() | expand |
Hello Dan, On 24.11.21 07:56, Dan Carpenter wrote: > If the last channel is not available then "dev" is freed. Fortunately, > we can just use "pdev->irq" instead. But in the case that we do not find any channel the irq for the card is still requested (via pdev->irq). > > Fixes: fd734c6f25ae ("can/sja1000: add driver for EMS PCMCIA card") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: In the first version, I just failed the probe. Sorry about that. > > drivers/net/can/sja1000/ems_pcmcia.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/can/sja1000/ems_pcmcia.c b/drivers/net/can/sja1000/ems_pcmcia.c > index e21b169c14c0..391a8253ed6f 100644 > --- a/drivers/net/can/sja1000/ems_pcmcia.c > +++ b/drivers/net/can/sja1000/ems_pcmcia.c > @@ -234,7 +234,7 @@ static int ems_pcmcia_add_card(struct pcmcia_device *pdev, unsigned long base) > free_sja1000dev(dev); > } > > - err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED, When adding this check, we should be fine: + if (card->channels) > + err = request_irq(pdev->irq, &ems_pcmcia_interrupt, IRQF_SHARED, > DRV_NAME, card); > if (!err) > return 0; > Thanks for checking this code after so many years! I saved that 17 year old EMS PCMCIA Card from my former CAN hardware box two weeks ago and made a 5.16-rc2 run on a 2006 Samsung X20 with Pentium M 1.7GHz yesterday. My only machine here at home with a PCMCIA slot :-D https://www.amazon.de/Samsung-Centrino-1-73GHz-Graphic-Accelerator/dp/B000AXSIRE And it still works with the CAN card! Best regards, Oliver
On Wed, Nov 24, 2021 at 08:37:27AM +0100, Oliver Hartkopp wrote: > Hello Dan, > > On 24.11.21 07:56, Dan Carpenter wrote: > > If the last channel is not available then "dev" is freed. Fortunately, > > we can just use "pdev->irq" instead. > > But in the case that we do not find any channel the irq for the card is > still requested (via pdev->irq). > > > > > Fixes: fd734c6f25ae ("can/sja1000: add driver for EMS PCMCIA card") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > v2: In the first version, I just failed the probe. Sorry about that. > > > > drivers/net/can/sja1000/ems_pcmcia.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/sja1000/ems_pcmcia.c b/drivers/net/can/sja1000/ems_pcmcia.c > > index e21b169c14c0..391a8253ed6f 100644 > > --- a/drivers/net/can/sja1000/ems_pcmcia.c > > +++ b/drivers/net/can/sja1000/ems_pcmcia.c > > @@ -234,7 +234,7 @@ static int ems_pcmcia_add_card(struct pcmcia_device *pdev, unsigned long base) > > free_sja1000dev(dev); > > } > > - err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED, > > When adding this check, we should be fine: > > + if (card->channels) Sure, I will send a v3 with that. regards, dan carpenter
On 24.11.21 09:57, Dan Carpenter wrote: > On Wed, Nov 24, 2021 at 08:37:27AM +0100, Oliver Hartkopp wrote: >> Hello Dan, >> >> On 24.11.21 07:56, Dan Carpenter wrote: >>> If the last channel is not available then "dev" is freed. Fortunately, >>> we can just use "pdev->irq" instead. >> >> But in the case that we do not find any channel the irq for the card is >> still requested (via pdev->irq). >> >>> >>> Fixes: fd734c6f25ae ("can/sja1000: add driver for EMS PCMCIA card") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> --- >>> v2: In the first version, I just failed the probe. Sorry about that. >>> >>> drivers/net/can/sja1000/ems_pcmcia.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/can/sja1000/ems_pcmcia.c b/drivers/net/can/sja1000/ems_pcmcia.c >>> index e21b169c14c0..391a8253ed6f 100644 >>> --- a/drivers/net/can/sja1000/ems_pcmcia.c >>> +++ b/drivers/net/can/sja1000/ems_pcmcia.c >>> @@ -234,7 +234,7 @@ static int ems_pcmcia_add_card(struct pcmcia_device *pdev, unsigned long base) >>> free_sja1000dev(dev); >>> } >>> - err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED, >> >> When adding this check, we should be fine: >> >> + if (card->channels) > > Sure, I will send a v3 with that. With these discussed changes you might directly add my Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> Thanks, Oliver
diff --git a/drivers/net/can/sja1000/ems_pcmcia.c b/drivers/net/can/sja1000/ems_pcmcia.c index e21b169c14c0..391a8253ed6f 100644 --- a/drivers/net/can/sja1000/ems_pcmcia.c +++ b/drivers/net/can/sja1000/ems_pcmcia.c @@ -234,7 +234,7 @@ static int ems_pcmcia_add_card(struct pcmcia_device *pdev, unsigned long base) free_sja1000dev(dev); } - err = request_irq(dev->irq, &ems_pcmcia_interrupt, IRQF_SHARED, + err = request_irq(pdev->irq, &ems_pcmcia_interrupt, IRQF_SHARED, DRV_NAME, card); if (!err) return 0;
If the last channel is not available then "dev" is freed. Fortunately, we can just use "pdev->irq" instead. Fixes: fd734c6f25ae ("can/sja1000: add driver for EMS PCMCIA card") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: In the first version, I just failed the probe. Sorry about that. drivers/net/can/sja1000/ems_pcmcia.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)