| Message ID | 20210819154935.19826-1-lutovinova@ispras.ru |
|---|---|
| State | New |
| Headers | show |
| Series | media: allegro: request irq after initializing mbox_status | expand |
Hi Nadezda, On Thu, 19 Aug 2021 18:49:35 +0300, Nadezda Lutovinova wrote: > If IRQ occurs between calling devm_request_threaded_irq() and > allegro_firmware_request_nowait(), then null pointer dereference > occurs since dev->mbox_status wasn't initialized yet but used > in allegro_mbox_notify(). Thanks for the patch. As explained in [0], this is not an issue. > > The patch puts registration of the interrupt handler after > initializing of neccesery data. The interrupt handler must be registered during the execution of allegro_fw_callback(), because the driver exchanges messages with the firmware during the bring up. With this patch this is not guaranteed anymore. Michael [0] https://lore.kernel.org/linux-media/20210511072834.GC17882@pengutronix.de/ > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru> > --- > .../media/platform/allegro-dvt/allegro-core.c | 24 +++++++++---------- > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/platform/allegro-dvt/allegro-core.c b/drivers/media/platform/allegro-dvt/allegro-core.c > index 887b492e4ad1..9c1997ff74e8 100644 > --- a/drivers/media/platform/allegro-dvt/allegro-core.c > +++ b/drivers/media/platform/allegro-dvt/allegro-core.c > @@ -3707,18 +3707,6 @@ static int allegro_probe(struct platform_device *pdev) > return PTR_ERR(dev->sram); > } > > - irq = platform_get_irq(pdev, 0); > - if (irq < 0) > - return irq; > - ret = devm_request_threaded_irq(&pdev->dev, irq, > - allegro_hardirq, > - allegro_irq_thread, > - IRQF_SHARED, dev_name(&pdev->dev), dev); > - if (ret < 0) { > - dev_err(&pdev->dev, "failed to request irq: %d\n", ret); > - return ret; > - } > - > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > if (ret) > return ret; > @@ -3732,6 +3720,18 @@ static int allegro_probe(struct platform_device *pdev) > return ret; > } > > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + ret = devm_request_threaded_irq(&pdev->dev, irq, > + allegro_hardirq, > + allegro_irq_thread, > + IRQF_SHARED, dev_name(&pdev->dev), dev); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to request irq: %d\n", ret); > + return ret; > + } > + > return 0; > } > > -- > 2.17.1 > >
diff --git a/drivers/media/platform/allegro-dvt/allegro-core.c b/drivers/media/platform/allegro-dvt/allegro-core.c index 887b492e4ad1..9c1997ff74e8 100644 --- a/drivers/media/platform/allegro-dvt/allegro-core.c +++ b/drivers/media/platform/allegro-dvt/allegro-core.c @@ -3707,18 +3707,6 @@ static int allegro_probe(struct platform_device *pdev) return PTR_ERR(dev->sram); } - irq = platform_get_irq(pdev, 0); - if (irq < 0) - return irq; - ret = devm_request_threaded_irq(&pdev->dev, irq, - allegro_hardirq, - allegro_irq_thread, - IRQF_SHARED, dev_name(&pdev->dev), dev); - if (ret < 0) { - dev_err(&pdev->dev, "failed to request irq: %d\n", ret); - return ret; - } - ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); if (ret) return ret; @@ -3732,6 +3720,18 @@ static int allegro_probe(struct platform_device *pdev) return ret; } + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + ret = devm_request_threaded_irq(&pdev->dev, irq, + allegro_hardirq, + allegro_irq_thread, + IRQF_SHARED, dev_name(&pdev->dev), dev); + if (ret < 0) { + dev_err(&pdev->dev, "failed to request irq: %d\n", ret); + return ret; + } + return 0; }
If IRQ occurs between calling devm_request_threaded_irq() and allegro_firmware_request_nowait(), then null pointer dereference occurs since dev->mbox_status wasn't initialized yet but used in allegro_mbox_notify(). The patch puts registration of the interrupt handler after initializing of neccesery data. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Nadezda Lutovinova <lutovinova@ispras.ru> --- .../media/platform/allegro-dvt/allegro-core.c | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-)