linux-kernel - Re: [RFT PATCH 1/4] usb: dwc2: Simplify and fix DMA alignment code

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f94fc372-d81b-e8e4-e7ef-780fe7db1237@roeck-us.net>
Date:   Thu, 27 Feb 2020 20:28:17 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Minas Harutyunyan <hminas@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Antti Seppälä <a.seppala@...il.com>,
        Boris ARZUR <boris@...bu.org>, linux-usb@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Martin Schiller <ms@....tdt.de>
Subject: Re: [RFT PATCH 1/4] usb: dwc2: Simplify and fix DMA alignment code

On 2/27/20 2:27 PM, Guenter Roeck wrote:
> On 2/27/20 2:06 PM, Doug Anderson wrote:
[ ... ]
>>> -       if (urb->num_sgs || urb->sg ||
>>> -           urb->transfer_buffer_length == 0 ||
>>> +       if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 ||
>>> +           (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) ||
>>>             !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
>>
>> Maybe I'm misunderstanding things, but it feels like we need something
>> more here.  Specifically I'm worried about the fact when the transfer
>> buffer is already aligned but the length is not a multiple of the
>> endpoint's maximum transfer size.  You need to handle that, right?
>> AKA something like this (untested):
>>
>> /* Simple case of not having to allocate a bounce buffer */
>> if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 ||
>>     (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
>>   return 0;
>>
>> /* Can also avoid bounce buffer if alignment and size are good */
>> maxp = usb_endpoint_maxp(&ep->desc);
>> if (maxp == urb->transfer_buffer_length &&
> 
> No, transfer_buffer_length would have to be a multiple of maxp. There
> are many situations where roundup(transfer_buffer_length, maxp) !=
> transfer_buffer_length. I agree, this would be the prudent approach
> (and it was my original implementation), but then it didn't seem to
> cause trouble so far, and I was hesitant to add it in because it results
> in creating temporary buffers for almost every receive operation.
> I'd like to get some test feedback from Boris - if the current code
> causes crashes with his use case, we'll know that it is needed.
> Otherwise, we'll have to decide if the current approach (with fewer
> copies) is worth the risk, or if we want to play save and always
> copy if roundup(transfer_buffer_length, maxp) != transfer_buffer_length.
> 

Thinking more about this, the situation is actually much worse:
In Boris' testing, he found inbound transactions requested by usb
storage code with a requested transfer size of 13 bytes ... with
URB_NO_TRANSFER_DMA_MAP set. This means the requesting code has
provided a DMA ready buffer, transfer_buffer isn't even used,
and we can not reallocate it. In this situation we can just hope
that the chip (and the connected USB device) don't send more data
than requested.

With that in mind, I think we should stick with the current
scheme (ie only allocate a new buffer if the provided buffer is
unaligned) unless Boris comes back and tells us that it doesn't
work.

Thanks,
Guenter