linux-kernel - Re: [PATCH 2/3] usb: dwc2: host: do not delay retries after successful transfer

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] [day] [month] [year] [list]

Message-ID: <CAD=FV=W3uhhv1sqfYkErXk3rpNiN_u91=os9rG=NHJqMvrKWgg@mail.gmail.com>
Date:   Tue, 17 Jul 2018 12:58:50 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Minas Harutyunyan <hminas@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] usb: dwc2: host: do not delay retries after
 successful transfer

Hi,

On Fri, Jun 15, 2018 at 4:49 PM, Doug Anderson <dianders@...omium.org> wrote:
> Hi,
>
> On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
>> When handling split transactions we should not be delaying retrying
>> SSPLIT/CSPLIT after we successfully communicate with the device, so
>> let's reset dtd->num_naks counter when handling XFERCOMPL.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
>> ---
>>  drivers/usb/dwc2/hcd_intr.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Reviewed-by: Douglas Anderson <dianders@...omium.org>

Though this looked good, I just tested this myself and found that it
corrupts memory.  Please do not merge it.

Specifically slub_debug complains of a use after free and sure enough
I can see that in one case we call dwc2_xfercomp_isoc_split_in() and
it can call dwc2_release_channel() and that can free the qtd.  Setting
the qtd->num_naks to 0 at the end of the function is a use after free.

I've just tested the same USB peripheral that Dmitry was testing with.
I found two things:

* This patch is important for making that peripheral work

* When I move the "qtd->num_naks" to the beginning it's all good.

Dmitry is OOTO at the moment, so I'll re-post this patch with that change.

-Doug