Date:   Wed, 15 Feb 2023 14:48:45 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     =?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
Subject: Re: [PATCH] goku_udc: Add check for NULL in goku_irq
Message-ID: <Y+zivah57216KcuB@kroah.com>
References: <20230203101828.14799-1-abelova@astralinux.ru>
 <Y9zly1vrj9z4c1qT@kroah.com>
 <39993564-7310-a2e0-8139-14ccb9a03ba9@astralinux.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <39993564-7310-a2e0-8139-14ccb9a03ba9@astralinux.ru>
Precedence: bulk

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