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