Messages in this thread | | | Date | Sat, 11 Mar 2023 04:29:29 +0100 | Subject | Re: [PATCH] goku_udc: Add check for NULL in goku_irq | From | Mirsad Goran Todorovac <> |
| |
On 15. 02. 2023. 14:48, Greg Kroah-Hartman wrote: > On Wed, Feb 15, 2023 at 04:39:56PM +0300, Анастасия Белова wrote: >> >> 03.02.2023 13:45, Greg Kroah-Hartman пишет: >>> On Fri, Feb 03, 2023 at 01:18:28PM +0300, Anastasia Belova wrote: >>>> Before dereferencing dev->driver check it for NULL. >>>> >>>> If an interrupt handler is called after assigning >>>> NULL to dev->driver, but before resetting dev->int_enable, >>>> NULL-pointer will be dereferenced. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >>>> Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >>>> --- >>>> drivers/usb/gadget/udc/goku_udc.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/udc/goku_udc.c b/drivers/usb/gadget/udc/goku_udc.c >>>> index bdc56b24b5c9..896bba8b47f1 100644 >>>> --- a/drivers/usb/gadget/udc/goku_udc.c >>>> +++ b/drivers/usb/gadget/udc/goku_udc.c >>>> @@ -1616,8 +1616,9 @@ static irqreturn_t goku_irq(int irq, void *_dev) >>>> pm_next: >>>> if (stat & INT_USBRESET) { /* hub reset done */ >>>> ACK(INT_USBRESET); >>>> - INFO(dev, "USB reset done, gadget %s\n", >>>> - dev->driver->driver.name); >>>> + if (dev->driver) >>>> + INFO(dev, "USB reset done, gadget %s\n", >>>> + dev->driver->driver.name); >>> How can this ever happen? Can you trigger this somehow? If not, I >>> don't think this is going to be possible (also what's up with printk >>> from an irq handler???) >> >> Unfortunately, I can't find the way to trigger this at the moment. > > Then the change should not be made. > >> What about printk, should trace_printk be used instead? > > Why? > >>> Odds are, no one actually has this hardware anymore, right? >> >> Despite of this, such vulnerability should be fixed because >> there is a possibility to exploit it. > > How can this be "exploited" if it can not ever be triggered? > > Also, this would cause a NULL dereference in an irq handler, how can you > "exploit" that? > > Please only submit patches that actually do something. It is getting > very hard to want to even review patches from this "project" based on > the recent submissions. > > thanks, > > greg k-h
Hi Greg, Anastasia,
Without any pros or cons, or taking sides, there appears to be a similar check when using dev->driver->driver.name in
https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/udc/goku_udc.c#L1158
seq_printf(m, "%s - %s\n" "%s version: %s %s\n" "Gadget driver: %s\n" "Host %s, %s\n" "\n", pci_name(dev->pdev), driver_desc, driver_name, DRIVER_VERSION, dmastr(), dev->driver ? dev->driver->driver.name : "(none)", is_usb_connected ? ((tmp & PW_PULLUP) ? "full speed" : "powered") : "disconnected", udc_ep_state(dev->ep0state));
On the other hand, where could dev->drivre be reset without resetting dev->int_enable?
dev->driver = NULL appears here:
static int goku_udc_stop(struct usb_gadget *g) { struct goku_udc *dev = to_goku_udc(g); unsigned long flags;
spin_lock_irqsave(&dev->lock, flags); dev->driver = NULL; stop_activity(dev); spin_unlock_irqrestore(&dev->lock, flags);
return 0; }
it is followed by stop_activity() calling udc_reset():
static void udc_reset(struct goku_udc *dev) { struct goku_udc_regs __iomem *regs = dev->regs;
writel(0, ®s->power_detect); writel(0, ®s->int_enable); readl(®s->int_enable); dev->int_enable = 0; . . .
... but this happens in between spin_lock_irqsave() and spin_unlock_irqsave(), which appears like a correct way to do it.
But second appearance is here:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/udc/goku_udc.c#L1559
spin_lock(&dev->lock);
rescan: stat = readl(®s->int_status) & dev->int_enable; if (!stat) goto done; dev->irqs++;
/* device-wide irqs */ if (unlikely(stat & INT_DEVWIDE)) { if (stat & INT_SYSERROR) { ERROR(dev, "system error\n"); stop_activity(dev); stat = 0; handled = 1; // FIXME have a neater way to prevent re-enumeration dev->driver = NULL; goto done; }
goto done leads to:
done: (void)readl(®s->int_enable); spin_unlock(&dev->lock);
This unlocks dev->lock before setting dev->int_enable to zero, or calling writel(0, ®s->int_enable); which could be problematic. Unless it called stop_activity(dev) four lines earlier. Which does bot of:
writel(0, ®s->int_enable); dev->int_enable = 0;
So, FWIW, we seem to be safe. Yet, there might be no harm in printing "(null)" rather than having an NULL pointer dereference, it seems.
Yet, there is another unprotected dereference of dev->driver:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/udc/goku_udc.c#L1513
spin_unlock (&dev->lock); tmp = dev->driver->setup(&dev->gadget, &ctrl); spin_lock (&dev->lock);
All others (in goku_udc.c, at least) have triple safeguards like:
if (dev->gadget.speed != USB_SPEED_UNKNOWN && dev->driver && dev->driver->suspend) { spin_unlock(&dev->lock); dev->driver->suspend(&dev->gadget); spin_lock(&dev->lock); }
So the above should maybe put to:
if (dev->driver && dev->driver->setup) { spin_unlock (&dev->lock); tmp = dev->driver->setup(&dev->gadget, &ctrl); spin_lock (&dev->lock); }
instead to be completely certain.
Forgive me for this uninspired rant. Thank you if you've read this far. I hope this helps.
My $0.02.
Regards, Mirsad
-- Mirsad Goran Todorovac Sistem inženjer Grafički fakultet | Akademija likovnih umjetnosti Sveučilište u Zagrebu System engineer Faculty of Graphic Arts | Academy of Fine Arts University of Zagreb, Republic of Croatia The European Union
| |