linux-kernel - Re: [PATCH] USB: output an error message when the pipe type doesn't match the endpoint type

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: <4C7E87DD.3080508@simon.arlott.org.uk>
Date:	Wed, 01 Sep 2010 18:05:33 +0100
From:	Simon Arlott <simon@...e.lp0.eu>
To:	Alan Stern <stern@...land.harvard.edu>
CC:	Greg KH <greg@...ah.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	USB list <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] USB: output an error message when the pipe type doesn't
 match the endpoint type

On 31/08/10 15:16, Alan Stern wrote:
> On Mon, 30 Aug 2010, Simon Arlott wrote:
>> Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if
>> CONFIG_USB_DEBUG is enabled, but it doesn't output anything if this scenario
>> occurs.
>> 
>> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
>> index 419e6b3..c14fc08 100644
>> --- a/drivers/usb/core/urb.c
>> +++ b/drivers/usb/core/urb.c
>> @@ -401,8 +401,11 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>>  	/* Check that the pipe's type matches the endpoint's type */
>> -	if (usb_pipetype(urb->pipe) != pipetypes[xfertype])
>> +	if (usb_pipetype(urb->pipe) != pipetypes[xfertype]) {
>> +		dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
>> +			usb_pipetype(urb->pipe), pipetypes[xfertype]);
>>  		return -EPIPE;		/* The most suitable error code :-) */
>> +	}
> 
> This is okay with me.  If you're serious about not changing the
> behavior merely because debugging is enabled, you could move this test
> out of the debug-only region and possibly change the dev_err to
> dev_dbg.  However doing so might break some devices that are currently
> working.

I'd expect that to break potentially many devices, although only cxacru
stopped working for me. The USB API isn't really suitable for adding
this type of check because it allows the drivers to get away with too
much already.

usb_clear_halt() takes a pipe when it really wants the endpoint, the
pipe type is ignored.

usb_bulk_msg() auto-detects the type between interrupt and bulk, as
does usb_interrupt_msg() because the latter just calls the former.

(I think -EINVAL might be a better return code. The pipe isn't broken,
it doesn't exist.)

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