| Message ID | 20240430104137.1014471-1-zhengyejian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
On 2024/5/3 19:51, Sean Young wrote: > 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. > Hi, Sean, I have fixed the warning in v3: https://lore.kernel.org/all/20240509124414.1392304-1-zhengyejian1@huawei.com/, would you mind taking a look at it ? I introduce a new funciton dvb_usb_clear_halt() to clear sndbulkpipe and rcvbulkpipe, dvb_usb_check_bulk_endpoint() only do pipe type check and is moved to the top of the function. -- Thanks, Zheng Yejian > > 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/