media: dvb: usb: Fix WARNING in dib0700_i2c_xfer/usb_submit_urb

Commit Message

Alan Stern March 25, 2025, 7:28 p.m. UTC

The syzbot fuzzer reported a WARNING related to the dib0700 dvb-usb
driver:

usb 1-1: BOGUS control dir, pipe 80000f80 doesn't match bRequestType c0
WARNING: CPU: 1 PID: 5901 at drivers/usb/core/urb.c:413 usb_submit_urb+0x11d9/0x18c0 drivers/usb/core/urb.c:411
...
Call Trace:
 <TASK>
 usb_start_wait_urb+0x113/0x520 drivers/usb/core/message.c:59
 usb_internal_control_msg drivers/usb/core/message.c:103 [inline]
 usb_control_msg+0x2b1/0x4c0 drivers/usb/core/message.c:154
 dib0700_ctrl_rd drivers/media/usb/dvb-usb/dib0700_core.c:95 [inline]
 dib0700_i2c_xfer_legacy drivers/media/usb/dvb-usb/dib0700_core.c:315 [inline]
 dib0700_i2c_xfer+0xc53/0x1060 drivers/media/usb/dvb-usb/dib0700_core.c:361
 __i2c_transfer+0x866/0x2220
 i2c_transfer+0x271/0x3b0 drivers/i2c/i2c-core-base.c:2315
 i2cdev_ioctl_rdwr+0x452/0x710 drivers/i2c/i2c-dev.c:306
 i2cdev_ioctl+0x759/0x9f0 drivers/i2c/i2c-dev.c:467
 vfs_ioctl fs/ioctl.c:51 [inline]

Evidently the fuzzer submitted an I2C transfer containing a length-0
read message.  The dib0700 driver translated this more or less
literally into a length-0 USB read request.  But the USB protocol does
not allow reads to have length 0; all length-0 transfers are
considered to be writes.  Hence the WARNING above.

Fix the problem by adding the I2C_AQ_NO_ZERO_LEN_READ adapter quirk
flag to all the USB I2C adapter devices managed by dvb-usb-i2c.c,
following Wolfram Sang's suggestion.  This tells the I2C core not to
allow length-0 read messages.

Reported-by: [e-mail]
Tested-by: [e-mail]
Suggested-by: Wolfram Sang <[e-mail]>
Signed-off-by: Alan Stern <[e-mail]>
Link: https://lore.kernel.org/linux-usb/[e-mail]/

---

Comments

Wolfram Sang March 25, 2025, 7:56 p.m. UTC | #1

> +	static const struct i2c_adapter_quirks i2c_usb_quirks = {
> +		.flags = I2C_AQ_NO_ZERO_LEN_READ,
> +	};

Why didn't you create the static struct outside of probe?

Alan Stern March 25, 2025, 9:47 p.m. UTC | #2

On Tue, Mar 25, 2025 at 08:56:57PM +0100, Wolfram Sang wrote:
> 
> > +	static const struct i2c_adapter_quirks i2c_usb_quirks = {
> > +		.flags = I2C_AQ_NO_ZERO_LEN_READ,
> > +	};
> 
> Why didn't you create the static struct outside of probe?

Because it's used only in that one function.  But if you prefer, I will 
move the definition outside of the function.  It doesn't make any real 
difference.

Alan Stern

Wolfram Sang March 25, 2025, 10:17 p.m. UTC | #3

On Tue, Mar 25, 2025 at 05:47:53PM -0400, Alan Stern wrote:
> On Tue, Mar 25, 2025 at 08:56:57PM +0100, Wolfram Sang wrote:
> > 
> > > +	static const struct i2c_adapter_quirks i2c_usb_quirks = {
> > > +		.flags = I2C_AQ_NO_ZERO_LEN_READ,
> > > +	};
> > 
> > Why didn't you create the static struct outside of probe?
> 
> Because it's used only in that one function.  But if you prefer, I will 
> move the definition outside of the function.  It doesn't make any real 
> difference.

Then, for consistency reasons, I'd really prefer it outside probe. I
also think it doesn't really make a difference, it just looks unusual to
me.

Thanks and happy hacking!

Patch

Index: usb-devel/drivers/media/usb/dvb-usb/dvb-usb-i2c.c
===================================================================
--- usb-devel.orig/drivers/media/usb/dvb-usb/dvb-usb-i2c.c
+++ usb-devel/drivers/media/usb/dvb-usb/dvb-usb-i2c.c
@@ -10,6 +10,9 @@ 
 
 int dvb_usb_i2c_init(struct dvb_usb_device *d)
 {
+	static const struct i2c_adapter_quirks i2c_usb_quirks = {
+		.flags = I2C_AQ_NO_ZERO_LEN_READ,
+	};
 	int ret = 0;
 
 	if (!(d->props.caps & DVB_USB_IS_AN_I2C_ADAPTER))
@@ -24,6 +27,7 @@  int dvb_usb_i2c_init(struct dvb_usb_devi
 	strscpy(d->i2c_adap.name, d->desc->name, sizeof(d->i2c_adap.name));
 	d->i2c_adap.algo      = d->props.i2c_algo;
 	d->i2c_adap.algo_data = NULL;
+	d->i2c_adap.quirks    = &i2c_usb_quirks;
 	d->i2c_adap.dev.parent = &d->udev->dev;
 
 	i2c_set_adapdata(&d->i2c_adap, d);