Re: [PATCH] Added a check for NULL pointer in hid_add_device

[michael . banken]

Hi,

The idea was to make the creation of an empty hid device while inside the
kernel easier. This problem came up, when I was writing a kernel module
driver for a usb temperature sensor. The sensors were supposed to be
recognized in lm-sensors, which does not recognize usb devices. The
solution in this case was to create an empty hid device to hook it up to
sysfs.
In some older implementations of this driver the solution to this problem
was creating an empty hid device to hook it up to sysfs.
These older implementations of the driver are not working anymore, unless
built with the change I am suggesting, because in the current version
their method of creating the hid device causes a NULL pointer dereference
at the point where I added the check.

The reason, why I believe this change to be completely harmless is that in
the case where there actually is a NULL pointer the function would
dereference it anyway immediately after the line where I added the check.
The only problem that to my understanding could possibly arise is delaying
the inevitable oops for a little while.

best regards,
Michael

On 06/25/2013 4:03 PM, Benjamin Tissoires wrote:
> Hi Michael,
>
> On 06/25/2013 07:51 PM, Michael Banken wrote:
>> This check greatly simplifies creating a dummy hid device.
>
> Could you elaborate a little here? If you want to create a hid device
> from the user space, I encourage you to use uhid, which is available
> since v3.6.
>
>> With this check in place a hid dummy can be created simply by allocating
>> and adding the device.
>> This used to be possible in earlier Kernel versions.
>>
>> Signed-off-by: Michael Banken
>> <michael.banken@xxxxxxxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Lorenz Haspel <lorenz@xxxxxxxxxxx>
>> ---
>>  drivers/hid/hid-core.c |   37 ++++++++++++++++++++-----------------
>>  1 file changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 0951a9a..88c573e 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -2289,24 +2289,27 @@ int hid_add_device(struct hid_device *hdev)
>>  	if (hid_ignore(hdev))
>>  		return -ENODEV;
>>
>> -	/*
>> -	 * Read the device report descriptor once and use as template
>> -	 * for the driver-specific modifications.
>> -	 */
>> -	ret = hdev->ll_driver->parse(hdev);
>> -	if (ret)
>> -		return ret;
>> -	if (!hdev->dev_rdesc)
>> -		return -ENODEV;
>> -
>> -	/*
>> -	 * Scan generic devices for group information
>> -	 */
>> -	if (hid_ignore_special_drivers ||
>> -	    !hid_match_id(hdev, hid_have_special_driver)) {
>> -		ret = hid_scan_report(hdev);
>> +	if (hdev->ll_driver != NULL) {
>
> I am concerned by the fact that *every* hid specific driver will have to
> call hid_hw_start and hid_hw_stop at some point. These 2 functions  are
> not protected against a null pointer in hdev->ll_driver, so allowing
> here a null pointer will introduce a kernel oops later.
>
> I think it would be safer to have:
> if (!hdev->ll_driver)
> 	return -ENODEV;
>
> But if I understood correctly, this is not your point.
> So definitively, I need your usage scenarios of such hid "dummy" device.
>
> Cheers,
> Benjamin
>
>> +		/*
>> +		 * Read the device report descriptor once and use as template
>> +			 * for the driver-specific modifications.
>> +		 */
>> +		ret = hdev->ll_driver->parse(hdev);
>>  		if (ret)
>> -			hid_warn(hdev, "bad device descriptor (%d)\n", ret);
>> +			return ret;
>> +		if (!hdev->dev_rdesc)
>> +			return -ENODEV;
>> +
>> +		/*
>> +		 * Scan generic devices for group information
>> +		 */
>> +		if (hid_ignore_special_drivers ||
>> +		    !hid_match_id(hdev, hid_have_special_driver)) {
>> +			ret = hid_scan_report(hdev);
>> +			if (ret)
>> +				hid_warn(hdev,
>> +					"bad device descriptor (%d)\n", ret);
>> +		}
>>  	}
>>
>>  	/* XXX hack, any other cleaner solution after the driver core
>>
>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/