| Message ID | 20240430104137.1014471-1-zhengyejian1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: dvb-usb: Fix unexpected infinite loop in dvb_usb_read_remote_control() | expand |
On Tue, Apr 30, 2024 at 06:41:37PM +0800, 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, we need to add endpoint check for 'generic_bulk_ctrl_endpoint'. > And as Sean suggested, the same check and clear halts should be done for > 'generic_bulk_ctrl_endpoint_response'. So introduce > dvb_usb_check_bulk_endpoint() to do it for both of them. > > Fixes: 4d43e13f723e ("V4L/DVB (4643): Multi-input patch for DVB-USB device") > Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> > --- > drivers/media/usb/dvb-usb/dvb-usb-init.c | 27 ++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > v2: > - As Sean suggested, check endpoint and clear halt for both > 'generic_bulk_ctrl_endpoint' and 'generic_bulk_ctrl_endpoint_response' > with the new introduced dvb_usb_check_bulk_endpoint(); > Link: https://lore.kernel.org/all/ZjCl97Ww6NrzJQCB@gofer.mess.org/ > Link: https://lore.kernel.org/all/ZjC7rXU7ViaH60_S@gofer.mess.org/ > > - Add Fixes tag. > > v1: > - Link: https://lore.kernel.org/all/20240412135256.1546051-1-zhengyejian1@huawei.com/ > > diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c > index fbf58012becd..7eb321bab84f 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_check_bulk_endpoint(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_check_bulk_endpoint(d, d->props.generic_bulk_ctrl_endpoint); > + if (ret) > + goto frontend_init_err; > + ret = dvb_usb_check_bulk_endpoint(d, d->props.generic_bulk_ctrl_endpoint_response); > + if (ret) > + goto frontend_init_err; > > return 0; This results in the following warning: drivers/media/usb/dvb-usb/dvb-usb-init.c:133:9: warning: 'adap' may be used uninitialized [-Wmaybe-uninitialized] I think these tests should be moved to the top of the function, so no cleanup is required. 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..7eb321bab84f 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_check_bulk_endpoint(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_check_bulk_endpoint(d, d->props.generic_bulk_ctrl_endpoint); + if (ret) + goto frontend_init_err; + ret = dvb_usb_check_bulk_endpoint(d, d->props.generic_bulk_ctrl_endpoint_response); + if (ret) + goto frontend_init_err; return 0;
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, we need to add endpoint check for 'generic_bulk_ctrl_endpoint'. And as Sean suggested, the same check and clear halts should be done for 'generic_bulk_ctrl_endpoint_response'. So introduce dvb_usb_check_bulk_endpoint() to do it for both of them. Fixes: 4d43e13f723e ("V4L/DVB (4643): Multi-input patch for DVB-USB device") Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> --- drivers/media/usb/dvb-usb/dvb-usb-init.c | 27 ++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) v2: - As Sean suggested, check endpoint and clear halt for both 'generic_bulk_ctrl_endpoint' and 'generic_bulk_ctrl_endpoint_response' with the new introduced dvb_usb_check_bulk_endpoint(); Link: https://lore.kernel.org/all/ZjCl97Ww6NrzJQCB@gofer.mess.org/ Link: https://lore.kernel.org/all/ZjC7rXU7ViaH60_S@gofer.mess.org/ - Add Fixes tag. v1: - Link: https://lore.kernel.org/all/20240412135256.1546051-1-zhengyejian1@huawei.com/