On Thu, Jul 14, 2011 at 10:48 AM, David Herrmann
<dh.herrmann@xxxxxxxxxxxxxx> 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.
>
>>
>> Regards
>> David
>>
>
Although this race condition seems to be quite obvious, I have lots of
trouble to trigger it. So I don't know whether you actually care to
fix it, but just to keep it up: *ping*
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
