Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Jul 2023 08:25:58 +0200
From:   Herve Codina <herve.codina@bootlin.com>
To:     Yu Hao <yhao016@ucr.edu>
Cc:     chunfeng.yun@mediatek.com, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: mtu3: Fix possible use-before-initialization bug
Message-ID: <20230710082558.2f82d607@bootlin.com>
In-Reply-To: <CA+UBctBqtSvyBWf9ZwKbecTrh9_6sCDm_TyU-ncb+6h5y19K5g@mail.gmail.com>
References: <CA+UBctDxfb6+70+hzuXJ-gwb65E0uoNzXYEhpJT92sXr2CE7OA@mail.gmail.com>
        <20230705080625.02b2bac5@bootlin.com>
        <CA+UBctBqtSvyBWf9ZwKbecTrh9_6sCDm_TyU-ncb+6h5y19K5g@mail.gmail.com>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Yu,

On Sun, 9 Jul 2023 17:48:15 -0700
Yu Hao <yhao016@ucr.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;

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

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com