linux-kernel - HIDDEV: potential NULL dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 04 Sep 2010 14:54:52 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	cjb@...top.org
CC:	Jiri Kosina <jkosina@...e.cz>, linux-input@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>
Subject: HIDDEV: potential NULL dereference

Hi,

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

commit 7032269e87ade34cc12891675371fa2ac150a620
Author: Chris Ball <cjb@...top.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@...top.org>
    Signed-off-by: Jiri Kosina <jkosina@...e.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/