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

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

[Benjamin Tissoires]

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@mathe.stud.uni-erlangen.de>
> Signed-off-by: Lorenz Haspel <lorenz@badgers.com>
> ---
>  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
> 

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@mathe.stud.uni-erlangen.de>
>> Signed-off-by: Lorenz Haspel <lorenz@badgers.com>
>> ---
>>  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
>>
>
>

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

[David Herrmann]

Hi

On Wed, Jun 26, 2013 at 5:45 PM,
<michael.banken@mathe.stud.uni-erlangen.de> wrote:
> 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.

So you create an empty HID device to achieve what? I really cannot
follow, sorry. If you want lm_sensors to pick up your sensors, why not
patch lm_sensors to do so?

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

All other parts of the HID layer assume ll_driver to be NULL. It works
for you, because you avoid any ll_driver interaction so these parts of
the HID core that work with it will never get called. However, that's
not how it is supposed to work.

Could you describe why you cannot hook up your device as USB device
directly? Does lm_sensors only scan HID bus devices, not USB bus
devices or what is the exact reason?

> best regards,
> Michael

Cheers
David