linux-kernel - RE: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

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]

Date:	Tue, 12 Nov 2013 23:09:19 +0000
From:	Paul Zimmerman <Paul.Zimmerman@...opsys.com>
To:	Alan Stern <stern@...land.harvard.edu>,
	David Cohen <david.a.cohen@...ux.intel.com>
CC:	Michal Nazarewicz <mina86@...a86.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCHv2 2/2] check quirk to pad epout buf size when not
 aligned to maxpacketsize

> From: linux-usb-owner@...r.kernel.org [mailto:linux-usb-owner@...r.kernel.org] On Behalf Of Alan Stern
> Sent: Tuesday, November 12, 2013 7:51 AM
> 
> On Mon, 11 Nov 2013, David Cohen wrote:
> 
> > Hi Alan, Michal,
> >
> > On 11/11/2013 01:09 PM, Michal Nazarewicz wrote:
> > > On Mon, Nov 11 2013, Alan Stern wrote:
> > >> On Mon, 11 Nov 2013, Michal Nazarewicz wrote:
> > >>
> > >>> Check gadget.quirk_ep_out_aligned_size to decide if buffer size requires
> > >>> to be aligned to maxpacketsize of an out endpoint.  ffs_epfile_io() needs
> > >>> to pad epout buffer to match above condition if quirk is found.
> > >>>
> > >>> Signed-off-by: Michal Nazarewicz <mina86@...a86.com>
> > >>
> > >> I think this is still wrong.
> > >>
> > >>> @@ -824,7 +832,7 @@ static ssize_t ffs_epfile_io(struct file *file,
> > >>>   		req->context  = &done;
> > >>>   		req->complete = ffs_epfile_io_complete;
> > >>>   		req->buf      = data;
> > >>> -		req->length   = len;
> > >>> +		req->length   = data_len;
> > >>
> > >> IIUC, req->length should still be set to len, not to data_len.
> >
> > I misunderstood the first time I read it:
> > In order to avoid DWC3 to stall, we need to update req->length (this is
> > the most important fix). kmalloc() is updated too to prevent USB
> > controller to overflow buffer boundaries.
> 
> Here I disagree.
> 
> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
> Gadget drivers should not have to worry.  Most especially, gadget
> drivers should not lie about a request length.

The whole point of this quirk is to lie to the DWC3 driver. The quirk is
only enabled for the DWC3 driver.

> If the UDC driver decides to round up req->length before sending it to
> the hardware, that's okay.

Not really. If the DWC3 driver unconditionally rounds up req->length,
then in the case where the buffer has not been expanded to a multiple of
maxpacket, by this quirk or otherwise, there is the potential to write
beyond the end of the allocation.

If we do what you suggest, then all the gadgets will have to be audited
to make sure an OUT buffer with a non-aligned length is never passed to
the DWC3 driver.

I still think that's the best solution anyway. Just make that the rule,
and then there is no need for this quirk at all. And there is no need to
round up req->length in the DWC3 driver either.

> But req->length should be set to len, not data_len.

According to gadget.h:

	@buf: Buffer used for data
	@length: Length of that data

So why shouldn't length be the length of the allocated data buffer?
Remember, this is for the OUT direction only, so the host has control
over how much data is actually sent. You could even argue that an OUT
data buffer should always be a multiple of the max packet size, given
how USB works.

> And if the hardware receives more than len bytes of data,
> the UDC driver should set req->status to -EOVERFLOW.

That would be nice, but I don't see how to accomplish that given the
above.

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/