[PATCH v2] usb: gadget: dummy_hcd: fix gpf in gadget_setup

2021-04-18 Thread Anirudh Rayabharam 
Fix a general protection fault reported by syzbot due to a race between
gadget_setup() and gadget_unbind() in raw_gadget.

The gadget core is supposed to guarantee that there won't be any more
callbacks to the gadget driver once the driver's unbind routine is
called. That guarantee is enforced in usb_gadget_remove_driver as
follows:

usb_gadget_disconnect(udc->gadget);
if (udc->gadget->irq)
synchronize_irq(udc->gadget->irq);
udc->driver->unbind(udc->gadget);
usb_gadget_udc_stop(udc);

usb_gadget_disconnect turns off the pullup resistor, telling the host
that the gadget is no longer connected and preventing the transmission
of any more USB packets. Any packets that have already been received
are sure to processed by the UDC driver's interrupt handler by the time
synchronize_irq returns.

But this doesn't work with dummy_hcd, because dummy_hcd doesn't use
interrupts; it uses a timer instead. It does have code to emulate the
effect of synchronize_irq, but that code doesn't get invoked at the
right time -- it currently runs in usb_gadget_udc_stop, after the unbind
callback instead of before. Indeed, there's no way for
usb_gadget_remove_driver to invoke this code before the unbind callback.

To fix this, move the synchronize_irq() emulation code to dummy_pullup
so that it runs before unbind. Also, add a comment explaining why it is
necessary to have it there.

Suggested-by: Alan Stern 
Reported-by: syzbot+eb4674092e6cc8d9e...@syzkaller.appspotmail.com
Acked-by: Alan Stern 
Signed-off-by: Anirudh Rayabharam 
---

Changes in v2:
Improvements in the comment as suggested by Alan Stern. 

v1: https://lore.kernel.org/lkml/20210417125212.6274-1-m...@anirudhrb.com/

---
 drivers/usb/gadget/udc/dummy_hcd.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
b/drivers/usb/gadget/udc/dummy_hcd.c
index ce24d4f28f2a..7db773c87379 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -903,6 +903,21 @@ static int dummy_pullup(struct usb_gadget *_gadget, int 
value)
spin_lock_irqsave(&dum->lock, flags);
dum->pullup = (value != 0);
set_link_state(dum_hcd);
+   if (value == 0) {
+   /*
+* Emulate synchronize_irq(): wait for callbacks to finish.
+* This seems to be the best place to emulate the call to
+* synchronize_irq() that's in usb_gadget_remove_driver().
+* Doing it in dummy_udc_stop() would be too late since it
+* is called after the unbind callback and unbind shouldn't
+* be invoked until all the other callbacks are finished.
+*/
+   while (dum->callback_usage > 0) {
+   spin_unlock_irqrestore(&dum->lock, flags);
+   usleep_range(1000, 2000);
+   spin_lock_irqsave(&dum->lock, flags);
+   }
+   }
spin_unlock_irqrestore(&dum->lock, flags);
 
usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd));
@@ -1004,14 +1019,6 @@ static int dummy_udc_stop(struct usb_gadget *g)
spin_lock_irq(&dum->lock);
dum->ints_enabled = 0;
stop_activity(dum);
-
-   /* emulate synchronize_irq(): wait for callbacks to finish */
-   while (dum->callback_usage > 0) {
-   spin_unlock_irq(&dum->lock);
-   usleep_range(1000, 2000);
-   spin_lock_irq(&dum->lock);
-   }
-
dum->driver = NULL;
spin_unlock_irq(&dum->lock);
 
-- 
2.26.2

[PATCH] usb: gadget: dummy_hcd: fix gpf in gadget_setup

2021-04-17 Thread Anirudh Rayabharam 
Fix a general protection fault reported by syzbot due to a race between
gadget_setup() and gadget_unbind() in raw_gadget.

The gadget core is supposed to guarantee that there won't be any more
callbacks to the gadget driver once the driver's unbind routine is
called. That guarantee is enforced in usb_gadget_remove_driver as
follows:

usb_gadget_disconnect(udc->gadget);
if (udc->gadget->irq)
synchronize_irq(udc->gadget->irq);
udc->driver->unbind(udc->gadget);
usb_gadget_udc_stop(udc);

usb_gadget_disconnect turns off the pullup resistor, telling the host
that the gadget is no longer connected and preventing the transmission
of any more USB packets. Any packets that have already been received
are sure to processed by the UDC driver's interrupt handler by the time
synchronize_irq returns.

But this doesn't work with dummy_hcd, because dummy_hcd doesn't use
interrupts; it uses a timer instead.  It does have code to emulate the
effect of synchronize_irq, but that code doesn't get invoked at the
right time -- it currently runs in usb_gadget_udc_stop, after the unbind
callback instead of before.  Indeed, there's no way for
usb_gadget_remove_driver to invoke this code before the unbind
callback.

To fix this, move the synchronize_irq() emulation code to dummy_pullup
so that it runs before unbind. Also, add a comment explaining why it is
necessary to have it there.

Suggested-by: Alan Stern 
Reported-by: syzbot+eb4674092e6cc8d9e...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam 
---
 drivers/usb/gadget/udc/dummy_hcd.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
b/drivers/usb/gadget/udc/dummy_hcd.c
index ce24d4f28f2a..d0dae6406612 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -903,6 +903,21 @@ static int dummy_pullup(struct usb_gadget *_gadget, int 
value)
spin_lock_irqsave(&dum->lock, flags);
dum->pullup = (value != 0);
set_link_state(dum_hcd);
+   if (value == 0) {
+   /*
+* emulate synchronize_irq(): wait for callbacks to finish
+* This seems to be the best to place to emulate the call
+* to synchronize_irq(). Doing it in dummy_udc_stop() would
+* be too late since it is called after the unbind callback.
+* Also, there is no way for core:usb_gadget_remove_driver()
+* to invoke this code before the unbind callback.
+*/
+   while (dum->callback_usage > 0) {
+   spin_unlock_irqrestore(&dum->lock, flags);
+   usleep_range(1000, 2000);
+   spin_lock_irqsave(&dum->lock, flags);
+   }
+   }
spin_unlock_irqrestore(&dum->lock, flags);
 
usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd));
@@ -1004,14 +1019,6 @@ static int dummy_udc_stop(struct usb_gadget *g)
spin_lock_irq(&dum->lock);
dum->ints_enabled = 0;
stop_activity(dum);
-
-   /* emulate synchronize_irq(): wait for callbacks to finish */
-   while (dum->callback_usage > 0) {
-   spin_unlock_irq(&dum->lock);
-   usleep_range(1000, 2000);
-   spin_lock_irq(&dum->lock);
-   }
-
dum->driver = NULL;
spin_unlock_irq(&dum->lock);
 
-- 
2.26.2

Re: [syzbot] general protection fault in gadget_setup

2021-04-16 Thread Anirudh Rayabharam 
On Fri, Apr 16, 2021 at 11:27:34AM -0400, Alan Stern wrote:
> On Fri, Apr 16, 2021 at 11:10:35AM +0530, Anirudh Rayabharam wrote:
> > On Tue, Apr 13, 2021 at 12:13:11PM -0400, Alan Stern wrote:
> > > Maybe we can test this reasoning by putting a delay just before the call 
> > > to dum->driver->setup.  That runs in the timer handler, so it's not a 
> > > good place to delay, but it may be okay just for testing purposes.
> > > 
> > > Hopefully this patch will make the race a lot more likely to occur.  Is 
> > 
> > Hi Alan, 
> > 
> > Indeed, I was able to reproduce this bug easily on my machine with your
> > delay patch applied and using this syzkaller program:
> > 
> > syz_usb_connect$cdc_ncm(0x1, 0x6e, &(0x7f40)={{0x12, 0x1, 0x0, 0x2, 
> > 0x0, 0x0, 0x8, 0x525, 0xa4a1, 0x40, 0x1, 0x2, 0x3, 0x1, [{{0x9, 0x2, 0x5c, 
> > 0x2, 0x1, 0x0, 0x0, 0x0, {{0x9, 0x4, 0x0, 0x0, 0x1, 0x2, 0xd, 0x0, 0x0, 
> > {{0x5}, {0x5}, {0xd}, {0x6}}, {{0x9, 0x5, 0x81, 0x3, 0x200}}]}}, 
> > &(0x7f000480)={0x0, 0x0, 0x0, 0x0, 0x3, [{0x0, 0x0}, {0x0, 0x0}, {0x0, 
> > 0x0}]})
> > 
> > I also tested doing the synchronize_irq emulation in dummy_pullup and it
> > fixed the issue. The patch is below.
> 
> That's great!  Thanks for testing.
> 
> > Thanks!
> > 
> > - Anirudh.
> > 
> > diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
> > b/drivers/usb/gadget/udc/dummy_hcd.c
> > index ce24d4f28f2a..931d4612d859 100644
> > --- a/drivers/usb/gadget/udc/dummy_hcd.c
> > +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> > @@ -903,6 +903,12 @@ static int dummy_pullup(struct usb_gadget *_gadget, 
> > int value)
> > spin_lock_irqsave(&dum->lock, flags);
> > dum->pullup = (value != 0);
> > set_link_state(dum_hcd);
> > +   /* emulate synchronize_irq(): wait for callbacks to finish */ 
> > +   while (dum->callback_usage > 0) {
> > +   spin_unlock_irqrestore(&dum->lock, flags);
> > +   usleep_range(1000, 2000);
> > +   spin_lock_irqsave(&dum->lock, flags);
> > +   }
> 
> We should do this only if value == 0.  No synchronization is needed when 
> the pullup is turned on.
 
Oh right! My bad.

> Also, there should be a comment explaining that this is necessary 
> because there's no other place to emulate the call made to 
> synchronize_irq() in core.c:usb_gadget_remove_driver().

Will do.

> > spin_unlock_irqrestore(&dum->lock, flags);
> >  
> > usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd));
> > @@ -1005,13 +1011,6 @@ static int dummy_udc_stop(struct usb_gadget *g)
> > dum->ints_enabled = 0;
> > stop_activity(dum);
> >  
> > -   /* emulate synchronize_irq(): wait for callbacks to finish */
> > -   while (dum->callback_usage > 0) {
> > -   spin_unlock_irq(&dum->lock);
> > -   usleep_range(1000, 2000);
> > -   spin_lock_irq(&dum->lock);
> > -   }
> > -
> > dum->driver = NULL;
> > spin_unlock_irq(&dum->lock);
> 
> Actually, I wanted to move this emulation code into a new subroutine and 
> then call that subroutine from _both_ places.  Would you like to write 

Does it really need to be called from both places?

> and submit a patch that does this?

Sure! I will do that.

Thanks!

- Anirudh.

Re: [syzbot] general protection fault in gadget_setup

2021-04-15 Thread Anirudh Rayabharam 
On Tue, Apr 13, 2021 at 12:13:11PM -0400, Alan Stern wrote:
> On Tue, Apr 13, 2021 at 10:12:05AM +0200, Dmitry Vyukov wrote:
> > On Tue, Apr 13, 2021 at 10:08 AM syzbot
> >  wrote:
> > >
> > > Hello,
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit:0f4498ce Merge tag 'for-5.12/dm-fixes-2' of 
> > > git://git.kern..
> > > git tree:   upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=124adbf6d0
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=daeff30c2474a60f
> > > dashboard link: 
> > > https://syzkaller.appspot.com/bug?extid=eb4674092e6cc8d9e0bd
> > > userspace arch: i386
> > >
> > > Unfortunately, I don't have any reproducer for this issue yet.
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the 
> > > commit:
> > > Reported-by: syzbot+eb4674092e6cc8d9e...@syzkaller.appspotmail.com
> > 
> > I suspect that the raw gadget_unbind() can be called while the timer
> > is still active. gadget_unbind() sets gadget data to NULL.
> > But I am not sure which unbind call this is:
> > usb_gadget_remove_driver() or right in udc_bind_to_driver() due to a
> > start error.
> 
> This certainly looks like a race between gadget_unbind and gadget_setup 
> in raw_gadget.
> 
> In theory, this race shouldn't matter.  The gadget core is supposed to 
> guarantee that there won't be any more callbacks to the gadget driver 
> once the driver's unbind routine is called.  That guarantee is enforced 
> in usb_gadget_remove_driver as follows:
> 
>   usb_gadget_disconnect(udc->gadget);
>   if (udc->gadget->irq)
>   synchronize_irq(udc->gadget->irq);
>   udc->driver->unbind(udc->gadget);
>   usb_gadget_udc_stop(udc);
> 
> usb_gadget_disconnect turns off the pullup resistor, telling the host 
> that the gadget is no longer connected and preventing the transmission 
> of any more USB packets.  Any packets that have already been received 
> are sure to processed by the UDC driver's interrupt handler by the time 
> synchronize_irq returns.
> 
> But this doesn't work with dummy_hcd, because dummy_hcd doesn't use 
> interrupts; it uses a timer instead.  It does have code to emulate the 
> effect of synchronize_irq, but that code doesn't get invoked at the 
> right time -- it currently runs in usb_gadget_udc_stop, after the unbind 
> callback instead of before.  Indeed, there's no way for 
> usb_gadget_remove_driver to invoke this code before the unbind 
> callback,.
> 
> I thought the synchronize_irq emulation problem had been completely 
> solved, but evidently it hasn't.  It looks like the best solution is to 
> add a call of the synchronize_irq emulation code in dummy_pullup.
> 
> Maybe we can test this reasoning by putting a delay just before the call 
> to dum->driver->setup.  That runs in the timer handler, so it's not a 
> good place to delay, but it may be okay just for testing purposes.
> 
> Hopefully this patch will make the race a lot more likely to occur.  Is 

Hi Alan, 

Indeed, I was able to reproduce this bug easily on my machine with your
delay patch applied and using this syzkaller program:

syz_usb_connect$cdc_ncm(0x1, 0x6e, &(0x7f40)={{0x12, 0x1, 0x0, 0x2, 
0x0, 0x0, 0x8, 0x525, 0xa4a1, 0x40, 0x1, 0x2, 0x3, 0x1, [{{0x9, 0x2, 0x5c, 0x2, 
0x1, 0x0, 0x0, 0x0, {{0x9, 0x4, 0x0, 0x0, 0x1, 0x2, 0xd, 0x0, 0x0, {{0x5}, 
{0x5}, {0xd}, {0x6}}, {{0x9, 0x5, 0x81, 0x3, 0x200}}]}}, 
&(0x7f000480)={0x0, 0x0, 0x0, 0x0, 0x3, [{0x0, 0x0}, {0x0, 0x0}, {0x0, 
0x0}]})

I also tested doing the synchronize_irq emulation in dummy_pullup and it
fixed the issue. The patch is below.

Thanks!

- Anirudh.

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
b/drivers/usb/gadget/udc/dummy_hcd.c
index ce24d4f28f2a..931d4612d859 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -903,6 +903,12 @@ static int dummy_pullup(struct usb_gadget *_gadget, int 
value)
spin_lock_irqsave(&dum->lock, flags);
dum->pullup = (value != 0);
set_link_state(dum_hcd);
+   /* emulate synchronize_irq(): wait for callbacks to finish */
+   while (dum->callback_usage > 0) {
+   spin_unlock_irqrestore(&dum->lock, flags);
+   usleep_range(1000, 2000);
+   spin_lock_irqsave(&dum->lock, flags);
+   }
spin_unlock_irqrestore(&dum->lock, flags);
 
usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd));
@@ -1005,13 +1011,6 @@ static int dummy_udc_stop(struct usb_gadget *g)
dum->ints_enabled = 0;
stop_activity(dum);
 
-   /* emulate synchronize_irq(): wait for callbacks to finish */
-   while (dum->callback_usage > 0) {
-   spin_unlock_irq(&dum->lock);
-   usleep_range(1000, 2000);
-   spin_lock_irq(&dum->lock);
-   }
-
dum->driver = NULL;
spin_unlock_irq(&dum->lock);
 
@@ -1900,6 +1899,7 @@ static void dummy_timer(struct timer_list *t)

Re: [PATCH v3] firmware_loader: fix use-after-free in firmware_fallback_sysfs

2021-04-14 Thread Anirudh Rayabharam 
On Wed, Apr 14, 2021 at 12:55:40PM +, Luis Chamberlain wrote:
> Shuah, a question for you toward the end here.
> 
> On Wed, Apr 14, 2021 at 02:24:05PM +0530, Anirudh Rayabharam wrote:
> > This use-after-free happens when a fw_priv object has been freed but
> > hasn't been removed from the pending list (pending_fw_head). The next
> > time fw_load_sysfs_fallback tries to insert into the list, it ends up
> > accessing the pending_list member of the previoiusly freed fw_priv.
> > 
> > The root cause here is that all code paths that abort the fw load
> > don't delete it from the pending list. For example:
> > 
> > _request_firmware()
> >   -> fw_abort_batch_reqs()
> >   -> fw_state_aborted()
> > 
> > To fix this, delete the fw_priv from the list in __fw_set_state() if
> > the new state is DONE or ABORTED. This way, all aborts will remove
> > the fw_priv from the list. Accordingly, remove calls to list_del_init
> > that were being made before calling fw_state_(aborted|done).
> > 
> > Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending
> > list if it is already aborted. Instead, just jump out and return early.
> > 
> > Fixes: bcfbd3523f3c ("firmware: fix a double abort case with 
> > fw_load_sysfs_fallback")
> > Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
> > Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam 
> > ---
> > 
> > Changes in v3:
> > Modified the patch to incorporate suggestions by Luis Chamberlain in
> > order to fix the root cause instead of applying a "band-aid" kind of
> > fix.
> > https://lore.kernel.org/lkml/20210403013143.gv4...@42.do-not-panic.com/
> > 
> > Changes in v2:
> > 1. Fixed 1 error and 1 warning (in the commit message) reported by
> > checkpatch.pl. The error was regarding the format for referring to
> > another commit "commit  ("oneline")". The warning was for line
> > longer than 75 chars. 
> > 
> > ---
> >  drivers/base/firmware_loader/fallback.c | 8 ++--
> >  drivers/base/firmware_loader/firmware.h | 6 +-
> >  drivers/base/firmware_loader/main.c | 2 ++
> >  3 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_loader/fallback.c 
> > b/drivers/base/firmware_loader/fallback.c
> > index 91899d185e31..73581b6998b4 100644
> > --- a/drivers/base/firmware_loader/fallback.c
> > +++ b/drivers/base/firmware_loader/fallback.c
> > @@ -94,7 +94,6 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
> > if (fw_sysfs_done(fw_priv))
> > return;
> >  
> > -   list_del_init(&fw_priv->pending_list);
> > fw_state_aborted(fw_priv);
> >  }
> >  
> > @@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct device 
> > *dev,
> >  * Same logic as fw_load_abort, only the DONE bit
> >  * is ignored and we set ABORT only on failure.
> >  */
> > -   list_del_init(&fw_priv->pending_list);
> > if (rc) {
> > fw_state_aborted(fw_priv);
> > written = rc;
> > @@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct fw_sysfs 
> > *fw_sysfs, long timeout)
> > }
> >  
> > mutex_lock(&fw_lock);
> > +   if (fw_state_is_aborted(fw_priv)) {
> > +   mutex_unlock(&fw_lock);
> > +   retval = -EAGAIN;
> > +   goto out;
> > +   }
> 
> Thanks for the quick follow up!
> 
> This would regress commit 76098b36b5db1 ("firmware: send -EINTR on
> signal abort on fallback mechanism") which I had mentioned in my follow
> up email you posted a link to. It would regress it since the condition
> is just being met earlier and you nullify the effort. So essentially
> on Android you would make not being able to detect signal handlers
> like the SIGCHLD signal sent to init, if init was the same process
> dealing with the sysfs fallback firmware upload.

Thanks for the detailed comments, Luis!

I don't see how my patch changes existing error code behaviour. Even
without my patch this function would return -EAGAIN if the fw is already
aborted. Without my patch, it would call fw_sysfs_wait_timeout() which
would return -ENOENT (because fw is already aborted) as follows:

ret = wait_for_completion_killable_timeout(...)
if (ret != 0 && fw_st->status == FW

Re: [PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

2021-04-14 Thread Anirudh Rayabharam 
On Tue, Apr 13, 2021 at 04:51:38PM +, Luis Chamberlain wrote:
> On Tue, Apr 13, 2021 at 04:12:42PM +0530, Anirudh Rayabharam wrote:
> > The use-after-free happens when a fw_priv object has been freed but
> > hasn't been removed from the pending list (pending_fw_head). The next
> > time fw_load_sysfs_fallback tries to insert into the list, it ends up
> > accessing the pending_list member of the previoiusly freed fw_priv.
> > 
> > In commit bcfbd3523f3c ("firmware: fix a double abort case with
> > fw_load_sysfs_fallback"), fw_load_abort() is skipped if
> > fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
> > not be removed from the pending list.
> > 
> > To fix this, delete the fw_priv from the pending list when retval
> > is -ENOENT instead of skipping the entire block.
> > 
> > Fixes: bcfbd3523f3c ("firmware: fix a double abort case with 
> > fw_load_sysfs_fallback")
> > Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
> > Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam 
> 
> Thanks for your patch Anirudh, but please also see this reply to the
> issue:
> 
> http://lkml.kernel.org/r/20210403013143.gv4...@42.do-not-panic.com

I have now sent a v3 that is more along the lines of the patch suggested
in the above thread.

Thanks!

- Anirudh.

[PATCH v3] firmware_loader: fix use-after-free in firmware_fallback_sysfs

2021-04-14 Thread Anirudh Rayabharam 
This use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.

The root cause here is that all code paths that abort the fw load
don't delete it from the pending list. For example:

_request_firmware()
  -> fw_abort_batch_reqs()
  -> fw_state_aborted()

To fix this, delete the fw_priv from the list in __fw_set_state() if
the new state is DONE or ABORTED. This way, all aborts will remove
the fw_priv from the list. Accordingly, remove calls to list_del_init
that were being made before calling fw_state_(aborted|done).

Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending
list if it is already aborted. Instead, just jump out and return early.

Fixes: bcfbd3523f3c ("firmware: fix a double abort case with 
fw_load_sysfs_fallback")
Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam 
---

Changes in v3:
Modified the patch to incorporate suggestions by Luis Chamberlain in
order to fix the root cause instead of applying a "band-aid" kind of
fix.
https://lore.kernel.org/lkml/20210403013143.gv4...@42.do-not-panic.com/

Changes in v2:
1. Fixed 1 error and 1 warning (in the commit message) reported by
checkpatch.pl. The error was regarding the format for referring to
another commit "commit  ("oneline")". The warning was for line
longer than 75 chars. 

---
 drivers/base/firmware_loader/fallback.c | 8 ++--
 drivers/base/firmware_loader/firmware.h | 6 +-
 drivers/base/firmware_loader/main.c | 2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..73581b6998b4 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -94,7 +94,6 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
if (fw_sysfs_done(fw_priv))
return;
 
-   list_del_init(&fw_priv->pending_list);
fw_state_aborted(fw_priv);
 }
 
@@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct device *dev,
 * Same logic as fw_load_abort, only the DONE bit
 * is ignored and we set ABORT only on failure.
 */
-   list_del_init(&fw_priv->pending_list);
if (rc) {
fw_state_aborted(fw_priv);
written = rc;
@@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct fw_sysfs 
*fw_sysfs, long timeout)
}
 
mutex_lock(&fw_lock);
+   if (fw_state_is_aborted(fw_priv)) {
+   mutex_unlock(&fw_lock);
+   retval = -EAGAIN;
+   goto out;
+   }
list_add(&fw_priv->pending_list, &pending_fw_head);
mutex_unlock(&fw_lock);
 
@@ -540,6 +543,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs 
*fw_sysfs, long timeout)
} else if (fw_priv->is_paged_buf && !fw_priv->data)
retval = -ENOMEM;
 
+out:
device_del(f_dev);
 err_put_dev:
put_device(f_dev);
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 63bd29fdcb9c..36bdb413c998 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -117,8 +117,12 @@ static inline void __fw_state_set(struct fw_priv *fw_priv,
 
WRITE_ONCE(fw_st->status, status);
 
-   if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED)
+   if (status == FW_STATUS_DONE || status == FW_STATUS_ABORTED) {
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+   list_del_init(&fw_priv->pending_list);
+#endif
complete_all(&fw_st->completion);
+   }
 }
 
 static inline void fw_state_aborted(struct fw_priv *fw_priv)
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 4fdb8219cd08..68c549d71230 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -783,8 +783,10 @@ static void fw_abort_batch_reqs(struct firmware *fw)
return;
 
fw_priv = fw->priv;
+   mutex_lock(&fw_lock);
if (!fw_state_is_aborted(fw_priv))
fw_state_aborted(fw_priv);
+   mutex_unlock(&fw_lock);
 }
 
 /* called from request_firmware() and request_firmware_work_func() */
-- 
2.26.2

Re: [PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

2021-04-13 Thread Anirudh Rayabharam 
On Tue, Apr 13, 2021 at 04:51:38PM +, Luis Chamberlain wrote:
> On Tue, Apr 13, 2021 at 04:12:42PM +0530, Anirudh Rayabharam wrote:
> > The use-after-free happens when a fw_priv object has been freed but
> > hasn't been removed from the pending list (pending_fw_head). The next
> > time fw_load_sysfs_fallback tries to insert into the list, it ends up
> > accessing the pending_list member of the previoiusly freed fw_priv.
> > 
> > In commit bcfbd3523f3c ("firmware: fix a double abort case with
> > fw_load_sysfs_fallback"), fw_load_abort() is skipped if
> > fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
> > not be removed from the pending list.
> > 
> > To fix this, delete the fw_priv from the pending list when retval
> > is -ENOENT instead of skipping the entire block.
> > 
> > Fixes: bcfbd3523f3c ("firmware: fix a double abort case with 
> > fw_load_sysfs_fallback")
> > Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
> > Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam 
> 
> Thanks for your patch Anirudh, but please also see this reply to the
> issue:
> 
> http://lkml.kernel.org/r/20210403013143.gv4...@42.do-not-panic.com

Hi Luis! Thanks for pointing me to this. I completely forgot to check
the existing discussion on this issue.

> 
> The way you patched the issue is just a band-aid, meaning we keep on
> moving the issue further and it seems that's just the wrong approach.
> 
> Can you try the patch in that thread, to verify if the UAF goes away?

The patch in that thread doesn't work. But I think I know what's
missing. The root problem here is that all code paths that abort fw load
don't remove it from the pending list. For example:

_request_firmware()
 -> fw_abort_batch_reqs()
   -> fw_state_aborted()

In the above code path, the fw_priv is aborted but not removed from
pending list. So, the patch in the above thread fails because the load
is being aborted after it has been added to the list.

So, to fix the root cause of this issue we should make it so that all
aborts remove the fw_priv from the pending list. Perhaps we should add
a list_del_init in __fw_set_state() just before calling complete_all().
This way, all code paths that abort will delete the fw_priv from the
list.

The patch in the above thread also makes some changes to the error
codes. Since it isn't directly related to the UAF, the error codes
change should be a separate patch right?

Thanks!

- Anirudh.

[PATCH v2] firmware_loader: fix use-after-free in firmware_fallback_sysfs

2021-04-13 Thread Anirudh Rayabharam 
The use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.

In commit bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback"), fw_load_abort() is skipped if
fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
not be removed from the pending list.

To fix this, delete the fw_priv from the pending list when retval
is -ENOENT instead of skipping the entire block.

Fixes: bcfbd3523f3c ("firmware: fix a double abort case with 
fw_load_sysfs_fallback")
Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam 
---

Changes in v2:
1. Fixed 1 error and 1 warning (in the commit message) reported by
   checkpatch.pl. The error was regarding the format for referring to
   another commit "commit  ("oneline")". The warning was for line
   longer than 75 chars.

---
 drivers/base/firmware_loader/fallback.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..56ae4ab3199d 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -526,9 +526,14 @@ static int fw_load_sysfs_fallback(struct fw_sysfs 
*fw_sysfs, long timeout)
}
 
retval = fw_sysfs_wait_timeout(fw_priv, timeout);
-   if (retval < 0 && retval != -ENOENT) {
+   if (retval < 0) {
mutex_lock(&fw_lock);
-   fw_load_abort(fw_sysfs);
+
+   if (retval != -ENOENT)
+   fw_load_abort(fw_sysfs);
+   else
+   list_del_init(&fw_priv->pending_list);
+
mutex_unlock(&fw_lock);
}
 
-- 
2.26.2

[PATCH] firmware_loader: fix use-after-free in firmware_fallback_sysfs

2021-04-13 Thread Anirudh Rayabharam 
The use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.

In bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback"), fw_load_abort() is skipped if
fw_sysfs_wait_timeout() returns -ENOENT. This causes the fw_priv to
not be removed from the pending list.

To fix this, delete the fw_priv from the pending list when retval
is -ENOENT instead of skipping the entire block.

Fixes: bcfbd3523f3c ("firmware: fix a double abort case with 
fw_load_sysfs_fallback")
Reported-and-tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam 
---
 drivers/base/firmware_loader/fallback.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..56ae4ab3199d 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -526,9 +526,14 @@ static int fw_load_sysfs_fallback(struct fw_sysfs 
*fw_sysfs, long timeout)
}
 
retval = fw_sysfs_wait_timeout(fw_priv, timeout);
-   if (retval < 0 && retval != -ENOENT) {
+   if (retval < 0) {
mutex_lock(&fw_lock);
-   fw_load_abort(fw_sysfs);
+
+   if (retval != -ENOENT)
+   fw_load_abort(fw_sysfs);
+   else
+   list_del_init(&fw_priv->pending_list);
+
mutex_unlock(&fw_lock);
}
 
-- 
2.26.2

Re: [PATCH] net: hso: fix null-ptr-deref during tty device unregistration

2021-04-07 Thread Anirudh Rayabharam 
On Tue, Apr 06, 2021 at 04:39:21PM -0700, David Miller wrote:
> From: Anirudh Rayabharam 
> Date: Tue,  6 Apr 2021 18:13:59 +0530
> 
> > Multiple ttys try to claim the same the minor number causing a double
> > unregistration of the same device. The first unregistration succeeds
> > but the next one results in a null-ptr-deref.
> > 
> > The get_free_serial_index() function returns an available minor number
> > but doesn't assign it immediately. The assignment is done by the caller
> > later. But before this assignment, calls to get_free_serial_index()
> > would return the same minor number.
> > 
> > Fix this by modifying get_free_serial_index to assign the minor number
> > immediately after one is found to be and rename it to obtain_minor()
> > to better reflect what it does. Similary, rename set_serial_by_index()
> > to release_minor() and modify it to free up the minor number of the
> > given hso_serial. Every obtain_minor() should have corresponding
> > release_minor() call.
> > 
> > Reported-by: syzbot+c49fe6089f295a05e...@syzkaller.appspotmail.com
> > Tested-by: syzbot+c49fe6089f295a05e...@syzkaller.appspotmail.com
> > 
> > Signed-off-by: Anirudh Rayabharam 
> 
> This adds a new build warning:
> 
>   CC [M]  drivers/net/usb/hso.o
> drivers/net/usb/hso.c: In function ‘hso_serial_common_create’:
> drivers/net/usb/hso.c:2256:6: warning: unused variable ‘minor’ 
> [-Wunused-variable]
> 
> Please fix this and add an appropriate Fixes: tag, thank you.

I have sent out a v2 with these changes.

Thanks!

- Anirudh.

[PATCH v2] net: hso: fix null-ptr-deref during tty device unregistration

2021-04-07 Thread Anirudh Rayabharam 
Multiple ttys try to claim the same the minor number causing a double
unregistration of the same device. The first unregistration succeeds
but the next one results in a null-ptr-deref.

The get_free_serial_index() function returns an available minor number
but doesn't assign it immediately. The assignment is done by the caller
later. But before this assignment, calls to get_free_serial_index()
would return the same minor number.

Fix this by modifying get_free_serial_index to assign the minor number
immediately after one is found to be and rename it to obtain_minor()
to better reflect what it does. Similary, rename set_serial_by_index()
to release_minor() and modify it to free up the minor number of the
given hso_serial. Every obtain_minor() should have corresponding
release_minor() call.

Fixes: 72dc1c096c705 ("HSO: add option hso driver")
Reported-by: syzbot+c49fe6089f295a05e...@syzkaller.appspotmail.com
Tested-by: syzbot+c49fe6089f295a05e...@syzkaller.appspotmail.com
Reviewed-by: Greg Kroah-Hartman 
Signed-off-by: Anirudh Rayabharam 
---

Changes in v2:
1. Fixed unused variable warning
2. Added "Fixes:" tag
3. Added the "Reviewed-by:" tags that were receieved

---
 drivers/net/usb/hso.c | 33 -
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 31d51346786a..9bc58e64b5b7 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -611,7 +611,7 @@ static struct hso_serial *get_serial_by_index(unsigned 
index)
return serial;
 }
 
-static int get_free_serial_index(void)
+static int obtain_minor(struct hso_serial *serial)
 {
int index;
unsigned long flags;
@@ -619,8 +619,10 @@ static int get_free_serial_index(void)
spin_lock_irqsave(&serial_table_lock, flags);
for (index = 0; index < HSO_SERIAL_TTY_MINORS; index++) {
if (serial_table[index] == NULL) {
+   serial_table[index] = serial->parent;
+   serial->minor = index;
spin_unlock_irqrestore(&serial_table_lock, flags);
-   return index;
+   return 0;
}
}
spin_unlock_irqrestore(&serial_table_lock, flags);
@@ -629,15 +631,12 @@ static int get_free_serial_index(void)
return -1;
 }
 
-static void set_serial_by_index(unsigned index, struct hso_serial *serial)
+static void release_minor(struct hso_serial *serial)
 {
unsigned long flags;
 
spin_lock_irqsave(&serial_table_lock, flags);
-   if (serial)
-   serial_table[index] = serial->parent;
-   else
-   serial_table[index] = NULL;
+   serial_table[serial->minor] = NULL;
spin_unlock_irqrestore(&serial_table_lock, flags);
 }
 
@@ -2230,6 +2229,7 @@ static int hso_stop_serial_device(struct hso_device 
*hso_dev)
 static void hso_serial_tty_unregister(struct hso_serial *serial)
 {
tty_unregister_device(tty_drv, serial->minor);
+   release_minor(serial);
 }
 
 static void hso_serial_common_free(struct hso_serial *serial)
@@ -2253,24 +2253,22 @@ static void hso_serial_common_free(struct hso_serial 
*serial)
 static int hso_serial_common_create(struct hso_serial *serial, int num_urbs,
int rx_size, int tx_size)
 {
-   int minor;
int i;
 
tty_port_init(&serial->port);
 
-   minor = get_free_serial_index();
-   if (minor < 0)
+   if (obtain_minor(serial))
goto exit2;
 
/* register our minor number */
serial->parent->dev = tty_port_register_device_attr(&serial->port,
-   tty_drv, minor, &serial->parent->interface->dev,
+   tty_drv, serial->minor, &serial->parent->interface->dev,
serial->parent, hso_serial_dev_groups);
-   if (IS_ERR(serial->parent->dev))
+   if (IS_ERR(serial->parent->dev)) {
+   release_minor(serial);
goto exit2;
+   }
 
-   /* fill in specific data for later use */
-   serial->minor = minor;
serial->magic = HSO_SERIAL_MAGIC;
spin_lock_init(&serial->serial_lock);
serial->num_rx_urbs = num_urbs;
@@ -2667,9 +2665,6 @@ static struct hso_device *hso_create_bulk_serial_device(
 
serial->write_data = hso_std_serial_write_data;
 
-   /* and record this serial */
-   set_serial_by_index(serial->minor, serial);
-
/* setup the proc dirs and files if needed */
hso_log_port(hso_dev);
 
@@ -2726,9 +2721,6 @@ struct hso_device *hso_create_mux_serial_device(struct 
usb_interface *interface,
serial->shared_int->ref_count++;
mutex_unlock(&serial->shared_int->shared_int_lock);
 
-   /* and rec

Re: [PATCH] media: pvrusb2: fix warning in pvr2_i2c_core_done

2021-04-07 Thread Anirudh Rayabharam 
On Tue, Apr 06, 2021 at 11:38:25AM +0200, Hans Verkuil wrote:
> On 01/04/2021 14:33, Anirudh Rayabharam wrote:
> > syzbot has reported the following warning in pvr2_i2c_done:
> > 
> > sysfs group 'power' not found for kobject '1-0043'
> > 
> > When the device is disconnected (pvr_hdw_disconnect), the i2c adapter is
> > not unregistered along with the USB and vl42 teardown. As part of the
> 
> vl42 -> v4l2
> 
> > USB device disconnect, the sysfs files of the subdevices are also
> > deleted. So, by the time pvr_i2c_core_done is called by
> > pvr_context_destroy, the sysfs files have been deleted.
> > 
> > To fix this, unregister the i2c adapter too in pvr_hdw_disconnect. Make
> > the device deregistration code shared by calling pvr_hdw_disconnect from
> > pvr2_hdw_destory.
> 
> destory -> destroy
> 
> > 
> > Reported-and-tested-by: 
> > syzbot+e74a998ca8f1df9cc...@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam 
> > ---
> >  drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c 
> > b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > index f4a727918e35..791227787ff5 100644
> > --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > @@ -2676,9 +2676,7 @@ void pvr2_hdw_destroy(struct pvr2_hdw *hdw)
> > pvr2_stream_destroy(hdw->vid_stream);
> > hdw->vid_stream = NULL;
> > }
> > -   pvr2_i2c_core_done(hdw);
> > -   v4l2_device_unregister(&hdw->v4l2_dev);
> 
> I think this should still remain since pvr2_hdw_disconnect() doesn't call
> v4l2_device_unregister().
> 
> Can you test that with syzbot?

Sent v2 with this change. Tested it with syzbot and didn't find any
problems.

Thanks.

- Anirudh.

[PATCH v2] media: pvrusb2: fix warning in pvr2_i2c_core_done

2021-04-07 Thread Anirudh Rayabharam 
syzbot has reported the following warning in pvr2_i2c_done:

sysfs group 'power' not found for kobject '1-0043'

When the device is disconnected (pvr_hdw_disconnect), the i2c adapter is
not unregistered along with the USB and v4l2 teardown. As part of the USB
device disconnect, the sysfs files of the subdevices are also deleted.
So, by the time pvr_i2c_core_done is called by pvr_context_destroy, the
sysfs files have been deleted.

To fix this, unregister the i2c adapter too in pvr_hdw_disconnect. Make
the device deregistration code shared by calling pvr_hdw_disconnect from
pvr2_hdw_destroy.

Reported-by: syzbot+e74a998ca8f1df9cc...@syzkaller.appspotmail.com
Tested-by: syzbot+e74a998ca8f1df9cc...@syzkaller.appspotmail.com

Reviewed-by: Greg Kroah-Hartman 

Signed-off-by: Anirudh Rayabharam 
---

syzbot test run result:
https://groups.google.com/g/syzkaller-bugs/c/ZRtPuAv8k7g/m/_MIsLKJtAAAJ

Changes in v2:
- Corrected typos in the patch description
- Added the received "Reviewed-by:" tags 
- Retain the call to v4l2_device_unregister() in pvr2_hdw_destroy()
  since pvr2_hdw_disconnect doesn't call it as pointed out by Hans.

---
 drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c 
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index f4a727918e35..d38dee1792e4 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -2676,9 +2676,8 @@ void pvr2_hdw_destroy(struct pvr2_hdw *hdw)
pvr2_stream_destroy(hdw->vid_stream);
hdw->vid_stream = NULL;
}
-   pvr2_i2c_core_done(hdw);
v4l2_device_unregister(&hdw->v4l2_dev);
-   pvr2_hdw_remove_usb_stuff(hdw);
+   pvr2_hdw_disconnect(hdw);
mutex_lock(&pvr2_unit_mtx);
do {
if ((hdw->unit_number >= 0) &&
@@ -2705,6 +2704,7 @@ void pvr2_hdw_disconnect(struct pvr2_hdw *hdw)
 {
pvr2_trace(PVR2_TRACE_INIT,"pvr2_hdw_disconnect(hdw=%p)",hdw);
LOCK_TAKE(hdw->big_lock);
+   pvr2_i2c_core_done(hdw);
LOCK_TAKE(hdw->ctl_lock);
pvr2_hdw_remove_usb_stuff(hdw);
LOCK_GIVE(hdw->ctl_lock);
-- 
2.26.2

Re: [PATCH] media: pvrusb2: fix warning in pvr2_i2c_core_done

2021-04-06 Thread Anirudh Rayabharam 
On Tue, Apr 06, 2021 at 11:38:25AM +0200, Hans Verkuil wrote:
> On 01/04/2021 14:33, Anirudh Rayabharam wrote:
> > syzbot has reported the following warning in pvr2_i2c_done:
> > 
> > sysfs group 'power' not found for kobject '1-0043'
> > 
> > When the device is disconnected (pvr_hdw_disconnect), the i2c adapter is
> > not unregistered along with the USB and vl42 teardown. As part of the
> 
> vl42 -> v4l2
> 
> > USB device disconnect, the sysfs files of the subdevices are also
> > deleted. So, by the time pvr_i2c_core_done is called by
> > pvr_context_destroy, the sysfs files have been deleted.
> > 
> > To fix this, unregister the i2c adapter too in pvr_hdw_disconnect. Make
> > the device deregistration code shared by calling pvr_hdw_disconnect from
> > pvr2_hdw_destory.
> 
> destory -> destroy
> 

Ack, will fix these typos in v2.

> > 
> > Reported-and-tested-by: 
> > syzbot+e74a998ca8f1df9cc...@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam 
> > ---
> >  drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c 
> > b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > index f4a727918e35..791227787ff5 100644
> > --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> > @@ -2676,9 +2676,7 @@ void pvr2_hdw_destroy(struct pvr2_hdw *hdw)
> > pvr2_stream_destroy(hdw->vid_stream);
> > hdw->vid_stream = NULL;
> > }
> > -   pvr2_i2c_core_done(hdw);
> > -   v4l2_device_unregister(&hdw->v4l2_dev);
> 
> I think this should still remain since pvr2_hdw_disconnect() doesn't call
> v4l2_device_unregister().

Then we might run into the same warning again. pvr2_hdw_disconnect()
calls pvr2_hdw_remove_usb_stuff() which calls v4l2_device_disconnect().
Perhaps there we should call v4l2_device_unregister() instead?

> 
> Can you test that with syzbot?

Will do.

Thanks!

- Anirudh.

> 
> Regards,
> 
>   Hans
> 
> > -   pvr2_hdw_remove_usb_stuff(hdw);
> > +   pvr2_hdw_disconnect(hdw);
> > mutex_lock(&pvr2_unit_mtx);
> > do {
> > if ((hdw->unit_number >= 0) &&
> > @@ -2705,6 +2703,7 @@ void pvr2_hdw_disconnect(struct pvr2_hdw *hdw)
> >  {
> > pvr2_trace(PVR2_TRACE_INIT,"pvr2_hdw_disconnect(hdw=%p)",hdw);
> > LOCK_TAKE(hdw->big_lock);
> > +   pvr2_i2c_core_done(hdw);
> > LOCK_TAKE(hdw->ctl_lock);
> > pvr2_hdw_remove_usb_stuff(hdw);
> > LOCK_GIVE(hdw->ctl_lock);
> > 
>

[PATCH] net: hso: fix null-ptr-deref during tty device unregistration

2021-04-06 Thread Anirudh Rayabharam 
Multiple ttys try to claim the same the minor number causing a double
unregistration of the same device. The first unregistration succeeds
but the next one results in a null-ptr-deref.

The get_free_serial_index() function returns an available minor number
but doesn't assign it immediately. The assignment is done by the caller
later. But before this assignment, calls to get_free_serial_index()
would return the same minor number.

Fix this by modifying get_free_serial_index to assign the minor number
immediately after one is found to be and rename it to obtain_minor()
to better reflect what it does. Similary, rename set_serial_by_index()
to release_minor() and modify it to free up the minor number of the
given hso_serial. Every obtain_minor() should have corresponding
release_minor() call.

Reported-by: syzbot+c49fe6089f295a05e...@syzkaller.appspotmail.com
Tested-by: syzbot+c49fe6089f295a05e...@syzkaller.appspotmail.com

Signed-off-by: Anirudh Rayabharam 
---
 drivers/net/usb/hso.c | 32 
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 31d51346786a..295ca330e70c 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -611,7 +611,7 @@ static struct hso_serial *get_serial_by_index(unsigned 
index)
return serial;
 }
 
-static int get_free_serial_index(void)
+static int obtain_minor(struct hso_serial *serial)
 {
int index;
unsigned long flags;
@@ -619,8 +619,10 @@ static int get_free_serial_index(void)
spin_lock_irqsave(&serial_table_lock, flags);
for (index = 0; index < HSO_SERIAL_TTY_MINORS; index++) {
if (serial_table[index] == NULL) {
+   serial_table[index] = serial->parent;
+   serial->minor = index;
spin_unlock_irqrestore(&serial_table_lock, flags);
-   return index;
+   return 0;
}
}
spin_unlock_irqrestore(&serial_table_lock, flags);
@@ -629,15 +631,12 @@ static int get_free_serial_index(void)
return -1;
 }
 
-static void set_serial_by_index(unsigned index, struct hso_serial *serial)
+static void release_minor(struct hso_serial *serial)
 {
unsigned long flags;
 
spin_lock_irqsave(&serial_table_lock, flags);
-   if (serial)
-   serial_table[index] = serial->parent;
-   else
-   serial_table[index] = NULL;
+   serial_table[serial->minor] = NULL;
spin_unlock_irqrestore(&serial_table_lock, flags);
 }
 
@@ -2230,6 +2229,7 @@ static int hso_stop_serial_device(struct hso_device 
*hso_dev)
 static void hso_serial_tty_unregister(struct hso_serial *serial)
 {
tty_unregister_device(tty_drv, serial->minor);
+   release_minor(serial);
 }
 
 static void hso_serial_common_free(struct hso_serial *serial)
@@ -2258,19 +2258,18 @@ static int hso_serial_common_create(struct hso_serial 
*serial, int num_urbs,
 
tty_port_init(&serial->port);
 
-   minor = get_free_serial_index();
-   if (minor < 0)
+   if (obtain_minor(serial))
goto exit2;
 
/* register our minor number */
serial->parent->dev = tty_port_register_device_attr(&serial->port,
-   tty_drv, minor, &serial->parent->interface->dev,
+   tty_drv, serial->minor, &serial->parent->interface->dev,
serial->parent, hso_serial_dev_groups);
-   if (IS_ERR(serial->parent->dev))
+   if (IS_ERR(serial->parent->dev)) {
+   release_minor(serial);
goto exit2;
+   }
 
-   /* fill in specific data for later use */
-   serial->minor = minor;
serial->magic = HSO_SERIAL_MAGIC;
spin_lock_init(&serial->serial_lock);
serial->num_rx_urbs = num_urbs;
@@ -2667,9 +2666,6 @@ static struct hso_device *hso_create_bulk_serial_device(
 
serial->write_data = hso_std_serial_write_data;
 
-   /* and record this serial */
-   set_serial_by_index(serial->minor, serial);
-
/* setup the proc dirs and files if needed */
hso_log_port(hso_dev);
 
@@ -2726,9 +2722,6 @@ struct hso_device *hso_create_mux_serial_device(struct 
usb_interface *interface,
serial->shared_int->ref_count++;
mutex_unlock(&serial->shared_int->shared_int_lock);
 
-   /* and record this serial */
-   set_serial_by_index(serial->minor, serial);
-
/* setup the proc dirs and files if needed */
hso_log_port(hso_dev);
 
@@ -3113,7 +3106,6 @@ static void hso_free_interface(struct usb_interface 
*interface)
cancel_work_sync(&serial_table[i]->async_get_intf);
hso_serial_tty_unregister(serial);
kref_put(&serial_table[i]->ref, hso_serial_ref_free);
-   set_serial_by_index(i, NULL);
}
}
 
-- 
2.26.2

[PATCH] media: pvrusb2: fix warning in pvr2_i2c_core_done

2021-04-01 Thread Anirudh Rayabharam 
syzbot has reported the following warning in pvr2_i2c_done:

sysfs group 'power' not found for kobject '1-0043'

When the device is disconnected (pvr_hdw_disconnect), the i2c adapter is
not unregistered along with the USB and vl42 teardown. As part of the
USB device disconnect, the sysfs files of the subdevices are also
deleted. So, by the time pvr_i2c_core_done is called by
pvr_context_destroy, the sysfs files have been deleted.

To fix this, unregister the i2c adapter too in pvr_hdw_disconnect. Make
the device deregistration code shared by calling pvr_hdw_disconnect from
pvr2_hdw_destory.

Reported-and-tested-by: syzbot+e74a998ca8f1df9cc...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam 
---
 drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c 
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index f4a727918e35..791227787ff5 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -2676,9 +2676,7 @@ void pvr2_hdw_destroy(struct pvr2_hdw *hdw)
pvr2_stream_destroy(hdw->vid_stream);
hdw->vid_stream = NULL;
}
-   pvr2_i2c_core_done(hdw);
-   v4l2_device_unregister(&hdw->v4l2_dev);
-   pvr2_hdw_remove_usb_stuff(hdw);
+   pvr2_hdw_disconnect(hdw);
mutex_lock(&pvr2_unit_mtx);
do {
if ((hdw->unit_number >= 0) &&
@@ -2705,6 +2703,7 @@ void pvr2_hdw_disconnect(struct pvr2_hdw *hdw)
 {
pvr2_trace(PVR2_TRACE_INIT,"pvr2_hdw_disconnect(hdw=%p)",hdw);
LOCK_TAKE(hdw->big_lock);
+   pvr2_i2c_core_done(hdw);
LOCK_TAKE(hdw->ctl_lock);
pvr2_hdw_remove_usb_stuff(hdw);
LOCK_GIVE(hdw->ctl_lock);
-- 
2.26.2

[PATCH] ACPI: fix build warning in processor_idle.c

2021-03-31 Thread Anirudh Rayabharam 
GCC shows the following warning during build:

drivers/acpi/processor_idle.c: In function ‘acpi_idle_play_dead’:
drivers/acpi/processor_idle.c:542:15: warning: extra tokens at end
of #ifdef directive

Fix by replacing "ifdef" with "if".

Signed-off-by: Anirudh Rayabharam 
---
 drivers/acpi/processor_idle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 19fb28a8005b..0925b1477230 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -539,7 +539,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, 
int index)
} else
return -ENODEV;
 
-#ifdef defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
+#if defined(CONFIG_X86) && defined(CONFIG_HOTPLUG_CPU)
/* If NMI wants to wake up CPU0, start CPU0. */
if (wakeup_cpu0())
start_cpu0();
-- 
2.26.2

[PATCH resend] jfs: fix use-after-free in lbmIODone

2021-03-22 Thread Anirudh Rayabharam 
Fix use-after-free by waiting for ongoing IO to complete before freeing
lbufs in lbmLogShutdown. Add a counter in struct jfs_log to keep track
of the number of in-flight IO operations and a wait queue to wait on for
the IO operations to complete.

Reported-by: syzbot+5d2008bd1f1b722ba...@syzkaller.appspotmail.com
Suggested-by: Hillf Danton 
Signed-off-by: Anirudh Rayabharam 
---
 fs/jfs/jfs_logmgr.c | 17 ++---
 fs/jfs/jfs_logmgr.h |  2 ++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 9330eff210e0..82d20c4687aa 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -1815,6 +1815,8 @@ static int lbmLogInit(struct jfs_log * log)
 */
init_waitqueue_head(&log->free_wait);
 
+   init_waitqueue_head(&log->io_waitq);
+
log->lbuf_free = NULL;
 
for (i = 0; i < LOGPAGES;) {
@@ -1864,6 +1866,7 @@ static void lbmLogShutdown(struct jfs_log * log)
struct lbuf *lbuf;
 
jfs_info("lbmLogShutdown: log:0x%p", log);
+   wait_event(log->io_waitq, !atomic_read(&log->io_inflight));
 
lbuf = log->lbuf_free;
while (lbuf) {
@@ -1990,6 +1993,8 @@ static int lbmRead(struct jfs_log * log, int pn, struct 
lbuf ** bpp)
bio->bi_end_io = lbmIODone;
bio->bi_private = bp;
bio->bi_opf = REQ_OP_READ;
+
+   atomic_inc(&log->io_inflight);
/*check if journaling to disk has been disabled*/
if (log->no_integrity) {
bio->bi_iter.bi_size = 0;
@@ -2135,6 +2140,7 @@ static void lbmStartIO(struct lbuf * bp)
bio->bi_private = bp;
bio->bi_opf = REQ_OP_WRITE | REQ_SYNC;
 
+   atomic_inc(&log->io_inflight);
/* check if journaling to disk has been disabled */
if (log->no_integrity) {
bio->bi_iter.bi_size = 0;
@@ -2200,6 +2206,8 @@ static void lbmIODone(struct bio *bio)
 
bio_put(bio);
 
+   log = bp->l_log;
+
/*
 *  pagein completion
 */
@@ -2211,7 +2219,7 @@ static void lbmIODone(struct bio *bio)
/* wakeup I/O initiator */
LCACHE_WAKEUP(&bp->l_ioevent);
 
-   return;
+   goto out;
}
 
/*
@@ -2230,13 +2238,12 @@ static void lbmIODone(struct bio *bio)
INCREMENT(lmStat.pagedone);
 
/* update committed lsn */
-   log = bp->l_log;
log->clsn = (bp->l_pn << L2LOGPSIZE) + bp->l_ceor;
 
if (bp->l_flag & lbmDIRECT) {
LCACHE_WAKEUP(&bp->l_ioevent);
LCACHE_UNLOCK(flags);
-   return;
+   goto out;
}
 
tail = log->wqueue;
@@ -2315,6 +2322,10 @@ static void lbmIODone(struct bio *bio)
 
LCACHE_UNLOCK(flags);   /* unlock+enable */
}
+
+out:
+   if (atomic_dec_and_test(&log->io_inflight))
+   wake_up(&log->io_waitq);
 }
 
 int jfsIOWait(void *arg)
diff --git a/fs/jfs/jfs_logmgr.h b/fs/jfs/jfs_logmgr.h
index 805877ce5020..3e92fe251f28 100644
--- a/fs/jfs/jfs_logmgr.h
+++ b/fs/jfs/jfs_logmgr.h
@@ -400,6 +400,8 @@ struct jfs_log {
uuid_t uuid;/* 16: 128-bit uuid of log device */
 
int no_integrity;   /* 3: flag to disable journaling to disk */
+   atomic_t io_inflight;
+   wait_queue_head_t io_waitq;
 };
 
 /*
-- 
2.26.2

[PATCH] jfs: fix use-after-free in lbmIODone

2021-03-15 Thread Anirudh Rayabharam 
Fix use-after-free by waiting for ongoing IO to complete before freeing
lbufs in lbmLogShutdown. Add a counter in struct jfs_log to keep track
of the number of in-flight IO operations and a wait queue to wait on for
the IO operations to complete.

Reported-by: syzbot+5d2008bd1f1b722ba...@syzkaller.appspotmail.com
Suggested-by: Hillf Danton 
Signed-off-by: Anirudh Rayabharam 
---
 fs/jfs/jfs_logmgr.c | 17 ++---
 fs/jfs/jfs_logmgr.h |  2 ++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 9330eff210e0..82d20c4687aa 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -1815,6 +1815,8 @@ static int lbmLogInit(struct jfs_log * log)
 */
init_waitqueue_head(&log->free_wait);
 
+   init_waitqueue_head(&log->io_waitq);
+
log->lbuf_free = NULL;
 
for (i = 0; i < LOGPAGES;) {
@@ -1864,6 +1866,7 @@ static void lbmLogShutdown(struct jfs_log * log)
struct lbuf *lbuf;
 
jfs_info("lbmLogShutdown: log:0x%p", log);
+   wait_event(log->io_waitq, !atomic_read(&log->io_inflight));
 
lbuf = log->lbuf_free;
while (lbuf) {
@@ -1990,6 +1993,8 @@ static int lbmRead(struct jfs_log * log, int pn, struct 
lbuf ** bpp)
bio->bi_end_io = lbmIODone;
bio->bi_private = bp;
bio->bi_opf = REQ_OP_READ;
+
+   atomic_inc(&log->io_inflight);
/*check if journaling to disk has been disabled*/
if (log->no_integrity) {
bio->bi_iter.bi_size = 0;
@@ -2135,6 +2140,7 @@ static void lbmStartIO(struct lbuf * bp)
bio->bi_private = bp;
bio->bi_opf = REQ_OP_WRITE | REQ_SYNC;
 
+   atomic_inc(&log->io_inflight);
/* check if journaling to disk has been disabled */
if (log->no_integrity) {
bio->bi_iter.bi_size = 0;
@@ -2200,6 +2206,8 @@ static void lbmIODone(struct bio *bio)
 
bio_put(bio);
 
+   log = bp->l_log;
+
/*
 *  pagein completion
 */
@@ -2211,7 +2219,7 @@ static void lbmIODone(struct bio *bio)
/* wakeup I/O initiator */
LCACHE_WAKEUP(&bp->l_ioevent);
 
-   return;
+   goto out;
}
 
/*
@@ -2230,13 +2238,12 @@ static void lbmIODone(struct bio *bio)
INCREMENT(lmStat.pagedone);
 
/* update committed lsn */
-   log = bp->l_log;
log->clsn = (bp->l_pn << L2LOGPSIZE) + bp->l_ceor;
 
if (bp->l_flag & lbmDIRECT) {
LCACHE_WAKEUP(&bp->l_ioevent);
LCACHE_UNLOCK(flags);
-   return;
+   goto out;
}
 
tail = log->wqueue;
@@ -2315,6 +2322,10 @@ static void lbmIODone(struct bio *bio)
 
LCACHE_UNLOCK(flags);   /* unlock+enable */
}
+
+out:
+   if (atomic_dec_and_test(&log->io_inflight))
+   wake_up(&log->io_waitq);
 }
 
 int jfsIOWait(void *arg)
diff --git a/fs/jfs/jfs_logmgr.h b/fs/jfs/jfs_logmgr.h
index 805877ce5020..3e92fe251f28 100644
--- a/fs/jfs/jfs_logmgr.h
+++ b/fs/jfs/jfs_logmgr.h
@@ -400,6 +400,8 @@ struct jfs_log {
uuid_t uuid;/* 16: 128-bit uuid of log device */
 
int no_integrity;   /* 3: flag to disable journaling to disk */
+   atomic_t io_inflight;
+   wait_queue_head_t io_waitq;
 };
 
 /*
-- 
2.26.2

Re: [PATCH] staging: wimax/i2400m: fix some byte order issues found by sparse

2021-02-12 Thread Anirudh Rayabharam 
On Fri, Feb 12, 2021 at 03:43:10PM +0100, Greg KH wrote:
> On Fri, Feb 12, 2021 at 08:00:25PM +0530, Anirudh Rayabharam wrote:
> > On Thu, Feb 11, 2021 at 09:35:27PM +0100, Greg KH wrote:
> > > On Fri, Feb 12, 2021 at 01:59:08AM +0530, Anirudh Rayabharam wrote:
> > > > Fix sparse byte-order warnings in the i2400m_bm_cmd_prepare()
> > > > function:
> > > > 
> > > > wimax/i2400m/fw.c:194:36: warning: restricted __le32 degrades to integer
> > > > wimax/i2400m/fw.c:195:34: warning: invalid assignment: +=
> > > > wimax/i2400m/fw.c:195:34:left side has type unsigned int
> > > > wimax/i2400m/fw.c:195:34:right side has type restricted __le32
> > > > wimax/i2400m/fw.c:196:32: warning: restricted __le32 degrades to integer
> > > > wimax/i2400m/fw.c:196:47: warning: restricted __le32 degrades to integer
> > > > wimax/i2400m/fw.c:196:66: warning: restricted __le32 degrades to integer
> > > > 
> > > > Signed-off-by: Anirudh Rayabharam 
> > > > ---
> > > >  drivers/staging/wimax/i2400m/fw.c | 14 +-
> > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/wimax/i2400m/fw.c 
> > > > b/drivers/staging/wimax/i2400m/fw.c
> > > > index b2fd4bd2c5f9..bce651a6b543 100644
> > > > --- a/drivers/staging/wimax/i2400m/fw.c
> > > > +++ b/drivers/staging/wimax/i2400m/fw.c
> > > > @@ -189,12 +189,16 @@ void i2400m_bm_cmd_prepare(struct 
> > > > i2400m_bootrom_header *cmd)
> > > >  {
> > > > if (i2400m_brh_get_use_checksum(cmd)) {
> > > > int i;
> > > > -   u32 checksum = 0;
> > > > +   __le32 checksum = 0;
> > > 
> > > __le32 is only for when the data crosses the kernel/user boundry, just
> > > use le32 in the kernel for stuff like this.
> > > 
> > But that throws a compile error.
> 
> What error?

drivers/staging/wimax/i2400m/fw.c:192:3: error: unknown type name
‘le32’; did you mean ‘__le32’?

- Anirudh

[PATCH v2] staging: wimax/i2400m: fix some byte order issues found by sparse

2021-02-12 Thread Anirudh Rayabharam 
Fix sparse byte-order warnings in the i2400m_bm_cmd_prepare()
function:

wimax/i2400m/fw.c:194:36: warning: restricted __le32 degrades to integer
wimax/i2400m/fw.c:195:34: warning: invalid assignment: +=
wimax/i2400m/fw.c:195:34:left side has type unsigned int
wimax/i2400m/fw.c:195:34:right side has type restricted __le32
wimax/i2400m/fw.c:196:32: warning: restricted __le32 degrades to integer
wimax/i2400m/fw.c:196:47: warning: restricted __le32 degrades to integer
wimax/i2400m/fw.c:196:66: warning: restricted __le32 degrades to integer

Signed-off-by: Anirudh Rayabharam 
---
 drivers/staging/wimax/i2400m/fw.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/wimax/i2400m/fw.c 
b/drivers/staging/wimax/i2400m/fw.c
index b2fd4bd2c5f9..92ea5c101e76 100644
--- a/drivers/staging/wimax/i2400m/fw.c
+++ b/drivers/staging/wimax/i2400m/fw.c
@@ -189,12 +189,17 @@ void i2400m_bm_cmd_prepare(struct i2400m_bootrom_header 
*cmd)
 {
if (i2400m_brh_get_use_checksum(cmd)) {
int i;
-   u32 checksum = 0;
+   __le32 checksum = 0;
const u32 *checksum_ptr = (void *) cmd->payload;
-   for (i = 0; i < cmd->data_size / 4; i++)
-   checksum += cpu_to_le32(*checksum_ptr++);
-   checksum += cmd->command + cmd->target_addr + cmd->data_size;
-   cmd->block_checksum = cpu_to_le32(checksum);
+
+   for (i = 0; i < le32_to_cpu(cmd->data_size) / 4; i++)
+   le32_add_cpu(&checksum, *checksum_ptr++);
+
+   le32_add_cpu(&checksum, le32_to_cpu(cmd->command));
+   le32_add_cpu(&checksum, le32_to_cpu(cmd->target_addr));
+   le32_add_cpu(&checksum, le32_to_cpu(cmd->data_size));
+
+   cmd->block_checksum = checksum;
}
 }
 EXPORT_SYMBOL_GPL(i2400m_bm_cmd_prepare);
-- 
2.26.2

Re: [PATCH] staging: wimax/i2400m: fix some byte order issues found by sparse

2021-02-12 Thread Anirudh Rayabharam 
On Thu, Feb 11, 2021 at 09:35:27PM +0100, Greg KH wrote:
> On Fri, Feb 12, 2021 at 01:59:08AM +0530, Anirudh Rayabharam wrote:
> > Fix sparse byte-order warnings in the i2400m_bm_cmd_prepare()
> > function:
> > 
> > wimax/i2400m/fw.c:194:36: warning: restricted __le32 degrades to integer
> > wimax/i2400m/fw.c:195:34: warning: invalid assignment: +=
> > wimax/i2400m/fw.c:195:34:left side has type unsigned int
> > wimax/i2400m/fw.c:195:34:right side has type restricted __le32
> > wimax/i2400m/fw.c:196:32: warning: restricted __le32 degrades to integer
> > wimax/i2400m/fw.c:196:47: warning: restricted __le32 degrades to integer
> > wimax/i2400m/fw.c:196:66: warning: restricted __le32 degrades to integer
> > 
> > Signed-off-by: Anirudh Rayabharam 
> > ---
> >  drivers/staging/wimax/i2400m/fw.c | 14 +-
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/wimax/i2400m/fw.c 
> > b/drivers/staging/wimax/i2400m/fw.c
> > index b2fd4bd2c5f9..bce651a6b543 100644
> > --- a/drivers/staging/wimax/i2400m/fw.c
> > +++ b/drivers/staging/wimax/i2400m/fw.c
> > @@ -189,12 +189,16 @@ void i2400m_bm_cmd_prepare(struct 
> > i2400m_bootrom_header *cmd)
> >  {
> > if (i2400m_brh_get_use_checksum(cmd)) {
> > int i;
> > -   u32 checksum = 0;
> > +   __le32 checksum = 0;
> 
> __le32 is only for when the data crosses the kernel/user boundry, just
> use le32 in the kernel for stuff like this.
> 
But that throws a compile error. Also, I don't see le32 defined
in any common header. It is defined in fs/ntfs/types.h but that's not
accessible here.

> > const u32 *checksum_ptr = (void *) cmd->payload;
> 
> Add a blank line here, right?
It wasn't there before but makes sense. I'll send v2 with this change.

Thanks!

- Anirudh.

[PATCH] staging: wimax/i2400m: fix some byte order issues found by sparse

2021-02-11 Thread Anirudh Rayabharam 
Fix sparse byte-order warnings in the i2400m_bm_cmd_prepare()
function:

wimax/i2400m/fw.c:194:36: warning: restricted __le32 degrades to integer
wimax/i2400m/fw.c:195:34: warning: invalid assignment: +=
wimax/i2400m/fw.c:195:34:left side has type unsigned int
wimax/i2400m/fw.c:195:34:right side has type restricted __le32
wimax/i2400m/fw.c:196:32: warning: restricted __le32 degrades to integer
wimax/i2400m/fw.c:196:47: warning: restricted __le32 degrades to integer
wimax/i2400m/fw.c:196:66: warning: restricted __le32 degrades to integer

Signed-off-by: Anirudh Rayabharam 
---
 drivers/staging/wimax/i2400m/fw.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/wimax/i2400m/fw.c 
b/drivers/staging/wimax/i2400m/fw.c
index b2fd4bd2c5f9..bce651a6b543 100644
--- a/drivers/staging/wimax/i2400m/fw.c
+++ b/drivers/staging/wimax/i2400m/fw.c
@@ -189,12 +189,16 @@ void i2400m_bm_cmd_prepare(struct i2400m_bootrom_header 
*cmd)
 {
if (i2400m_brh_get_use_checksum(cmd)) {
int i;
-   u32 checksum = 0;
+   __le32 checksum = 0;
const u32 *checksum_ptr = (void *) cmd->payload;
-   for (i = 0; i < cmd->data_size / 4; i++)
-   checksum += cpu_to_le32(*checksum_ptr++);
-   checksum += cmd->command + cmd->target_addr + cmd->data_size;
-   cmd->block_checksum = cpu_to_le32(checksum);
+   for (i = 0; i < le32_to_cpu(cmd->data_size) / 4; i++)
+   le32_add_cpu(&checksum, *checksum_ptr++);
+
+   le32_add_cpu(&checksum, le32_to_cpu(cmd->command));
+   le32_add_cpu(&checksum, le32_to_cpu(cmd->target_addr));
+   le32_add_cpu(&checksum, le32_to_cpu(cmd->data_size));
+
+   cmd->block_checksum = checksum;
}
 }
 EXPORT_SYMBOL_GPL(i2400m_bm_cmd_prepare);
-- 
2.26.2

Re: [PATCH v2] staging: rtl8723bs: core: fix line over 80 characters warning

2019-03-30 Thread Anirudh Rayabharam 
On Wed, Mar 27, 2019 at 11:49:07PM +0530, Anirudh Rayabharam wrote:
> Checkpatch.pl complains that these lines are over 80 characters. Use the
> "psecuritypriv" pointer for consistency, remove unnecessary parantheses
> and fix the alignment.
> 
> This patch just cleans up a condition, it doesn't affect runtime.
> 
> Signed-off-by: Anirudh Rayabharam 
> ---
> v2: Made the commit message clearer, removed unnecessary parantheses and fixed
> the alignment as suggested by Dan Carpenter 
> 
>  drivers/staging/rtl8723bs/core/rtw_ap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c 
> b/drivers/staging/rtl8723bs/core/rtw_ap.c
> index 18fabf5ff44b..8062b7f36de2 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
> @@ -2336,8 +2336,8 @@ void rtw_ap_restore_network(struct adapter *padapter)
>   Update_RA_Entry(padapter, psta);
>   /* pairwise key */
>   /* per sta pairwise key and settings */
> - if ((padapter->securitypriv.dot11PrivacyAlgrthm == 
> _TKIP_) ||
> - (padapter->securitypriv.dot11PrivacyAlgrthm == 
> _AES_)) {
> + if (psecuritypriv->dot11PrivacyAlgrthm == _TKIP_ ||
> + psecuritypriv->dot11PrivacyAlgrthm == _AES_) {
>   rtw_setstakey_cmd(padapter, psta, true, false);
>   }
>   }
> -- 
> 2.17.1
> 

Unfortunately, the v2 of this patch was missed and v1 was applied to Greg's
staging tree. So, I am abandoning v2 of this patch. I will fix the alignment
and braces issue in a new patch.

[PATCH v2] staging: rtl8723bs: core: fix line over 80 characters warning

2019-03-27 Thread Anirudh Rayabharam 
Checkpatch.pl complains that these lines are over 80 characters. Use the
"psecuritypriv" pointer for consistency, remove unnecessary parantheses
and fix the alignment.

This patch just cleans up a condition, it doesn't affect runtime.

Signed-off-by: Anirudh Rayabharam 
---
v2: Made the commit message clearer, removed unnecessary parantheses and fixed
the alignment as suggested by Dan Carpenter 

 drivers/staging/rtl8723bs/core/rtw_ap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c 
b/drivers/staging/rtl8723bs/core/rtw_ap.c
index 18fabf5ff44b..8062b7f36de2 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ap.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
@@ -2336,8 +2336,8 @@ void rtw_ap_restore_network(struct adapter *padapter)
Update_RA_Entry(padapter, psta);
/* pairwise key */
/* per sta pairwise key and settings */
-   if ((padapter->securitypriv.dot11PrivacyAlgrthm == 
_TKIP_) ||
-   (padapter->securitypriv.dot11PrivacyAlgrthm == 
_AES_)) {
+   if (psecuritypriv->dot11PrivacyAlgrthm == _TKIP_ ||
+   psecuritypriv->dot11PrivacyAlgrthm == _AES_) {
rtw_setstakey_cmd(padapter, psta, true, false);
}
}
-- 
2.17.1

[PATCH] staging: rtl8723bs: core: fix line over 80 characters warning

2019-03-26 Thread Anirudh Rayabharam 
Shorten the expression by re-using the part that was already computed to
fix the line over 80 characters warning reported by checkpatch.pl.

Signed-off-by: Anirudh Rayabharam 
---
 drivers/staging/rtl8723bs/core/rtw_ap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c 
b/drivers/staging/rtl8723bs/core/rtw_ap.c
index 18fabf5ff44b..bc0230672457 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ap.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
@@ -2336,8 +2336,8 @@ void rtw_ap_restore_network(struct adapter *padapter)
Update_RA_Entry(padapter, psta);
/* pairwise key */
/* per sta pairwise key and settings */
-   if ((padapter->securitypriv.dot11PrivacyAlgrthm == 
_TKIP_) ||
-   (padapter->securitypriv.dot11PrivacyAlgrthm == 
_AES_)) {
+   if ((psecuritypriv->dot11PrivacyAlgrthm == _TKIP_) ||
+   (psecuritypriv->dot11PrivacyAlgrthm == _AES_)) {
rtw_setstakey_cmd(padapter, psta, true, false);
}
}
-- 
2.17.1