On Tue, 03 Oct 2017 16:21:57 +0200, Alan Stern wrote:
On Tue, 3 Oct 2017, Takashi Iwai wrote:
On Mon, 25 Sep 2017 14:39:51 +0200, Andrey Konovalov wrote:
Hi!
I've got the following report while fuzzing the kernel with syzkaller.
On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
It seems that there's no check of the endpoint type.
usb 1-1: BOGUS urb xfer, pipe 1 != type 3 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
How is this bug triggered? As it's syzkaller with QEMU, it looks hitting an inconsistent state the driver didn't expect (it sets the fixed endpoint), then USB-core detects the inconsistency and spews the kernel warning with stack trace. If so, it's no serious problem as it appears.
Suppose my guess is right, I'm not sure what's the best way to fix this. Certainly we can add more sanity check in the caller side. OTOH, I find the reaction of USB core too aggressive, it's not necessary to be dev_WARN() but a normal dev_err(). Or I might be looking at a wrong place?
It's a dev_WARN because it indicates a potentially serious error in the driver: The driver has submitted an interrupt URB to a bulk endpoint. That may not sound bad, but the same check gets triggered if a driver submits a bulk URB to an isochronous endpoint, or any other invalid combination.
Most likely the explanation here is that the driver doesn't bother to check the endpoint type because it expects the endpoint will always be interrupt. But that is not a safe strategy. USB devices and their firmware should not be trusted unnecessarily.
The best fix is, like you said, to add a sanity check in the caller.
OK, but then do we have some handy helper for the check? As other bug reports by syzkaller suggest, there are a few other drivers that do the same, submitting a urb with naive assumption of the fixed EP for specific devices. In the end we'll need to put the very same checks there in multiple places.
thanks,
Takashi