| Message ID | 20240412135256.1546051-1-zhengyejian1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: dvb-usb: Fix unexpected infinite loop in dvb_usb_read_remote_control() | expand |
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 Sean
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
On Tue, Apr 30, 2024 at 05:19:56PM +0800, Zheng Yejian wrote: > 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) I don't think this is a good function name; we are checking that the endpoint is correct and also clearing halts. How about: dvb_usb_check_bulk_endpoint() Looks good otherwise Sean > +{ > + 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 >
On 2024/4/30 17:36, Sean Young wrote: > On Tue, Apr 30, 2024 at 05:19:56PM +0800, Zheng Yejian wrote: >> 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) > > I don't think this is a good function name; we are checking that the > endpoint is correct and also clearing halts. > > How about: dvb_usb_check_bulk_endpoint() Sure, I'll do it in v2! -- Thank, Zheng Yejian > > Looks good otherwise > > Sean > >> +{ >> + 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 >>
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)); }
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 ? :)