| 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
| ||
|
Date: Thu, 26 May 2016 16:24:17 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Felipe Balbi <balbi@...nel.org>
Cc: Greg KH <gregkh@...uxfoundation.org>,
Mark Brown <broonie@...nel.org>,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check
endpoint type
Hi,
On 26 May 2016 at 15:48, Felipe Balbi <balbi@...nel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@...aro.org> writes:
>> Hi Felipe,
>>
>> On 26 May 2016 at 14:22, Felipe Balbi <balbi@...nel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@...aro.org> writes:
>>>> When handling the endpoint interrupt handler, it maybe disable the endpoint
>>>> from another core user to set the USB endpoint descriptor pointor to be NULL
>>>> while issuing usb_gadget_giveback_request() function to release lock. So it
>>>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with
>>>> one NULL USB endpoint descriptor.
>>>
>>> too complex, Baolin :-) Can you see if this helps:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a
>>>
>>> The only situation when that can happen is while we drop our lock on
>>> dwc3_gadget_giveback().
>>
>> OK, But line 1974 and line 2025 as below may be at risk? So I think
>> can we have a common place to solve the problem in case
>> usb_endpoint_xfer_xxx() APIs are issued at this risk? What do you
>> think? Thanks.
>>
>> 1956 static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>> 1957 const struct dwc3_event_depevt *event, int status)
>> 1958 {
>> 1959 struct dwc3_request *req;
>> 1960 struct dwc3_trb *trb;
>> 1961 unsigned int slot;
>> 1962 unsigned int i;
>> 1963 int ret;
>> 1964
>> 1965 do {
>> 1966 req = next_request(&dep->req_queued);
>> 1967 if (WARN_ON_ONCE(!req))
>> 1968 return 1;
>> 1969
>> 1970 i = 0;
>> 1971 do {
>> 1972 slot = req->start_slot + i;
>> 1973 if ((slot == DWC3_TRB_NUM - 1) &&
>> 1974 usb_endpoint_xfer_isoc(dep->endpoint.desc))
>
> this is executed still with locks held.
Yeah, that's right.
>
>> 1975 slot++;
>> 1976 slot %= DWC3_TRB_NUM;
>> 1977 trb = &dep->trb_pool[slot];
>> 1978
>> 1979 ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb,
>> 1980 event, status);
>> 1981 if (ret)
>> 1982 break;
>> 1983 } while (++i < req->request.num_mapped_sgs);
>> 1984
>> 1985 dwc3_gadget_giveback(dep, req, status);
>
> the problem can only show up after this call
>
>> 1986
>> 1987 if (ret)
>> 1988 break;
>> 1989 } while (1);
>> .......
>>
>> 2011 static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
>> 2012 struct dwc3_ep *dep, const struct dwc3_event_depevt *event)
>> 2013 {
>> 2014 unsigned status = 0;
>> 2015 int clean_busy;
>> 2016 u32 is_xfer_complete;
>> 2017
>> 2018 is_xfer_complete = (event->endpoint_event ==
>> DWC3_DEPEVT_XFERCOMPLETE);
>> 2019
>> 2020 if (event->status & DEPEVT_STATUS_BUSERR)
>> 2021 status = -ECONNRESET;
>> 2022
>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>> 2024 if (clean_busy && (is_xfer_complete ||
>> 2025
>
> note the patch I linked you. This is where I added a bail out if the
> descriptor is NULL. Here's the patch for reference:
>
> commit 4d100e870616ccd30cf29abf21d437ec746e1f54
> Author: Felipe Balbi <felipe.balbi@...ux.intel.com>
> Date: Wed May 18 12:37:21 2016 +0300
>
> usb: dwc3: gadget: fix for possible endpoint disable race
>
> when we call dwc3_gadget_giveback(), we end up
> releasing our controller's lock. Another thread
> could get scheduled and disable the endpoint,
> subsequently setting dep->endpoint.desc to NULL.
>
> In that case, we would end up dereferencing a NULL
> pointer which would result in a Kernel Oops. Let's
> avoid the problem by simply returning early if we
> have a NULL descriptor.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@...ux.intel.com>
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f31a59cd5162..3d0573c74b13 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2019,6 +2019,14 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
> break;
> } while (1);
>
> + /*
> + * Our endpoint might get disabled by another thread during
> + * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
> + * early on so DWC3_EP_BUSY flag gets cleared
> + */
> + if (!dep->endpoint.desc)
> + return 1;
> +
> if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> list_empty(&dep->started_list)) {
> if (list_empty(&dep->pending_list)) {
> @@ -2085,6 +2093,14 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
> dwc->u1u2 = 0;
> }
>
> + /*
> + * Our endpoint might get disabled by another thread during
> + * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
> + * early on so DWC3_EP_BUSY flag gets cleared
> + */
> + if (!dep->endpoint.desc)
> + return;
> +
> if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
> int ret;
>
> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
> gadget.c (as in my testing/next from today) won't even get executed, so
> we're safe there.
Never will be executed? then we can remove the
usb_endpoint_xfer_isoc() (line 2025) at risk?
2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
2024 if (clean_busy && (is_xfer_complete ||
2025
usb_endpoint_xfer_isoc(dep->endpoint.desc)))
2026 dep->flags &= ~DWC3_EP_BUSY;
>
> --
> balbi
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists