Re: [bug report] media: allegro: possible NULL pointer dereference.

[Michael Tretter]

Hi Lucas,

On Tue, 11 May 2021 10:49:16 +0200, Lucas Stach wrote:
> Am Dienstag, dem 11.05.2021 um 09:28 +0200 schrieb Michael Tretter:
> > On Sat, 08 May 2021 19:04:55 +0300, Yuri Savinykh wrote:
> > > At the moment of enabling irq handling:
> > > 
> > > 3166     ret = devm_request_threaded_irq(&pdev->dev, irq,
> > > 3167                     allegro_hardirq,
> > > 3168                     allegro_irq_thread,
> > > 3169                     IRQF_SHARED, dev_name(&pdev->dev), dev);
> > > 
> > > there is still uninitialized field mbox_status of struct allegro_dev *dev.
> > > If an interrupt occurs in the interval between the installation of the
> > > interrupt handler and the initialization of this field, NULL pointer
> > > dereference happens.
> > > 
> > > This field is dereferenced in the handler function without any check:
> > > 
> > > 1801 static irqreturn_t allegro_irq_thread(int irq, void *data)
> > > 1802 {
> > > 1803     struct allegro_dev *dev = data;
> > > 1804
> > > 1805     allegro_mbox_notify(dev->mbox_status);
> > > 
> > > 
> > > and then:
> > > 
> > > 752 static void allegro_mbox_notify(struct allegro_mbox *mbox)
> > > 753 {
> > > 754     struct allegro_dev *dev = mbox->dev;
> > > 
> > > The initialization of the mbox_status field happens asynchronously in
> > > allegro_fw_callback() via allegro_mcu_hw_init(). 
> > > 
> > > Is it guaranteed that an interrupt does not occur in this interval?
> > > If it is not, is it better to move interrupt handler installation
> > > after initialization of this field has been completed?
> > 
> > Thanks for the report. The interrupt is triggered by the firmware, which is
> > only loaded in allegro_fw_callback(), and is enabled only after the
> > initialization of mbox_status in allegro_mcu_hw_init():
> > 
> > 3507	allegro_mcu_enable_interrupts(dev)
> > 
> > The interrupt handler is installed in probe(), because that's where all the
> > platform information is retrieved. Unfortunately, at that time, the driver is
> > not able to setup the mailboxes, because the mailbox configuration depends on
> > the firmware and is only known in allegro_fw_callback().
> > 
> > It might be interesting to tie the interrupt more closely to the mailboxes,
> > because it is actually only used to notify the driver about mails in the
> > mailbox, but that's something I have not yet considered worth the effort.
> > 
> 
> The interrupt is installed with IRQF_SHARED, so your IRQ handler must
> be prepared to be called even if your device did not trigger an IRQ and
> even before your initialization is done, as another device on the same
> IRQ line might trigger the IRQ. In that case you must at least be able
> to return IRQ_NONE from your handler without crashing the kernel.

The allegro_hardirq() handler already checks the irq status register
(AL5_ITC_CPU_IRQ_STA) for the device and returns IRQ_NONE before even
dispatching the interrupt to the irq thread. In this case, the mailbox is not
read at all.

Michael