Re: [PATCH] goku_udc: Add check for NULL in goku_irq

Anastasia Belova <abelova@xxxxxxxxxxxxx> · Mon, 13 Mar 2023 15:19:50 +0300

11.03.2023 06:29, 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@xxxxxxxxxxxxx>
---
   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,

Hi Misrad,

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, &regs->power_detect);
	writel(0, &regs->int_enable);
	readl(&regs->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.

Are you sure that spin_lock_irqsave makes the code safe? This function 
disables interrupts on

local processor only (Linux Device Drivers, Third Edition). So it 
doesn't seem to be

absolutely safe on multiprocessor systems.

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(&regs->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(&regs->int_enable);
	spin_unlock(&dev->lock);

This unlocks dev->lock before setting dev->int_enable to zero, or calling writel(0, &regs->int_enable);
which could be problematic. Unless it called stop_activity(dev) four lines earlier. Which does
bot of:

	writel(0, &regs->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

Thanks,

Anastasia







