linux-kernel - Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29a71cbb61148d2085cc08da681526c4e20d31b9.camel@mediatek.com>
Date:   Tue, 11 Jul 2023 08:48:35 +0000
From:   Chunfeng Yun (云春峰) 
        <Chunfeng.Yun@...iatek.com>
To:     "yhao016@....edu" <yhao016@....edu>,
        "herve.codina@...tlin.com" <herve.codina@...tlin.com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug

On Mon, 2023-07-10 at 08:25 +0200, Herve Codina wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Yu,
> 
> On Sun, 9 Jul 2023 17:48:15 -0700
> Yu Hao <yhao016@....edu> wrote:
> 
> > Hi Hervé,
> > 
> > Thanks for the comments. How about this patch?
> > ---
> >  drivers/usb/mtu3/mtu3_gadget_ep0.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > index e4fd1bb14a55..af2884943c2a 100644
> > --- a/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > +++ b/drivers/usb/mtu3/mtu3_gadget_ep0.c
> > @@ -600,7 +600,7 @@ static void ep0_tx_state(struct mtu3 *mtu)
> >         mtu3_readl(mtu->mac_base, U3D_EP0CSR));
> >  }
> > 
> > -static void ep0_read_setup(struct mtu3 *mtu, struct
> usb_ctrlrequest *setup)
> > +static int ep0_read_setup(struct mtu3 *mtu, struct usb_ctrlrequest
> *setup)
> >  {
> >     struct mtu3_request *mreq;
> >     u32 count;
> > @@ -608,6 +608,8 @@ static void ep0_read_setup(struct mtu3 *mtu,
> > struct usb_ctrlrequest *setup)
> > 
> >     csr = mtu3_readl(mtu->mac_base, U3D_EP0CSR) & EP0_W1C_BITS;
> >     count = mtu3_readl(mtu->mac_base, U3D_RXCOUNT0);
> > +   if (count == 0)
> > +       return -EINVAL;
> 
> 'count' should be tested against sizeof(*setup). Indeed, we need to
> have a
> setup data packet in the fifo.
> 
> What do you think about:
> if (count < sizef(*setup))
> return -EINVAL;
before call this function, already check the data length in fifo, it
should be 8 bytes.
see mtu3_ep0_isr(), about line 761. 

I think no need this patch

Thanks a lot

> 
> > 
> >     ep0_read_fifo(mtu->ep0, (u8 *)setup, count);
> > 
> > @@ -642,7 +644,8 @@ __acquires(mtu->lock)
> >     struct mtu3_request *mreq;
> >     int handled = 0;
> > 
> > -   ep0_read_setup(mtu, &setup);
> > +   if (ep0_read_setup(mtu, &setup))
> > +       return -EINVAL;
> 
> Forward the error code to the caller ?
> 
> ret = ep0_read_setup(mtu, &setup)
> if (ret < 0)
> return ret;
> 
> 
> >     trace_mtu3_handle_setup(&setup);
> > 
> >     if ((setup.bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD)
> > @@ -764,7 +767,9 @@ irqreturn_t mtu3_ep0_isr(struct mtu3 *mtu)
> >             break;
> >         }
> > 
> > -       ep0_handle_setup(mtu);
> > +       if (ep0_handle_setup(mtu))
> > +           break;
> > +
> 
> Ok
> 
> >         ret = IRQ_HANDLED;
> >         break;
> >     default:
> 
> Be careful, your patch is wrongly indented.
> tabs replaced by 4 spaces. You need to keep tabs.
> 
> Regards,
> Hervé Codina
>