On Tue, 28 Sep 2010, Antonio Ospite wrote:
> Hi Alan, I am doing some stress testing on hidraw, if I have a loop with
> HIDIOCGFEATURE on a given report and I disconnect the device while the
> loop is running I get this:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
> IP: [<ffffffffa02c66b4>] hidraw_ioctl+0xfc/0x32c [hid]
>
> Full log attached along with the test program, the device is a Sony PS3
> Controller (sixaxis).
>
> If my objdump analysis is right, hidraw_ioctl+0xfc should be around line
> 361 in hidraw.c (with your patch applied):
>
> struct hid_device *hid = dev->hid;
>
> It looks like 'dev' (which is hidraw_table[minor]) can be NULL
> sometimes, can't it?
> This is not introduced by your changes tho.
>
> Just as a side note, the bug does not show up if the userspace program
> handles return values properly and exits as soon as it gets an error
> from the HID layer, see the XXX comment in test_hidraw_feature.c.
>
> This fixes it, if it looks ok I will resend the patch rebased on
> mainline code:
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 7df1310..3c040c6 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -322,6 +322,10 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
>
> mutex_lock(&minors_lock);
> dev = hidraw_table[minor];
> + if (!dev) {
> + ret = -ENODEV;
> + goto out;
> + }
>
> switch (cmd) {
> case HIDIOCGRDESCSIZE:
> @@ -412,6 +416,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
>
> ret = -ENOTTY;
> }
> +out:
> mutex_unlock(&minors_lock);
> return ret;
> }
Yes, this patch makes sense even for current mainline code. Could you
please resend it to me with Signed-off-by: and changelog text, so that I
could apply it?
Thanks!
--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
