HIDDEV: potential NULL dereference

HIDDEV: potential NULL dereference

[Jiri Slaby]

Hi,

I've a question for you :). How could this patch help:

commit 7032269e87ade34cc12891675371fa2ac150a620
Author: Chris Ball <cjb@laptop.org>
Date:   Thu Aug 12 19:07:40 2010 -0400

    HID: hiddev: protect against disconnect/NULL-dereference race

    One of our users reports consistently hitting a NULL dereference that
    resolves to the "hid_to_usb_dev(hid);" call in hiddev_ioctl(), when
    disconnecting a Lego WeDo USB HID device from an OLPC XO running
    Scratch software.  There's a FIXME comment and a guard against the
    dereference, but that happens farther down the function than the
    initial dereference does.

    This patch moves the call to be below the guard, and the user reports
    that it fixes the problem for him.  OLPC bug report:
    http://dev.laptop.org/ticket/10174

    Signed-off-by: Chris Ball <cjb@laptop.org>
    Signed-off-by: Jiri Kosina <jkosina@suse.cz>






when the code now looks like:
  struct usb_device *dev; // here was the assignment before the patch
  struct usbhid_device *usbhid = hid->driver_data;
...
  if (!hiddev->exist || !hid)
    return -EIO;
  dev = hid_to_usb_dev(hid);


If hid was ever NULL at this phase, the check couldn't improve anything
due to hid->driver_data dereference being still before the check. So
again my question, how this could change anything?

Above that, it just makes the window shorter, but the bug is still
there, isn't it? Is the following scenario reasonable?

A (hiddev_ioctl)           | B
------------------------------------------------
if (!hid)                  |
  return -EIO;             |
                           | hid = NULL
                           | kfree(hid);
dev = hid_to_usb_dev(hid); |


Actually who's the process B you are trying to catch the race against by
the patch? hid-core?

thanks,
-- 
js
suse labs

Re: HIDDEV: potential NULL dereference

[Chris Ball]

Hi Jiri,

   > If hid was ever NULL at this phase, the check couldn't improve
   > anything due to hid->driver_data dereference being still before
   > the check. So again my question, how this could change anything?
   > 
   > Above that, it just makes the window shorter, but the bug is
   > still there, isn't it? Is the following scenario reasonable?

You're right -- I'd missed the other dereference, sorry.  It's
surprising that we have two reports from users saying that the
patch got rid of a reproducible oops for them.

Dan Carpenter has commented on this too:
   http://www.spinics.net/lists/linux-input/msg10541.html

As he says, it looks like the code's overdue for some real locking.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>
One Laptop Per Child

Re: HIDDEV: potential NULL dereference

[Amit Nagal]

Hi ,

i was one of the users who informed that crash do disappears .

the reality with me is that the probability of occurence of crash
reduces significantly

but on rigourous testing crash resurfaces again .

sorry for wrong reporting .

Regards
Amit Nagal



On Sat, Sep 4, 2010 at 10:09 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi Jiri,
>
>   > If hid was ever NULL at this phase, the check couldn't improve
>   > anything due to hid->driver_data dereference being still before
>   > the check. So again my question, how this could change anything?
>   >
>   > Above that, it just makes the window shorter, but the bug is
>   > still there, isn't it? Is the following scenario reasonable?
>
> You're right -- I'd missed the other dereference, sorry.  It's
> surprising that we have two reports from users saying that the
> patch got rid of a reproducible oops for them.
>
> Dan Carpenter has commented on this too:
>   http://www.spinics.net/lists/linux-input/msg10541.html
>
> As he says, it looks like the code's overdue for some real locking.
>
> Thanks,
>
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: HIDDEV: potential NULL dereference

[Amit Nagal]

Hi ,

i was one of the users who informed that crash do disappears .

the reality with me is that the probability of occurence of crash
reduces significantly

but on rigourous testing crash resurfaces again .

sorry for wrong reporting .

Regards
Amit Nagal



On Sat, Sep 4, 2010 at 10:09 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi Jiri,
>
>   > If hid was ever NULL at this phase, the check couldn't improve
>   > anything due to hid->driver_data dereference being still before
>   > the check. So again my question, how this could change anything?
>   >
>   > Above that, it just makes the window shorter, but the bug is
>   > still there, isn't it? Is the following scenario reasonable?
>
> You're right -- I'd missed the other dereference, sorry.  It's
> surprising that we have two reports from users saying that the
> patch got rid of a reproducible oops for them.
>
> Dan Carpenter has commented on this too:
>   http://www.spinics.net/lists/linux-input/msg10541.html
>
> As he says, it looks like the code's overdue for some real locking.
>
> Thanks,
>
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: HIDDEV: potential NULL dereference

[Jiri Kosina]

On Thu, 9 Sep 2010, anil joshi wrote:

> 
> 
> On Thu, Sep 9, 2010 at 7:59 PM, anil joshi <anilsjoshi123@gmail.com> wrote:
>        hid->dev.parent is "usb_interfae"  pointer is kfree  in  usb_release_interfece 
> hid_to_usb_dev(dev) accessing hid->dev.parent 
> 
> I have tested this,while removing  hid device  
> 
> usb_release_interfece is belonging to khubd  process 
> 
> I get many use full information  from  Jiri Kosina, Markus Trippelsdorf, regarding this issue
> from http://kerneltrap.org/mailarchive/linux-kernel/2010/8/12/4606155/thread#mid-4606155
> 
> To avoid  NULL deference problem 
> 
> hiddev->exist is zero before usb_deregister_dev  call in  hiddev_disconnected  function there fore
> we could use
> static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> 
> 
>      if (hiddev->exist) {
>          hid_to_usb_dev(dev);
>        } else {
>             return -EIO
>    }
> 
> 
> Thanks to Markus Trippelsdorf , Jiri Kosina 

Hi Anil,

could you please send a proper patch (diff against current kernel) and a 
short changelog entry with the explanation, so that I could easily 
understand what you are proposing?

Thanks!

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: HIDDEV: potential NULL dereference

[Jiri Kosina]

On Thu, 9 Sep 2010, anil joshi wrote:

> 
> 
> On Thu, Sep 9, 2010 at 7:59 PM, anil joshi <anilsjoshi123@gmail.com> wrote:
>        hid->dev.parent is "usb_interfae"  pointer is kfree  in  usb_release_interfece 
> hid_to_usb_dev(dev) accessing hid->dev.parent 
> 
> I have tested this,while removing  hid device  
> 
> usb_release_interfece is belonging to khubd  process 
> 
> I get many use full information  from  Jiri Kosina, Markus Trippelsdorf, regarding this issue
> from http://kerneltrap.org/mailarchive/linux-kernel/2010/8/12/4606155/thread#mid-4606155
> 
> To avoid  NULL deference problem 
> 
> hiddev->exist is zero before usb_deregister_dev  call in  hiddev_disconnected  function there fore
> we could use
> static long hiddev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> 
> 
>      if (hiddev->exist) {
>          hid_to_usb_dev(dev);
>        } else {
>             return -EIO
>    }
> 
> 
> Thanks to Markus Trippelsdorf , Jiri Kosina 

Hi Anil,

could you please send a proper patch (diff against current kernel) and a 
short changelog entry with the explanation, so that I could easily 
understand what you are proposing?

Thanks!

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html