[PATCH 0/4] HID: logitech-hidpp: fixes for error conditions

[PATCH 0/4] HID: logitech-hidpp: fixes for error conditions

[Peter Wu]

Hi Jiri,

Here are four patches intended for the 3.19 stream and are based on
jikos/hid (for-next, v3.18-rc4-144-gd9372ee).

* The first is actually from Benjamin Tissoires, but modified to remove
  a now unneeded goto.
* The second one depends on the first (it could work without, but there
  will be a context mismatch).
* The third one can be applied independently of the others and is needed
  to avoid a possible buffer overread.
* The fourth and final patch fixes an unbalanced hid_device_io_start().

Tested by booting with three paired USB devices (QEMU + USB
passthrough), two of them are powered off and one M525 is active. evbug
registers mouse events.

Kind regards,
Peter

Peter Wu (4):
  HID: logitech-hidpp: do not return the name length
  HID: logitech-hidpp: check name retrieval return code
  HID: logitech-hidpp: add boundary check for name retrieval
  HID: logitech-hidpp: disable io in probe error path

 drivers/hid/hid-logitech-hidpp.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.1.3

[PATCH 1/4] HID: logitech-hidpp: do not return the name length

[Peter Wu]

We do not make any use of the actual name length get through
hidpp_get_device_name(). Original patch by Benjamin Tissoires, this
patch also replaces a (now) unnecessary goto by return NULL.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/hid/hid-logitech-hidpp.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1a6395d..5066df8 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
 	return count;
 }
 
-static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
+static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 {
 	u8 feature_type;
 	u8 feature_index;
@@ -473,28 +473,23 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
 	ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_GET_DEVICE_NAME_TYPE,
 		&feature_index, &feature_type);
 	if (ret)
-		goto out_err;
+		return NULL;
 
 	ret = hidpp_devicenametype_get_count(hidpp, feature_index,
 		&__name_length);
 	if (ret)
-		goto out_err;
+		return NULL;
 
 	name = kzalloc(__name_length + 1, GFP_KERNEL);
 	if (!name)
-		goto out_err;
+		return NULL;
 
-	*name_length = __name_length + 1;
 	while (index < __name_length)
 		index += hidpp_devicenametype_get_device_name(hidpp,
 			feature_index, index, name + index,
 			__name_length - index);
 
 	return name;
-
-out_err:
-	*name_length = 0;
-	return NULL;
 }
 
 /* -------------------------------------------------------------------------- */
@@ -989,7 +984,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	char *name;
-	u8 name_length;
 
 	if (use_unifying)
 		/*
@@ -999,7 +993,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
 		 */
 		name = hidpp_get_unifying_name(hidpp);
 	else
-		name = hidpp_get_device_name(hidpp, &name_length);
+		name = hidpp_get_device_name(hidpp);
 
 	if (!name)
 		hid_err(hdev, "unable to retrieve the name of the device");
@@ -1053,7 +1047,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 	bool connected = atomic_read(&hidpp->connected);
 	struct input_dev *input;
 	char *name, *devm_name;
-	u8 name_length;
 
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		wtp_connect(hdev, connected);
@@ -1080,7 +1073,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
 		return;
 	}
 
-	name = hidpp_get_device_name(hidpp, &name_length);
+	name = hidpp_get_device_name(hidpp);
 	if (!name) {
 		hid_err(hdev, "unable to retrieve the name of the device");
 	} else {
-- 
2.1.3

Re: [PATCH 1/4] HID: logitech-hidpp: do not return the name length

[Benjamin Tissoires]

On Dec 11 2014 or thereabouts, Peter Wu wrote:
> We do not make any use of the actual name length get through
> hidpp_get_device_name(). Original patch by Benjamin Tissoires, this
> patch also replaces a (now) unnecessary goto by return NULL.
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks for the v2 Peter.

Cheers,
Benjamin

>  drivers/hid/hid-logitech-hidpp.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1a6395d..5066df8 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
>  	return count;
>  }
>  
> -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
>  {
>  	u8 feature_type;
>  	u8 feature_index;
> @@ -473,28 +473,23 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
>  	ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_GET_DEVICE_NAME_TYPE,
>  		&feature_index, &feature_type);
>  	if (ret)
> -		goto out_err;
> +		return NULL;
>  
>  	ret = hidpp_devicenametype_get_count(hidpp, feature_index,
>  		&__name_length);
>  	if (ret)
> -		goto out_err;
> +		return NULL;
>  
>  	name = kzalloc(__name_length + 1, GFP_KERNEL);
>  	if (!name)
> -		goto out_err;
> +		return NULL;
>  
> -	*name_length = __name_length + 1;
>  	while (index < __name_length)
>  		index += hidpp_devicenametype_get_device_name(hidpp,
>  			feature_index, index, name + index,
>  			__name_length - index);
>  
>  	return name;
> -
> -out_err:
> -	*name_length = 0;
> -	return NULL;
>  }
>  
>  /* -------------------------------------------------------------------------- */
> @@ -989,7 +984,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
>  {
>  	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
>  	char *name;
> -	u8 name_length;
>  
>  	if (use_unifying)
>  		/*
> @@ -999,7 +993,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
>  		 */
>  		name = hidpp_get_unifying_name(hidpp);
>  	else
> -		name = hidpp_get_device_name(hidpp, &name_length);
> +		name = hidpp_get_device_name(hidpp);
>  
>  	if (!name)
>  		hid_err(hdev, "unable to retrieve the name of the device");
> @@ -1053,7 +1047,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>  	bool connected = atomic_read(&hidpp->connected);
>  	struct input_dev *input;
>  	char *name, *devm_name;
> -	u8 name_length;
>  
>  	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>  		wtp_connect(hdev, connected);
> @@ -1080,7 +1073,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>  		return;
>  	}
>  
> -	name = hidpp_get_device_name(hidpp, &name_length);
> +	name = hidpp_get_device_name(hidpp);
>  	if (!name) {
>  		hid_err(hdev, "unable to retrieve the name of the device");
>  	} else {
> -- 
> 2.1.3
> 

[PATCH 2/4] HID: logitech-hidpp: check name retrieval return code

[Peter Wu]

hidpp_devicenametype_get_device_name() may return a negative value on
protocol errors (for example, when the device is powered off).
Explicitly check this condition to avoid a long-running loop.

(0 cannot be returned as __name_length - index > 0, but check for it
anyway as it would otherwise result in an infinite loop.)

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5066df8..4d72c20 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -484,10 +484,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
 	if (!name)
 		return NULL;
 
-	while (index < __name_length)
-		index += hidpp_devicenametype_get_device_name(hidpp,
+	while (index < __name_length) {
+		ret = hidpp_devicenametype_get_device_name(hidpp,
 			feature_index, index, name + index,
 			__name_length - index);
+		if (ret <= 0) {
+			kfree(name);
+			return NULL;
+		}
+		index += ret;
+	}
 
 	return name;
 }
-- 
2.1.3

Re: [PATCH 2/4] HID: logitech-hidpp: check name retrieval return code

[Benjamin Tissoires]

On Dec 11 2014 or thereabouts, Peter Wu wrote:
> hidpp_devicenametype_get_device_name() may return a negative value on
> protocol errors (for example, when the device is powered off).
> Explicitly check this condition to avoid a long-running loop.
> 
> (0 cannot be returned as __name_length - index > 0, but check for it
> anyway as it would otherwise result in an infinite loop.)
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 5066df8..4d72c20 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -484,10 +484,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
>  	if (!name)
>  		return NULL;
>  
> -	while (index < __name_length)
> -		index += hidpp_devicenametype_get_device_name(hidpp,
> +	while (index < __name_length) {
> +		ret = hidpp_devicenametype_get_device_name(hidpp,
>  			feature_index, index, name + index,
>  			__name_length - index);
> +		if (ret <= 0) {
> +			kfree(name);
> +			return NULL;
> +		}
> +		index += ret;
> +	}
>  
>  	return name;
>  }
> -- 
> 2.1.3
> 

[PATCH 3/4] HID: logitech-hidpp: add boundary check for name retrieval

[Peter Wu]

The HID response has a limited size. Do not trust the value returned by
hardware, check that it really fits in the message.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/hid/hid-logitech-hidpp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4d72c20..4292cc3 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -313,6 +313,9 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
 
 	len = response.rap.params[1];
 
+	if (2 + len > sizeof(response.rap.params))
+		return NULL;
+
 	name = kzalloc(len + 1, GFP_KERNEL);
 	if (!name)
 		return NULL;
-- 
2.1.3

Re: [PATCH 3/4] HID: logitech-hidpp: add boundary check for name retrieval

[Benjamin Tissoires]

On Dec 11 2014 or thereabouts, Peter Wu wrote:
> The HID response has a limited size. Do not trust the value returned by
> hardware, check that it really fits in the message.
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---

Well spotted!

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin


>  drivers/hid/hid-logitech-hidpp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4d72c20..4292cc3 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -313,6 +313,9 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
>  
>  	len = response.rap.params[1];
>  
> +	if (2 + len > sizeof(response.rap.params))
> +		return NULL;
> +
>  	name = kzalloc(len + 1, GFP_KERNEL);
>  	if (!name)
>  		return NULL;
> -- 
> 2.1.3
> 

[PATCH 4/4] HID: logitech-hidpp: disable io in probe error path

[Peter Wu]

Balance a hid_device_io_start() call with hid_device_io_stop() in the
error path. This avoids processing of HID reports when the probe fails
which possibly leads to invalid memory access in hid_device_probe() as
report_enum->report_id_hash might already be freed via
hid_close_report().

hid_set_drvdata() is called before wtp_allocate, be consistent and clear
drvdata too on the error path of wtp_allocate.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
Hi Andrew,

There are few users of hid_device_io_start/stop, this is the first one
to get start/stop out of sync. Should the comment of
hid_device_io_start() be updated to ensure that hid_device_io_stop()
gets called before probe() returns? Or should the hid-core be changed to
handle this out-of-sync issue:

		if (ret) {
			if (hdev->io_started))
				down(&hdev->driver_input_lock);
			hid_close_report(hdev);
			hdev->driver = NULL;
		}

Is my observation correct or not that HID reports can arrive during
hid_close_report() when io_started == true?

Kind regards,
Peter
---
 drivers/hid/hid-logitech-hidpp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4292cc3..2f420c0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
 		ret = wtp_allocate(hdev, id);
 		if (ret)
-			return ret;
+			goto wtp_allocate_fail;
 	}
 
 	INIT_WORK(&hidpp->work, delayed_work_cb);
@@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
 		if (!connected) {
 			hid_err(hdev, "Device not connected");
+			hid_device_io_stop(hdev);
 			goto hid_parse_fail;
 		}
 
@@ -1186,6 +1187,7 @@ hid_hw_start_fail:
 hid_parse_fail:
 	cancel_work_sync(&hidpp->work);
 	mutex_destroy(&hidpp->send_mutex);
+wtp_allocate_fail:
 	hid_set_drvdata(hdev, NULL);
 	return ret;
 }
-- 
2.1.3

Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path

[Benjamin Tissoires]

On Dec 11 2014 or thereabouts, Peter Wu wrote:
> Balance a hid_device_io_start() call with hid_device_io_stop() in the
> error path. This avoids processing of HID reports when the probe fails
> which possibly leads to invalid memory access in hid_device_probe() as
> report_enum->report_id_hash might already be freed via
> hid_close_report().

Well spotted too!

> 
> hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> drvdata too on the error path of wtp_allocate.

This is not strictly speaking required. We will have a dangling value in
hdev->private_data, but this should be overwritten before the next use.

Anyway, it makes sense to clean up after a failure, so the patch is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
> Hi Andrew,
> 
> There are few users of hid_device_io_start/stop, this is the first one
> to get start/stop out of sync. Should the comment of
> hid_device_io_start() be updated to ensure that hid_device_io_stop()
> gets called before probe() returns? Or should the hid-core be changed to
> handle this out-of-sync issue:
> 
> 		if (ret) {
> 			if (hdev->io_started))
> 				down(&hdev->driver_input_lock);
> 			hid_close_report(hdev);
> 			hdev->driver = NULL;
> 		}
> 
> Is my observation correct or not that HID reports can arrive during
> hid_close_report() when io_started == true?
> 
> Kind regards,
> Peter
> ---
>  drivers/hid/hid-logitech-hidpp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4292cc3..2f420c0 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>  		ret = wtp_allocate(hdev, id);
>  		if (ret)
> -			return ret;
> +			goto wtp_allocate_fail;
>  	}
>  
>  	INIT_WORK(&hidpp->work, delayed_work_cb);
> @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
>  		if (!connected) {
>  			hid_err(hdev, "Device not connected");
> +			hid_device_io_stop(hdev);
>  			goto hid_parse_fail;
>  		}
>  
> @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
>  hid_parse_fail:
>  	cancel_work_sync(&hidpp->work);
>  	mutex_destroy(&hidpp->send_mutex);
> +wtp_allocate_fail:
>  	hid_set_drvdata(hdev, NULL);
>  	return ret;
>  }
> -- 
> 2.1.3
> 

Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path

[Andrew de los Reyes]

On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > Balance a hid_device_io_start() call with hid_device_io_stop() in the
> > error path. This avoids processing of HID reports when the probe fails
> > which possibly leads to invalid memory access in hid_device_probe() as
> > report_enum->report_id_hash might already be freed via
> > hid_close_report().
>
> Well spotted too!
>
> >
> > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> > drvdata too on the error path of wtp_allocate.
>
> This is not strictly speaking required. We will have a dangling value in
> hdev->private_data, but this should be overwritten before the next use.
>
> Anyway, it makes sense to clean up after a failure, so the patch is
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Cheers,
> Benjamin
>
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> > Hi Andrew,
> >
> > There are few users of hid_device_io_start/stop, this is the first one
> > to get start/stop out of sync. Should the comment of
> > hid_device_io_start() be updated to ensure that hid_device_io_stop()
> > gets called before probe() returns? Or should the hid-core be changed to
> > handle this out-of-sync issue:

I do not have a strong opinion on this, and will defer to others. The
reason I needed to communicate during probe() was to have the driver
probe the actual device for details. In this use case, I would be okay
to disable IO at the end of probe() and have it become reenabled via
the normal (default) methods.

-andrew

> >
> >               if (ret) {
> >                       if (hdev->io_started))
> >                               down(&hdev->driver_input_lock);
> >                       hid_close_report(hdev);
> >                       hdev->driver = NULL;
> >               }
> >
> > Is my observation correct or not that HID reports can arrive during
> > hid_close_report() when io_started == true?
> >
> > Kind regards,
> > Peter
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 4292cc3..2f420c0 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> >               ret = wtp_allocate(hdev, id);
> >               if (ret)
> > -                     return ret;
> > +                     goto wtp_allocate_fail;
> >       }
> >
> >       INIT_WORK(&hidpp->work, delayed_work_cb);
> > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >       if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> >               if (!connected) {
> >                       hid_err(hdev, "Device not connected");
> > +                     hid_device_io_stop(hdev);
> >                       goto hid_parse_fail;
> >               }
> >
> > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> >  hid_parse_fail:
> >       cancel_work_sync(&hidpp->work);
> >       mutex_destroy(&hidpp->send_mutex);
> > +wtp_allocate_fail:
> >       hid_set_drvdata(hdev, NULL);
> >       return ret;
> >  }
> > --
> > 2.1.3
> >
> --
> 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

Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path

[Peter Wu]

On Thursday 11 December 2014 09:37:06 Andrew de los Reyes wrote:
> On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > > Balance a hid_device_io_start() call with hid_device_io_stop() in the
> > > error path. This avoids processing of HID reports when the probe fails
> > > which possibly leads to invalid memory access in hid_device_probe() as
> > > report_enum->report_id_hash might already be freed via
> > > hid_close_report().
> >
> > Well spotted too!
> >
> > >
> > > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> > > drvdata too on the error path of wtp_allocate.
> >
> > This is not strictly speaking required. We will have a dangling value in
> > hdev->private_data, but this should be overwritten before the next use.
> >
> > Anyway, it makes sense to clean up after a failure, so the patch is
> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > > ---
> > > Hi Andrew,
> > >
> > > There are few users of hid_device_io_start/stop, this is the first one
> > > to get start/stop out of sync. Should the comment of
> > > hid_device_io_start() be updated to ensure that hid_device_io_stop()
> > > gets called before probe() returns? Or should the hid-core be changed to
> > > handle this out-of-sync issue:
> 
> I do not have a strong opinion on this, and will defer to others. The
> reason I needed to communicate during probe() was to have the driver
> probe the actual device for details. In this use case, I would be okay
> to disable IO at the end of probe() and have it become reenabled via
> the normal (default) methods.
> 
> -andrew

Keeping the reports enabled when the probe succeeds is fine, I am
referring to the error path. If the probe fails, then reports should
never be accepted or a corruption may occur (if I see it correctly).

Is this analysis correct?

 hid_device_probe()
   hid_device_io_start()
   return FAILURE
 hid_close_report(device)
   report_enum = device
        ->report_enum[i]
   hid_free_report(report_enum
        ->report_id_hash[j])    <-- NOTE: freed
                    ... interrupt occurs ...
                                    hid_input_report()
                                      hid_get_report()
                                        report = report_enum->report_id_hash[n];
                                                 ^ NOTE: use-after-free
                                        return report if not NULL
                                      hdrv->raw_event(report) <--- BOOM?
   kfree(device->rdesc)
 device->driver = NULL

Kind regards,
Peter

> > >
> > >               if (ret) {
> > >                       if (hdev->io_started))
> > >                               down(&hdev->driver_input_lock);
> > >                       hid_close_report(hdev);
> > >                       hdev->driver = NULL;
> > >               }
> > >
> > > Is my observation correct or not that HID reports can arrive during
> > > hid_close_report() when io_started == true?
> > >
> > > Kind regards,
> > > Peter
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 4292cc3..2f420c0 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > >               ret = wtp_allocate(hdev, id);
> > >               if (ret)
> > > -                     return ret;
> > > +                     goto wtp_allocate_fail;
> > >       }
> > >
> > >       INIT_WORK(&hidpp->work, delayed_work_cb);
> > > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >       if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> > >               if (!connected) {
> > >                       hid_err(hdev, "Device not connected");
> > > +                     hid_device_io_stop(hdev);
> > >                       goto hid_parse_fail;
> > >               }
> > >
> > > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> > >  hid_parse_fail:
> > >       cancel_work_sync(&hidpp->work);
> > >       mutex_destroy(&hidpp->send_mutex);
> > > +wtp_allocate_fail:
> > >       hid_set_drvdata(hdev, NULL);
> > >       return ret;
> > >  }
> > > --
> > > 2.1.3
> > >

Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path

[Andrew de los Reyes]

I would think you'd need to call hid_device_io_stop in probe before
returning just to make sure the hid->driver_input_lock is in the right
state. You may be able to consult hid->io_started in the probe thread
after calling probe, and do the appropriate cleanup if probe returned
error. IMHO I like to keep things like lock/unlock in pairs near each
other, so it seems preferable to require that drivers handle calling
hid_device_io_stop themselves if probe will fail. I don't know in
general what the Linux stance is, tho.

-andrew

On Thu, Dec 11, 2014 at 9:53 AM, Peter Wu <peter@lekensteyn.nl> wrote:
> On Thursday 11 December 2014 09:37:06 Andrew de los Reyes wrote:
>> On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>> >
>> > On Dec 11 2014 or thereabouts, Peter Wu wrote:
>> > > Balance a hid_device_io_start() call with hid_device_io_stop() in the
>> > > error path. This avoids processing of HID reports when the probe fails
>> > > which possibly leads to invalid memory access in hid_device_probe() as
>> > > report_enum->report_id_hash might already be freed via
>> > > hid_close_report().
>> >
>> > Well spotted too!
>> >
>> > >
>> > > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
>> > > drvdata too on the error path of wtp_allocate.
>> >
>> > This is not strictly speaking required. We will have a dangling value in
>> > hdev->private_data, but this should be overwritten before the next use.
>> >
>> > Anyway, it makes sense to clean up after a failure, so the patch is
>> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> >
>> > Cheers,
>> > Benjamin
>> >
>> > >
>> > > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
>> > > ---
>> > > Hi Andrew,
>> > >
>> > > There are few users of hid_device_io_start/stop, this is the first one
>> > > to get start/stop out of sync. Should the comment of
>> > > hid_device_io_start() be updated to ensure that hid_device_io_stop()
>> > > gets called before probe() returns? Or should the hid-core be changed to
>> > > handle this out-of-sync issue:
>>
>> I do not have a strong opinion on this, and will defer to others. The
>> reason I needed to communicate during probe() was to have the driver
>> probe the actual device for details. In this use case, I would be okay
>> to disable IO at the end of probe() and have it become reenabled via
>> the normal (default) methods.
>>
>> -andrew
>
> Keeping the reports enabled when the probe succeeds is fine, I am
> referring to the error path. If the probe fails, then reports should
> never be accepted or a corruption may occur (if I see it correctly).
>
> Is this analysis correct?
>
>  hid_device_probe()
>    hid_device_io_start()
>    return FAILURE
>  hid_close_report(device)
>    report_enum = device
>         ->report_enum[i]
>    hid_free_report(report_enum
>         ->report_id_hash[j])    <-- NOTE: freed
>                     ... interrupt occurs ...
>                                     hid_input_report()
>                                       hid_get_report()
>                                         report = report_enum->report_id_hash[n];
>                                                  ^ NOTE: use-after-free
>                                         return report if not NULL
>                                       hdrv->raw_event(report) <--- BOOM?
>    kfree(device->rdesc)
>  device->driver = NULL
>
> Kind regards,
> Peter
>
>> > >
>> > >               if (ret) {
>> > >                       if (hdev->io_started))
>> > >                               down(&hdev->driver_input_lock);
>> > >                       hid_close_report(hdev);
>> > >                       hdev->driver = NULL;
>> > >               }
>> > >
>> > > Is my observation correct or not that HID reports can arrive during
>> > > hid_close_report() when io_started == true?
>> > >
>> > > Kind regards,
>> > > Peter
>> > > ---
>> > >  drivers/hid/hid-logitech-hidpp.c | 4 +++-
>> > >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> > > index 4292cc3..2f420c0 100644
>> > > --- a/drivers/hid/hid-logitech-hidpp.c
>> > > +++ b/drivers/hid/hid-logitech-hidpp.c
>> > > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> > >       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>> > >               ret = wtp_allocate(hdev, id);
>> > >               if (ret)
>> > > -                     return ret;
>> > > +                     goto wtp_allocate_fail;
>> > >       }
>> > >
>> > >       INIT_WORK(&hidpp->work, delayed_work_cb);
>> > > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> > >       if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
>> > >               if (!connected) {
>> > >                       hid_err(hdev, "Device not connected");
>> > > +                     hid_device_io_stop(hdev);
>> > >                       goto hid_parse_fail;
>> > >               }
>> > >
>> > > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
>> > >  hid_parse_fail:
>> > >       cancel_work_sync(&hidpp->work);
>> > >       mutex_destroy(&hidpp->send_mutex);
>> > > +wtp_allocate_fail:
>> > >       hid_set_drvdata(hdev, NULL);
>> > >       return ret;
>> > >  }
>> > > --
>> > > 2.1.3
>> > >
>

Re: [PATCH 0/4] HID: logitech-hidpp: fixes for error conditions

[Jiri Kosina]

On Thu, 11 Dec 2014, Peter Wu wrote:

> Hi Jiri,
> 
> Here are four patches intended for the 3.19 stream and are based on
> jikos/hid (for-next, v3.18-rc4-144-gd9372ee).
> 
> * The first is actually from Benjamin Tissoires, but modified to remove
>   a now unneeded goto.
> * The second one depends on the first (it could work without, but there
>   will be a context mismatch).
> * The third one can be applied independently of the others and is needed
>   to avoid a possible buffer overread.
> * The fourth and final patch fixes an unbalanced hid_device_io_start().
> 
> Tested by booting with three paired USB devices (QEMU + USB
> passthrough), two of them are powered off and one M525 is active. evbug
> registers mouse events.
> 

Good stuff, thanks!

Applied.

-- 
Jiri Kosina
SUSE Labs