Messages in this thread Patch in this message | | | Date | Tue, 30 Apr 2024 17:19:56 +0800 | Subject | Re: [PATCH] media: dvb-usb: Fix unexpected infinite loop in dvb_usb_read_remote_control() | From | Zheng Yejian <> |
| |
On 2024/4/30 16:04, Sean Young wrote: > On Mon, Apr 29, 2024 at 07:24:21PM +0800, Zheng Yejian wrote: >> On 2024/4/12 21:52, Zheng Yejian wrote: >>> Infinite log printing occurs during fuzz test: >>> >>> rc rc1: DViCO FusionHDTV DVB-T USB (LGZ201) as ... >>> ... >>> dvb-usb: schedule remote query interval to 100 msecs. >>> dvb-usb: DViCO FusionHDTV DVB-T USB (LGZ201) successfully initialized ... >>> dvb-usb: bulk message failed: -22 (1/0) >>> dvb-usb: bulk message failed: -22 (1/0) >>> dvb-usb: bulk message failed: -22 (1/0) >>> ... >>> dvb-usb: bulk message failed: -22 (1/0) >>> >>> Looking into the codes, there is a loop in dvb_usb_read_remote_control(), >>> that is in rc_core_dvb_usb_remote_init() create a work that will call >>> dvb_usb_read_remote_control(), and this work will reschedule itself at >>> 'rc_interval' intervals to recursively call dvb_usb_read_remote_control(), >>> see following code snippet: >>> >>> rc_core_dvb_usb_remote_init() { >>> ... >>> INIT_DELAYED_WORK(&d->rc_query_work, dvb_usb_read_remote_control); >>> schedule_delayed_work(&d->rc_query_work, >>> msecs_to_jiffies(rc_interval)); >>> ... >>> } >>> >>> dvb_usb_read_remote_control() { >>> ... >>> err = d->props.rc.core.rc_query(d); >>> if (err) >>> err(...) // Did not return even if query failed >>> schedule_delayed_work(&d->rc_query_work, >>> msecs_to_jiffies(rc_interval)); >>> } >>> >>> When the infinite log printing occurs, the query callback >>> 'd->props.rc.core.rc_query' is cxusb_rc_query(). And the log is due to >>> the failure of finding a valid 'generic_bulk_ctrl_endpoint' >>> in usb_bulk_msg(), see following code snippet: >>> >>> cxusb_rc_query() { >>> cxusb_ctrl_msg() { >>> dvb_usb_generic_rw() { >>> ret = usb_bulk_msg(d->udev, usb_sndbulkpipe(d->udev, >>> d->props.generic_bulk_ctrl_endpoint),...); >>> if (ret) >>> err("bulk message failed: %d (%d/%d)",ret,wlen,actlen); >>> ... >>> } >>> ... >>> } >>> >>> By analyzing the corresponding USB descriptor, it shows that the >>> bNumEndpoints is 0 in its interface descriptor, but >>> the 'generic_bulk_ctrl_endpoint' is 1, that means user don't configure >>> a valid endpoint for 'generic_bulk_ctrl_endpoint', therefore this >>> 'invalid' USB device should be rejected before it calls into >>> dvb_usb_read_remote_control(). >>> >>> To fix it, iiuc, we can add endpoint check in dvb_usb_adapter_init(). >>> >>> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> >>> --- >>> drivers/media/usb/dvb-usb/dvb-usb-init.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> Hi, I'm not very familiar with USB driver, and I'm not sure the >>> type check is appropriate or not here. Would be any device properties >>> that allow 'generic_bulk_ctrl_endpoint' not being configured, or would >>> it be configured late after init ? :) >>> >> >> Kindly ping :) >> Hi, Mauro, would you mind taking a look at this code ? >> >> -- >> Thanks, >> Zheng Yejian >> >>> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c >>> index fbf58012becd..48e7b9fb93dd 100644 >>> --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c >>> +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c >>> @@ -104,6 +104,14 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) >>> * sometimes a timeout occurs, this helps >>> */ >>> if (d->props.generic_bulk_ctrl_endpoint != 0) { >>> + ret = usb_pipe_type_check(d->udev, usb_sndbulkpipe(d->udev, >>> + d->props.generic_bulk_ctrl_endpoint)); >>> + if (ret) >>> + goto frontend_init_err; >>> + ret = usb_pipe_type_check(d->udev, usb_rcvbulkpipe(d->udev, >>> + d->props.generic_bulk_ctrl_endpoint)); >>> + if (ret) >>> + goto frontend_init_err; >>> usb_clear_halt(d->udev, usb_sndbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint)); >>> usb_clear_halt(d->udev, usb_rcvbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint)); >>> } >> > > Thank you for your patch. > > I think your change looks good. The only comment I have is that the same > check should be done for generic_bulk_ctrl_endpoint_response as well; the > usb_clear_halt() should be done as well, I think. >
Thanks for your suggestion! Do you mean the following change? If it is ok, I'll send a v2!
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c index fbf58012becd..2a8395d6373c 100644 --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c @@ -23,6 +23,23 @@ static int dvb_usb_force_pid_filter_usage; module_param_named(force_pid_filter_usage, dvb_usb_force_pid_filter_usage, int, 0444); MODULE_PARM_DESC(force_pid_filter_usage, "force all dvb-usb-devices to use a PID filter, if any (default: 0).");
+static int dvb_usb_clear_halt(struct dvb_usb_device *d, u8 endpoint) +{ + if (endpoint) { + int ret; + + ret = usb_pipe_type_check(d->udev, usb_sndbulkpipe(d->udev, endpoint)); + if (ret) + return ret; + ret = usb_pipe_type_check(d->udev, usb_rcvbulkpipe(d->udev, endpoint)); + if (ret) + return ret; + usb_clear_halt(d->udev, usb_sndbulkpipe(d->udev, endpoint)); + usb_clear_halt(d->udev, usb_rcvbulkpipe(d->udev, endpoint)); + } + return 0; +} + static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) { struct dvb_usb_adapter *adap; @@ -103,10 +120,12 @@ static int dvb_usb_adapter_init(struct dvb_usb_device *d, short *adapter_nrs) * when reloading the driver w/o replugging the device * sometimes a timeout occurs, this helps */ - if (d->props.generic_bulk_ctrl_endpoint != 0) { - usb_clear_halt(d->udev, usb_sndbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint)); - usb_clear_halt(d->udev, usb_rcvbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint)); - } + ret = dvb_usb_clear_halt(d, d->props.generic_bulk_ctrl_endpoint); + if (ret) + goto frontend_init_err; + ret = dvb_usb_clear_halt(d, d->props.generic_bulk_ctrl_endpoint_response); + if (ret) + goto frontend_init_err;
return 0;
-- Thanks, Zheng Yejian > Thanks > > Sean
| |