Message-ID: <a5f90434-f3e5-f25e-76e2-b03e79cc16fe@alu.unizg.hr>
Date:   Sat, 11 Mar 2023 04:29:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] goku_udc: Add check for NULL in goku_irq
Content-Language: en-US, hr
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?0JDQvdCw0YHRgtCw0YHQuNGPINCR0LXQu9C+0LLQsA==?= 
        <abelova@astralinux.ru>
Cc:     Jakob Koschel <jakobkoschel@gmail.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, lvc-project@linuxtesting.org
References: <20230203101828.14799-1-abelova@astralinux.ru>
 <Y9zly1vrj9z4c1qT@kroah.com>
 <39993564-7310-a2e0-8139-14ccb9a03ba9@astralinux.ru>
 <Y+zivah57216KcuB@kroah.com>
From:   Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
In-Reply-To: <Y+zivah57216KcuB@kroah.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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, &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.

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

-- 
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