linux-kernel - Re: [PATCH v4 3/4] usb: ffs: 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]

Message-ID: <Pine.LNX.4.44L0.1311051026390.1360-100000@iolanthe.rowland.org>
Date:	Tue, 5 Nov 2013 10:38:29 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	David Cohen <david.a.cohen@...ux.intel.com>
cc:	balbi@...com, <gregkh@...uxfoundation.org>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when
 not aligned to maxpacketsize

On Tue, 5 Nov 2013, David Cohen wrote:

> >> +		/*
> >> +		 * Controller requires buffer size to be aligned to
> >> +		 * maxpacketsize of an out endpoint.
> >> +		 */
> >> +		if (gadget->quirk_ep_out_aligned_size && read) {
> >> +			/*
> >> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
> >> +			 * due to we're in a loop and 'len' may have been
> >> +			 * changed.
> >> +			 */
> >> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
> >> +			if (data && len > data_len) {
> >> +				kfree(data);
> >> +				data = NULL;
> >> +				data_len = 0;
> >> +			}
> >> +		}
> > 
> > Since the value of orig_len never changes, there's no point calling 
> > usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
> > once, before the loop starts.  Once you do that, you won't need 
> > orig_len at all.
> 
> orig_len doesn't change but ep->ep does. If USB specs say max packet
> size won't change even if ep does, than we can call it from outside the
> loop.

I'm not too familiar with this driver.  It looks like the only way 
ep->ep can change is if the endpoint gets enabled while you're sitting 
inside the wait_event_interruptible() call.

In fact, the whole structure of that loop looks peculiar.  Why not
acquire the mutex first and then do everything else?

Does it even make sense for ep to change?  Would this change be visible
to the host?  What if the host changes the alternate setting while this
loop is running -- does it make sense for the userspace program to
start a read or write under one altsetting but then have the read/write
take place under a different altsetting?

Alan Stern

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