[PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Gautam Menghani]

The warning "URB submitted while active" is reported if the function
send_packet() in imon.c is called if a write is already is in progress.
Add a check to return -EBUSY in case a write is already is in progress.
Also, mark tx.busy as false after transmission is completed.

Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
Cc: hdanton@sina.com
Suggested-by: hdanton@sina.com
Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
---
 drivers/media/rc/imon.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 735b925da998..a5b997c2c7e2 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
 	int retval = 0;
 	struct usb_ctrlrequest *control_req = NULL;
 
+	if (ictx->tx.busy)
+		return -EBUSY;
 	/* Check if we need to use control or interrupt urb */
 	if (!ictx->tx_control) {
 		pipe = usb_sndintpipe(ictx->usbdev_intf0,
@@ -654,6 +656,7 @@ static int send_packet(struct imon_context *ictx)
 			pr_err_ratelimited("task interrupted\n");
 		}
 		mutex_lock(&ictx->lock);
+		ictx->tx.busy = false;
 
 		retval = ictx->tx.status;
 		if (retval)
-- 
2.34.1

Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Greg KH]

On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> The warning "URB submitted while active" is reported if the function
> send_packet() in imon.c is called if a write is already is in progress.
> Add a check to return -EBUSY in case a write is already is in progress.
> Also, mark tx.busy as false after transmission is completed.
> 
> Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> Cc: hdanton@sina.com
> Suggested-by: hdanton@sina.com
> Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> ---
>  drivers/media/rc/imon.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 735b925da998..a5b997c2c7e2 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
>  	int retval = 0;
>  	struct usb_ctrlrequest *control_req = NULL;
>  
> +	if (ictx->tx.busy)
> +		return -EBUSY;

What happens if we go busy right after this check?  Where is the locking
here to protect this?

thanks,

greg k-h

Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Gautam Menghani]

On Sun, Aug 14, 2022 at 04:45:08PM +0200, Greg KH wrote:
> On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> > The warning "URB submitted while active" is reported if the function
> > send_packet() in imon.c is called if a write is already is in progress.
> > Add a check to return -EBUSY in case a write is already is in progress.
> > Also, mark tx.busy as false after transmission is completed.
> > 
> > Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> > Cc: hdanton@sina.com
> > Suggested-by: hdanton@sina.com
> > Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> > Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> > Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> > ---
> >  drivers/media/rc/imon.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> > index 735b925da998..a5b997c2c7e2 100644
> > --- a/drivers/media/rc/imon.c
> > +++ b/drivers/media/rc/imon.c
> > @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
> >  	int retval = 0;
> >  	struct usb_ctrlrequest *control_req = NULL;
> >  
> > +	if (ictx->tx.busy)
> > +		return -EBUSY;
> 
> What happens if we go busy right after this check?  Where is the locking
> here to protect this?

All calls to send_packet() are protected with ictx->lock() held. Are you referring 
to something else?

Also, if we return busy, the task is interrupted and the packet transaction fails, 
just like the current behaviour. With the above patch, warning is not triggered.
Here's the log from running the reproducer (with patch applied).

imon 1-1:0.0: Looks like you're trying to use an IR protocol this device does not support
imon 1-1:0.0: Unsupported IR protocol specified, overriding to iMON IR protocol
rc rc0: iMON Remote (15c2:0040) as /devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.0/rc/rc0
input: iMON Remote (15c2:0040) as /devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.0/rc/rc0/input5
imon 1-1:0.0: iMON device (15c2:0040, intf0) on usb<1:2> initialized
imon:vfd_write: send packet #0 failed
imon:send_packet: task interrupted
imon:send_packet: packet tx failed (-2)
imon:vfd_write: send packet #0 failed
usb 1-1: USB disconnect, device number 2

Thanks,
Gautam

Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Gautam Menghani]

On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> The warning "URB submitted while active" is reported if the function
> send_packet() in imon.c is called if a write is already is in progress.
> Add a check to return -EBUSY in case a write is already is in progress.
> Also, mark tx.busy as false after transmission is completed.
> 
> Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> Cc: hdanton@sina.com
> Suggested-by: hdanton@sina.com
> Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> ---
>  drivers/media/rc/imon.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 735b925da998..a5b997c2c7e2 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
>  	int retval = 0;
>  	struct usb_ctrlrequest *control_req = NULL;
>  
> +	if (ictx->tx.busy)
> +		return -EBUSY;
>  	/* Check if we need to use control or interrupt urb */
>  	if (!ictx->tx_control) {
>  		pipe = usb_sndintpipe(ictx->usbdev_intf0,
> @@ -654,6 +656,7 @@ static int send_packet(struct imon_context *ictx)
>  			pr_err_ratelimited("task interrupted\n");
>  		}
>  		mutex_lock(&ictx->lock);
> +		ictx->tx.busy = false;
>  
>  		retval = ictx->tx.status;
>  		if (retval)
> -- 
> 2.34.1
> 
Hi,

Please review the above fix and let me know if any changes are needed.

Thanks,
Gautam

Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Sean Young]

On Fri, Sep 02, 2022 at 11:42:41PM +0530, Gautam Menghani wrote:
> On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> > The warning "URB submitted while active" is reported if the function
> > send_packet() in imon.c is called if a write is already is in progress.
> > Add a check to return -EBUSY in case a write is already is in progress.
> > Also, mark tx.busy as false after transmission is completed.
> > 
> > Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> > Cc: hdanton@sina.com
> > Suggested-by: hdanton@sina.com
> > Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> > Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> > Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> > ---
> >  drivers/media/rc/imon.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> > index 735b925da998..a5b997c2c7e2 100644
> > --- a/drivers/media/rc/imon.c
> > +++ b/drivers/media/rc/imon.c
> > @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
> >  	int retval = 0;
> >  	struct usb_ctrlrequest *control_req = NULL;
> >  
> > +	if (ictx->tx.busy)
> > +		return -EBUSY;
> >  	/* Check if we need to use control or interrupt urb */
> >  	if (!ictx->tx_control) {
> >  		pipe = usb_sndintpipe(ictx->usbdev_intf0,
> > @@ -654,6 +656,7 @@ static int send_packet(struct imon_context *ictx)
> >  			pr_err_ratelimited("task interrupted\n");
> >  		}
> >  		mutex_lock(&ictx->lock);
> > +		ictx->tx.busy = false;
> >  
> >  		retval = ictx->tx.status;
> >  		if (retval)
> > -- 
> > 2.34.1
> > 
> Hi,
> 
> Please review the above fix and let me know if any changes are needed.

Greg has pointed out issues with this patch: there is no locking.

Thanks,

Sean

Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Gautam Menghani]

On Sat, Sep 03, 2022 at 08:49:56AM +0100, Sean Young wrote:
> On Fri, Sep 02, 2022 at 11:42:41PM +0530, Gautam Menghani wrote:
> > On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> > > The warning "URB submitted while active" is reported if the function
> > > send_packet() in imon.c is called if a write is already is in progress.
> > > Add a check to return -EBUSY in case a write is already is in progress.
> > > Also, mark tx.busy as false after transmission is completed.
> > > 
> > > Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> > > Cc: hdanton@sina.com
> > > Suggested-by: hdanton@sina.com
> > > Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> > > Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> > > Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> > > ---
> > >  drivers/media/rc/imon.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> > > index 735b925da998..a5b997c2c7e2 100644
> > > --- a/drivers/media/rc/imon.c
> > > +++ b/drivers/media/rc/imon.c
> > > @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
> > >  	int retval = 0;
> > >  	struct usb_ctrlrequest *control_req = NULL;
> > >  
> > > +	if (ictx->tx.busy)
> > > +		return -EBUSY;
> > >  	/* Check if we need to use control or interrupt urb */
> > >  	if (!ictx->tx_control) {
> > >  		pipe = usb_sndintpipe(ictx->usbdev_intf0,
> > > @@ -654,6 +656,7 @@ static int send_packet(struct imon_context *ictx)
> > >  			pr_err_ratelimited("task interrupted\n");
> > >  		}
> > >  		mutex_lock(&ictx->lock);
> > > +		ictx->tx.busy = false;
> > >  
> > >  		retval = ictx->tx.status;
> > >  		if (retval)
> > > -- 
> > > 2.34.1
> > > 
> > Hi,
> > 
> > Please review the above fix and let me know if any changes are needed.
> 
> Greg has pointed out issues with this patch: there is no locking.
> 
> Thanks,
> 
> Sean

Hi,

I am a bit confused about the locking part. All the calls to send_packet() are
made with ictx->lock held. So will the following be sufficient?

lockdep_assert_held(&ictx->lock);
if (ictx->tx.busy)
	return -EBUSY;

Or do we need to add a mutex/spin lock inside ictx->tx structure? 

Or should we instead wait for the the tx to be completed as follows:
wait_for_completion(&ictx->tx.finished);

Please advise.

Thanks,
Gautam

Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Greg KH]

On Sat, Sep 03, 2022 at 03:08:23PM +0530, Gautam Menghani wrote:
> On Sat, Sep 03, 2022 at 08:49:56AM +0100, Sean Young wrote:
> > On Fri, Sep 02, 2022 at 11:42:41PM +0530, Gautam Menghani wrote:
> > > On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> > > > The warning "URB submitted while active" is reported if the function
> > > > send_packet() in imon.c is called if a write is already is in progress.
> > > > Add a check to return -EBUSY in case a write is already is in progress.
> > > > Also, mark tx.busy as false after transmission is completed.
> > > > 
> > > > Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> > > > Cc: hdanton@sina.com
> > > > Suggested-by: hdanton@sina.com
> > > > Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> > > > Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> > > > Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> > > > ---
> > > >  drivers/media/rc/imon.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> > > > index 735b925da998..a5b997c2c7e2 100644
> > > > --- a/drivers/media/rc/imon.c
> > > > +++ b/drivers/media/rc/imon.c
> > > > @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
> > > >  	int retval = 0;
> > > >  	struct usb_ctrlrequest *control_req = NULL;
> > > >  
> > > > +	if (ictx->tx.busy)
> > > > +		return -EBUSY;
> > > >  	/* Check if we need to use control or interrupt urb */
> > > >  	if (!ictx->tx_control) {
> > > >  		pipe = usb_sndintpipe(ictx->usbdev_intf0,
> > > > @@ -654,6 +656,7 @@ static int send_packet(struct imon_context *ictx)
> > > >  			pr_err_ratelimited("task interrupted\n");
> > > >  		}
> > > >  		mutex_lock(&ictx->lock);
> > > > +		ictx->tx.busy = false;
> > > >  
> > > >  		retval = ictx->tx.status;
> > > >  		if (retval)
> > > > -- 
> > > > 2.34.1
> > > > 
> > > Hi,
> > > 
> > > Please review the above fix and let me know if any changes are needed.
> > 
> > Greg has pointed out issues with this patch: there is no locking.
> > 
> > Thanks,
> > 
> > Sean
> 
> Hi,
> 
> I am a bit confused about the locking part. All the calls to send_packet() are
> made with ictx->lock held. So will the following be sufficient?
> 
> lockdep_assert_held(&ictx->lock);
> if (ictx->tx.busy)
> 	return -EBUSY;
> 
> Or do we need to add a mutex/spin lock inside ictx->tx structure? 

If a lock is held, how can this function be called at the same time
through different ways?  That should not happen.

> Or should we instead wait for the the tx to be completed as follows:
> wait_for_completion(&ictx->tx.finished);

It depends on what you are trying to protect from here.  What is the
real issue that is allowing multiple submissions of the same urb to
happen?  Fix that issue instead of trying to paper over it in this lower
level function please.

thanks,

greg k-h