Re: [RFC] [PATCH] HID: Fix race condition between driver core and ll-driver

[David Herrmann <dh.herrmann@googlemail.com>]

On Wed, Jul 13, 2011 at 4:24 PM, David Herrmann
<dh.herrmann@googlemail.com> 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@googlemail.com>
> ---
>  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.

>
> Regards
> David
>
--
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