On Thu, 14 Jul 2011, David Herrmann wrote:
> On Wed, Jul 13, 2011 at 4:24 PM, David Herrmann
> <dh.herrmann@xxxxxxxxxxxxxx> wrote:
> > HID low level drivers register new devices with the HID core which then
> > adds the devices to the HID bus. The HID bus normally immediately probes
> > an appropriate driver which then handles HID input for this device.
> > The ll driver now uses the hid_input_report() function to report input
> > events for a specific device. However, if the HID bus unloads the driver
> > at the same time (for instance via a call to
> > /sys/bus/hid/devices/<dev>/unbind) then the hdev->driver pointer may be
> > used by hid_input_report() and hid_device_remove() at the same time
> > which may cause hdev->driver to point to invalid memory.
> >
> > This fix adds a semaphore to every hid device which protects
> > hdev->driver from asynchronous access. This semaphore is locked during
> > driver *_probe and *_remove and also inside hid_input_report(). The
> > *_probe and *_remove functions may sleep so the semaphore is good here,
> > however, hid_input_report() is in atomic context and hence only uses
> > down_trylock(). If it cannot acquire the lock it simply drops the input
> > package.
> >
> > The low-level drivers report input events synchronously so
> > hid_input_report() should never be entered twice at the same time on the
> > same device. Hence, the lock should always be available. But if the
> > driver is currently probed/removed then the lock is not available and
> > dropping the package should be safe because this is what would have
> > happened if the package arrived some milliseconds earlier/later.
> >
> > This also fixes another race condition while probing drivers:
> > First the *_probe function of the driver is called and only if that
> > succeeds, the related input device of hidinput is registered. If the low
> > level driver reports input events after the *_probe function returned
> > but before the input device is registered, then a NULL pointer
> > dereference will occur. (Equivalently on driver remove function).
> > This is not possible anymore, since the semaphore lock drops all
> > incoming packages until the driver/device is fully initialized.
> >
> > Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxxxxxxx>
> > ---
> > drivers/hid/hid-core.c | 41 ++++++++++++++++++++++++++++++++++-------
> > include/linux/hid.h | 2 ++
> > 2 files changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 2a268fc..59b1a5b 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -29,6 +29,7 @@
> > #include <linux/wait.h>
> > #include <linux/vmalloc.h>
> > #include <linux/sched.h>
> > +#include <linux/semaphore.h>
> >
> > #include <linux/hid.h>
> > #include <linux/hiddev.h>
> > @@ -1087,14 +1088,23 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
> > unsigned int i;
> > int ret;
>
> This must be "int ret = 0;".
>
> >
> > - if (!hid || !hid->driver)
> > + if (!hid)
> > return -ENODEV;
> > +
> > + if (down_trylock(&hid->driver_lock))
> > + return -EBUSY;
> > +
> > + if (!hid->driver) {
> > + ret = -ENODEV;
> > + goto unlock;
> > + }
> > report_enum = hid->report_enum + type;
> > hdrv = hid->driver;
> >
> > if (!size) {
> > dbg_hid("empty report\n");
> > - return -1;
> > + ret = -1;
> > + goto unlock;
> > }
> >
> > buf = kmalloc(sizeof(char) * HID_DEBUG_BUFSIZE, GFP_ATOMIC);
> > @@ -1118,17 +1128,23 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
> > nomem:
> > report = hid_get_report(report_enum, data);
> >
> > - if (!report)
> > - return -1;
> > + if (!report) {
> > + ret = -1;
> > + goto unlock;
> > + }
> >
> > if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
> > ret = hdrv->raw_event(hid, report, data, size);
> > - if (ret != 0)
> > - return ret < 0 ? ret : 0;
> > + if (ret != 0) {
> > + ret = ret < 0 ? ret : 0;
> > + goto unlock;
> > + }
> > }
> >
> > hid_report_raw_event(hid, type, data, size, interrupt);
> >
> > +unlock:
> > + up(&hid->driver_lock);
> > return 0;
>
> And this must be "return ret;" of course.
Applied, thanks David.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
