[PATCH 0/3] HID: logitech-{dj,hidpp}: more reliability fixes

[PATCH 0/3] HID: logitech-{dj,hidpp}: more reliability fixes

[Peter Wu]

Hi Jiri,

Here are more fixes intended for the 3.19 tree after a review. Two bugfixes.
one which was mentioned in a mail with Benjamin ("avoid unintended
fall-through") and a fix to avoid a possible 5 second delay for HID++ 2.0
errors. I haven't encountered a case where the hidpp module generates a HID++
2.0 error though, so maybe that change can go to 3.20 too if you want to keep
the changeset small.

The third (second) patch adds a check to avoid passing a short report. A similar
fix should probably be written for stable kernels (the code was changed in 3.19,
but the length check was already missing in older kernels).

Kind regards,
Peter

Peter Wu (3):
  HID: logitech-hidpp: detect HID++ 2.0 errors too
  HID: logitech-{dj,hidpp}: check report length
  HID: logitech-hidpp: avoid unintended fall-through

 drivers/hid/hid-logitech-dj.c    | 16 +++++++++++++++-
 drivers/hid/hid-logitech-hidpp.c | 30 ++++++++++++++++++++++++------
 2 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.1.3

[PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Peter Wu]

Devices speaking HID++ 2.0 report a different error code (0xff). Detect
these errors too to avoid 5 second delays when the device reports an
error. Caught by... well, a bug in the QEMU emulation of this receiver.

Renamed fap to rap for HID++ 1.0 errors because it is more logical,
it has no functional difference.

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2f420c0..ae23dec 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -105,6 +105,7 @@ struct hidpp_device {
 };
 
 
+/* HID++ 1.0 error codes */
 #define HIDPP_ERROR				0x8f
 #define HIDPP_ERROR_SUCCESS			0x00
 #define HIDPP_ERROR_INVALID_SUBID		0x01
@@ -119,6 +120,8 @@ struct hidpp_device {
 #define HIDPP_ERROR_REQUEST_UNAVAILABLE		0x0a
 #define HIDPP_ERROR_INVALID_PARAM_VALUE		0x0b
 #define HIDPP_ERROR_WRONG_PIN_CODE		0x0c
+/* HID++ 2.0 error codes */
+#define HIDPP20_ERROR				0xff
 
 static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
 
@@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
 	}
 
 	if (response->report_id == REPORT_ID_HIDPP_SHORT &&
-	    response->fap.feature_index == HIDPP_ERROR) {
+	    response->rap.sub_id == HIDPP_ERROR) {
+		ret = response->rap.params[1];
+		dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
+		goto exit;
+	}
+
+	if (response->report_id == REPORT_ID_HIDPP_LONG &&
+	    response->fap.feature_index == HIDPP20_ERROR) {
 		ret = response->fap.params[1];
-		dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
+		dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
 		goto exit;
 	}
 
@@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
 static inline bool hidpp_match_error(struct hidpp_report *question,
 		struct hidpp_report *answer)
 {
-	return (answer->fap.feature_index == HIDPP_ERROR) &&
+	return ((answer->rap.sub_id == HIDPP_ERROR) ||
+	    (answer->fap.feature_index == HIDPP20_ERROR)) &&
 	    (answer->fap.funcindex_clientid == question->fap.feature_index) &&
 	    (answer->fap.params[0] == question->fap.funcindex_clientid);
 }
-- 
2.1.3

[PATCH 2/3] HID: logitech-{dj,hidpp}: check report length

[Peter Wu]

Malicious USB devices can send bogus reports smaller than the expected
buffer size. Ensure that the length is valid to avoid reading out of
bounds.

For the old WTP, I do not have a HID descriptor so just check for the
minimum length in hidpp_raw_event (this can be changed to an inequality
later).

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

If you know that the WTP report (ID 2) has a length of 2, then you can change
"<" to "!=" and remove the paragraph from the commit message.

Kind regards,
Peter
---
 drivers/hid/hid-logitech-dj.c    | 16 +++++++++++++++-
 drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index c917ab6..5bc6d80 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
 
 	switch (data[0]) {
 	case REPORT_ID_DJ_SHORT:
+		if (size != DJREPORT_SHORT_LENGTH) {
+			dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
+			return false;
+		}
 		return logi_dj_dj_event(hdev, report, data, size);
 	case REPORT_ID_HIDPP_SHORT:
-		/* intentional fallthrough */
+		if (size != HIDPP_REPORT_SHORT_LENGTH) {
+			dev_err(&hdev->dev,
+				"Short HID++ report of bad size (%d)", size);
+			return false;
+		}
+		return logi_dj_hidpp_event(hdev, report, data, size);
 	case REPORT_ID_HIDPP_LONG:
+		if (size != HIDPP_REPORT_LONG_LENGTH) {
+			dev_err(&hdev->dev,
+				"Long HID++ report of bad size (%d)", size);
+			return false;
+		}
 		return logi_dj_hidpp_event(hdev, report, data, size);
 	}
 
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ae23dec..2315358 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 			return 1;
 		}
 		return hidpp_raw_hidpp_event(hidpp, data, size);
+	case 0x02:
+		if (size < 2) {
+			hid_err(hdev, "Received HID report of bad size (%d)",
+				size);
+			return 1;
+		}
+		if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
+			return wtp_raw_event(hdev, data, size);
+		return 1;
 	}
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
-		return wtp_raw_event(hdev, data, size);
-
 	return 0;
 }
 
-- 
2.1.3

[PATCH 3/3] HID: logitech-hidpp: avoid unintended fall-through

[Peter Wu]

Add a return to avoid a fall-through. Introduced in commit
57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID: logitech-hidpp: add
support of the first Logitech Wireless Touchpad").

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

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2315358..09eee17 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -811,6 +811,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
 			input_event(wd->input, EV_KEY, BTN_RIGHT,
 					!!(data[1] & 0x02));
 			input_sync(wd->input);
+			return 0;
 		} else {
 			if (size < 21)
 				return 1;
-- 
2.1.3

Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Benjamin Tissoires]

Hi Peter,

On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> these errors too to avoid 5 second delays when the device reports an
> error. Caught by... well, a bug in the QEMU emulation of this receiver.
>
> Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> it has no functional difference.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---

I'd like to have Nestor's opinion on this. I did not manage to find on
the documentation that HID++ 2.0 Long report error code is 0xff, so
introducing this change without Logitech's blessing would be
unfortunate.
I understand this will fix your qemu problem, but I am not entirely
sure if we do not have to check on 0xff and 0x8f in both short and
long responses.

Cheers,
Benjamin

>  drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2f420c0..ae23dec 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -105,6 +105,7 @@ struct hidpp_device {
>  };
>
>
> +/* HID++ 1.0 error codes */
>  #define HIDPP_ERROR                            0x8f
>  #define HIDPP_ERROR_SUCCESS                    0x00
>  #define HIDPP_ERROR_INVALID_SUBID              0x01
> @@ -119,6 +120,8 @@ struct hidpp_device {
>  #define HIDPP_ERROR_REQUEST_UNAVAILABLE                0x0a
>  #define HIDPP_ERROR_INVALID_PARAM_VALUE                0x0b
>  #define HIDPP_ERROR_WRONG_PIN_CODE             0x0c
> +/* HID++ 2.0 error codes */
> +#define HIDPP20_ERROR                          0xff
>
>  static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
>
> @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
>         }
>
>         if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> -           response->fap.feature_index == HIDPP_ERROR) {
> +           response->rap.sub_id == HIDPP_ERROR) {
> +               ret = response->rap.params[1];
> +               dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> +               goto exit;
> +       }
> +
> +       if (response->report_id == REPORT_ID_HIDPP_LONG &&
> +           response->fap.feature_index == HIDPP20_ERROR) {
>                 ret = response->fap.params[1];
> -               dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
> +               dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
>                 goto exit;
>         }
>
> @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
>  static inline bool hidpp_match_error(struct hidpp_report *question,
>                 struct hidpp_report *answer)
>  {
> -       return (answer->fap.feature_index == HIDPP_ERROR) &&
> +       return ((answer->rap.sub_id == HIDPP_ERROR) ||
> +           (answer->fap.feature_index == HIDPP20_ERROR)) &&
>             (answer->fap.funcindex_clientid == question->fap.feature_index) &&
>             (answer->fap.params[0] == question->fap.funcindex_clientid);
>  }
> --
> 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 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Peter Wu]

On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> > Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> > these errors too to avoid 5 second delays when the device reports an
> > error. Caught by... well, a bug in the QEMU emulation of this receiver.
> >
> > Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> > it has no functional difference.
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> 
> I'd like to have Nestor's opinion on this. I did not manage to find on
> the documentation that HID++ 2.0 Long report error code is 0xff, so
> introducing this change without Logitech's blessing would be
> unfortunate.
> I understand this will fix your qemu problem, but I am not entirely
> sure if we do not have to check on 0xff and 0x8f in both short and
> long responses.
> 
> Cheers,
> Benjamin

The error code was found by probing the hardware. The HID++ 2.0 spec
does define some error codes, for example an OutOfRange error when
GetFeatureID is called with a featureIndex greater than the available
features count. The documentation also defines the valid FeatureIndex
range as 1..254, so I thought it was reasonable to assume that 0xff is
the HID++ 2.0 error indicator.

Nestor, so far I have only seen the OutOfRange error when the arguments
are invalid. Are there other cases where HID++ 2.0 are reported instead
of HID++ 1.0?

QEMU was not the problem though, it was just a bug in my
usb-ltunify-receiver device emulation which exposed this missing check.

Kind regards,
Peter

> >  drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 2f420c0..ae23dec 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -105,6 +105,7 @@ struct hidpp_device {
> >  };
> >
> >
> > +/* HID++ 1.0 error codes */
> >  #define HIDPP_ERROR                            0x8f
> >  #define HIDPP_ERROR_SUCCESS                    0x00
> >  #define HIDPP_ERROR_INVALID_SUBID              0x01
> > @@ -119,6 +120,8 @@ struct hidpp_device {
> >  #define HIDPP_ERROR_REQUEST_UNAVAILABLE                0x0a
> >  #define HIDPP_ERROR_INVALID_PARAM_VALUE                0x0b
> >  #define HIDPP_ERROR_WRONG_PIN_CODE             0x0c
> > +/* HID++ 2.0 error codes */
> > +#define HIDPP20_ERROR                          0xff
> >
> >  static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
> >
> > @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> >         }
> >
> >         if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > -           response->fap.feature_index == HIDPP_ERROR) {
> > +           response->rap.sub_id == HIDPP_ERROR) {
> > +               ret = response->rap.params[1];
> > +               dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > +               goto exit;
> > +       }
> > +
> > +       if (response->report_id == REPORT_ID_HIDPP_LONG &&
> > +           response->fap.feature_index == HIDPP20_ERROR) {
> >                 ret = response->fap.params[1];
> > -               dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
> > +               dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> >                 goto exit;
> >         }
> >
> > @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
> >  static inline bool hidpp_match_error(struct hidpp_report *question,
> >                 struct hidpp_report *answer)
> >  {
> > -       return (answer->fap.feature_index == HIDPP_ERROR) &&
> > +       return ((answer->rap.sub_id == HIDPP_ERROR) ||
> > +           (answer->fap.feature_index == HIDPP20_ERROR)) &&
> >             (answer->fap.funcindex_clientid == question->fap.feature_index) &&
> >             (answer->fap.params[0] == question->fap.funcindex_clientid);
> >  }
> > --
> > 2.1.3

Re: [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length

[Benjamin Tissoires]

Hi Peter,

On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> Malicious USB devices can send bogus reports smaller than the expected
> buffer size. Ensure that the length is valid to avoid reading out of
> bounds.
>
> For the old WTP, I do not have a HID descriptor so just check for the
> minimum length in hidpp_raw_event (this can be changed to an inequality
> later).

Actually you have it :)
All the DJ devices share the same report descriptors as they are
provided by hid-logitech-dj :)

Anyway, the problem here would be with the bluetooth touchpad T651
which sends its raw events over teh mouse (0x02) collection (hint:
there is a "< 21" in wtp_raw_event :-P )

>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
> Hi,
>
> If you know that the WTP report (ID 2) has a length of 2, then you can change
> "<" to "!=" and remove the paragraph from the commit message.

"<" should be kept for the reason above.

>
> Kind regards,
> Peter
> ---
>  drivers/hid/hid-logitech-dj.c    | 16 +++++++++++++++-
>  drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
>  2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index c917ab6..5bc6d80 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
>
>         switch (data[0]) {
>         case REPORT_ID_DJ_SHORT:
> +               if (size != DJREPORT_SHORT_LENGTH) {
> +                       dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
> +                       return false;
> +               }
>                 return logi_dj_dj_event(hdev, report, data, size);
>         case REPORT_ID_HIDPP_SHORT:
> -               /* intentional fallthrough */
> +               if (size != HIDPP_REPORT_SHORT_LENGTH) {
> +                       dev_err(&hdev->dev,
> +                               "Short HID++ report of bad size (%d)", size);
> +                       return false;
> +               }
> +               return logi_dj_hidpp_event(hdev, report, data, size);
>         case REPORT_ID_HIDPP_LONG:
> +               if (size != HIDPP_REPORT_LONG_LENGTH) {
> +                       dev_err(&hdev->dev,
> +                               "Long HID++ report of bad size (%d)", size);
> +                       return false;
> +               }

This hunk is good to me.

>                 return logi_dj_hidpp_event(hdev, report, data, size);
>         }
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ae23dec..2315358 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
>                         return 1;
>                 }
>                 return hidpp_raw_hidpp_event(hidpp, data, size);
> +       case 0x02:
> +               if (size < 2) {
> +                       hid_err(hdev, "Received HID report of bad size (%d)",
> +                               size);
> +                       return 1;
> +               }
> +               if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> +                       return wtp_raw_event(hdev, data, size);
> +               return 1;
>         }
>
> -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> -               return wtp_raw_event(hdev, data, size);

This one is OK, but I don't like it.

wtp_raw_event also expects long hid++ reports, and I'd prefer having
the raw_event() callback first checking on the generic hid++ reports,
and then addressing the various subclasses of devices.
After a better look at the code, it occurs that the actual code is
already pretty messed up.
wtp_raw_event() is called both in the generic hidpp_raw_event() and in
the specific hidpp_raw_hidpp_event().
This is IMO a design flaw and it should be fixed in a better way.

I'd better have:

- A check on the report size
- A call to the specific hidpp_raw_hidpp_event()
- if the previous does not return 1 (consumed event), then check on
all subclasses and call their specific raw_event.

Does that make sense?

If you agree, you can split the patch in 3, one for the -dj, one for
the -hidpp checks, and one for the redesign. I'd be happy to make the
redesign if you do not want to reshuffle it in a third patch.

Cheers,
Benjamin

> -
>         return 0;
>  }
>
> --
> 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 3/3] HID: logitech-hidpp: avoid unintended fall-through

[Benjamin Tissoires]

On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> Add a return to avoid a fall-through. Introduced in commit
> 57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID: logitech-hidpp: add
> support of the first Logitech Wireless Touchpad").
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---

This one is reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/hid/hid-logitech-hidpp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2315358..09eee17 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -811,6 +811,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
>                         input_event(wd->input, EV_KEY, BTN_RIGHT,
>                                         !!(data[1] & 0x02));
>                         input_sync(wd->input);
> +                       return 0;
>                 } else {
>                         if (size < 21)
>                                 return 1;
> --
> 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 2/3] HID: logitech-{dj,hidpp}: check report length

[Peter Wu]

On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> > Malicious USB devices can send bogus reports smaller than the expected
> > buffer size. Ensure that the length is valid to avoid reading out of
> > bounds.
> >
> > For the old WTP, I do not have a HID descriptor so just check for the
> > minimum length in hidpp_raw_event (this can be changed to an inequality
> > later).
> 
> Actually you have it :)
> All the DJ devices share the same report descriptors as they are
> provided by hid-logitech-dj :)

I see, I thought it was read from the hardware, but that probably
applies to the other interfaces. Looks like the report should have a
length of (16 + 12 * 2 + 8 + 8) / 8 = 7 bytes, correct?

> Anyway, the problem here would be with the bluetooth touchpad T651
> which sends its raw events over teh mouse (0x02) collection (hint:
> there is a "< 21" in wtp_raw_event :-P )

Huh, how can that be allowed if the mouse descriptor accept less? Does
the bluetooth layer pad the report somehow?

> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> > Hi,
> >
> > If you know that the WTP report (ID 2) has a length of 2, then you can change
> > "<" to "!=" and remove the paragraph from the commit message.
> 
> "<" should be kept for the reason above.
> 
> >
> > Kind regards,
> > Peter
> > ---
> >  drivers/hid/hid-logitech-dj.c    | 16 +++++++++++++++-
> >  drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index c917ab6..5bc6d80 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> >
> >         switch (data[0]) {
> >         case REPORT_ID_DJ_SHORT:
> > +               if (size != DJREPORT_SHORT_LENGTH) {
> > +                       dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
> > +                       return false;
> > +               }
> >                 return logi_dj_dj_event(hdev, report, data, size);
> >         case REPORT_ID_HIDPP_SHORT:
> > -               /* intentional fallthrough */
> > +               if (size != HIDPP_REPORT_SHORT_LENGTH) {
> > +                       dev_err(&hdev->dev,
> > +                               "Short HID++ report of bad size (%d)", size);
> > +                       return false;
> > +               }
> > +               return logi_dj_hidpp_event(hdev, report, data, size);
> >         case REPORT_ID_HIDPP_LONG:
> > +               if (size != HIDPP_REPORT_LONG_LENGTH) {
> > +                       dev_err(&hdev->dev,
> > +                               "Long HID++ report of bad size (%d)", size);
> > +                       return false;
> > +               }
> 
> This hunk is good to me.
> 
> >                 return logi_dj_hidpp_event(hdev, report, data, size);
> >         }
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index ae23dec..2315358 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
> >                         return 1;
> >                 }
> >                 return hidpp_raw_hidpp_event(hidpp, data, size);
> > +       case 0x02:
> > +               if (size < 2) {
> > +                       hid_err(hdev, "Received HID report of bad size (%d)",
> > +                               size);
> > +                       return 1;
> > +               }
> > +               if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > +                       return wtp_raw_event(hdev, data, size);
> > +               return 1;
> >         }
> >
> > -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > -               return wtp_raw_event(hdev, data, size);
> 
> This one is OK, but I don't like it.
> 
> wtp_raw_event also expects long hid++ reports, and I'd prefer having
> the raw_event() callback first checking on the generic hid++ reports,
> and then addressing the various subclasses of devices.
> After a better look at the code, it occurs that the actual code is
> already pretty messed up.
> wtp_raw_event() is called both in the generic hidpp_raw_event() and in
> the specific hidpp_raw_hidpp_event().
> This is IMO a design flaw and it should be fixed in a better way.
> 
> I'd better have:
> 
> - A check on the report size
> - A call to the specific hidpp_raw_hidpp_event()
> - if the previous does not return 1 (consumed event), then check on
> all subclasses and call their specific raw_event.
> 
> Does that make sense?
> 
> If you agree, you can split the patch in 3, one for the -dj, one for
> the -hidpp checks, and one for the redesign. I'd be happy to make the
> redesign if you do not want to reshuffle it in a third patch.

wtp_raw_event got called earlier through the long HID++ report handler
(which returns, so it cannot be called twice?). It looked surprising at
first, so it makes sense to split it up. I'll send a V2 for this patch
(leaving the other ones in this bundle untouched).

Kind regards,
Peter

PS. I saw a mail on LKML from a maintainer who was not so happy with the
timing of patches. If my patch submissions are at the wrong moment,
please let me know.

>
> Cheers,
> Benjamin
> 
> > -
> >         return 0;
> >  }
> >
> > --
> > 2.1.3

Re: [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length

[Benjamin Tissoires]

On Tue, Dec 16, 2014 at 10:20 AM, Peter Wu <peter@lekensteyn.nl> wrote:
> On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote:
>> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
>> > Malicious USB devices can send bogus reports smaller than the expected
>> > buffer size. Ensure that the length is valid to avoid reading out of
>> > bounds.
>> >
>> > For the old WTP, I do not have a HID descriptor so just check for the
>> > minimum length in hidpp_raw_event (this can be changed to an inequality
>> > later).
>>
>> Actually you have it :)
>> All the DJ devices share the same report descriptors as they are
>> provided by hid-logitech-dj :)
>
> I see, I thought it was read from the hardware, but that probably
> applies to the other interfaces. Looks like the report should have a
> length of (16 + 12 * 2 + 8 + 8) / 8 = 7 bytes, correct?

Well, if you count the report ID, you get 8 :)
(it's easier to just plug a DJ mouse and look for the incoming report ;-P )

>
>> Anyway, the problem here would be with the bluetooth touchpad T651
>> which sends its raw events over teh mouse (0x02) collection (hint:
>> there is a "< 21" in wtp_raw_event :-P )
>
> Huh, how can that be allowed if the mouse descriptor accept less? Does
> the bluetooth layer pad the report somehow?

2 things actually:
- the bluetooth T651 connects through hidp directly (the bluetooth hid
stack), and it has its own report descriptor which contains the vendor
defined extra data in the mouse collection.
- look at the magic mouse report descriptor, apple does not even
announce the raw report ID, so from time to time, vendor do _really_
ugly things :)

>
>> >
>> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
>> > ---
>> > Hi,
>> >
>> > If you know that the WTP report (ID 2) has a length of 2, then you can change
>> > "<" to "!=" and remove the paragraph from the commit message.
>>
>> "<" should be kept for the reason above.
>>
>> >
>> > Kind regards,
>> > Peter
>> > ---
>> >  drivers/hid/hid-logitech-dj.c    | 16 +++++++++++++++-
>> >  drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
>> >  2 files changed, 24 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> > index c917ab6..5bc6d80 100644
>> > --- a/drivers/hid/hid-logitech-dj.c
>> > +++ b/drivers/hid/hid-logitech-dj.c
>> > @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
>> >
>> >         switch (data[0]) {
>> >         case REPORT_ID_DJ_SHORT:
>> > +               if (size != DJREPORT_SHORT_LENGTH) {
>> > +                       dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
>> > +                       return false;
>> > +               }
>> >                 return logi_dj_dj_event(hdev, report, data, size);
>> >         case REPORT_ID_HIDPP_SHORT:
>> > -               /* intentional fallthrough */
>> > +               if (size != HIDPP_REPORT_SHORT_LENGTH) {
>> > +                       dev_err(&hdev->dev,
>> > +                               "Short HID++ report of bad size (%d)", size);
>> > +                       return false;
>> > +               }
>> > +               return logi_dj_hidpp_event(hdev, report, data, size);
>> >         case REPORT_ID_HIDPP_LONG:
>> > +               if (size != HIDPP_REPORT_LONG_LENGTH) {
>> > +                       dev_err(&hdev->dev,
>> > +                               "Long HID++ report of bad size (%d)", size);
>> > +                       return false;
>> > +               }
>>
>> This hunk is good to me.
>>
>> >                 return logi_dj_hidpp_event(hdev, report, data, size);
>> >         }
>> >
>> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> > index ae23dec..2315358 100644
>> > --- a/drivers/hid/hid-logitech-hidpp.c
>> > +++ b/drivers/hid/hid-logitech-hidpp.c
>> > @@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
>> >                         return 1;
>> >                 }
>> >                 return hidpp_raw_hidpp_event(hidpp, data, size);
>> > +       case 0x02:
>> > +               if (size < 2) {
>> > +                       hid_err(hdev, "Received HID report of bad size (%d)",
>> > +                               size);
>> > +                       return 1;
>> > +               }
>> > +               if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>> > +                       return wtp_raw_event(hdev, data, size);
>> > +               return 1;
>> >         }
>> >
>> > -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>> > -               return wtp_raw_event(hdev, data, size);
>>
>> This one is OK, but I don't like it.
>>
>> wtp_raw_event also expects long hid++ reports, and I'd prefer having
>> the raw_event() callback first checking on the generic hid++ reports,
>> and then addressing the various subclasses of devices.
>> After a better look at the code, it occurs that the actual code is
>> already pretty messed up.
>> wtp_raw_event() is called both in the generic hidpp_raw_event() and in
>> the specific hidpp_raw_hidpp_event().
>> This is IMO a design flaw and it should be fixed in a better way.
>>
>> I'd better have:
>>
>> - A check on the report size
>> - A call to the specific hidpp_raw_hidpp_event()
>> - if the previous does not return 1 (consumed event), then check on
>> all subclasses and call their specific raw_event.
>>
>> Does that make sense?
>>
>> If you agree, you can split the patch in 3, one for the -dj, one for
>> the -hidpp checks, and one for the redesign. I'd be happy to make the
>> redesign if you do not want to reshuffle it in a third patch.
>
> wtp_raw_event got called earlier through the long HID++ report handler
> (which returns, so it cannot be called twice?). It looked surprising at

Yeah, that's what I was referring to when I said I badly designed this.

> first, so it makes sense to split it up. I'll send a V2 for this patch
> (leaving the other ones in this bundle untouched).

OK, thanks for that. I'll check on the rest after your series.

>
> Kind regards,
> Peter
>
> PS. I saw a mail on LKML from a maintainer who was not so happy with the
> timing of patches. If my patch submissions are at the wrong moment,
> please let me know.

This is my personal opinion and Jiri can say something different. I
tend not to send big patches while there is a window opened. Sometimes
Jiri has the time to get through them, sometime he does not.
In this case, I think the patches you sent should be in the bugs fixes
categories, and, IMO should make into 3.19-rc1 or 3.19-rc2 (especially
the length check which could lead to CVEs if not tackled soon enough).
For these kind of things there is no timing, and the sooner the
better.
That being said, make sure that you keep track of those patches in
case they get lost for obvious reasons and be prepared to remind about
them if they do not make their way in Jiri's tree.

Jiri, comments?

Cheers,
Benjamin

>
>>
>> Cheers,
>> Benjamin
>>
>> > -
>> >         return 0;
>> >  }
>> >
>> > --
>> > 2.1.3
>

[PATCH v2 1/3] HID: logitech-dj: check report length

[Peter Wu]

Malicious USB devices can send bogus reports smaller than the expected
buffer size. Ensure that the length is valid to avoid reading out of
bounds.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
 v2: splitted original report length check patch
---
 drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index c917ab6..5bc6d80 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
 
 	switch (data[0]) {
 	case REPORT_ID_DJ_SHORT:
+		if (size != DJREPORT_SHORT_LENGTH) {
+			dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
+			return false;
+		}
 		return logi_dj_dj_event(hdev, report, data, size);
 	case REPORT_ID_HIDPP_SHORT:
-		/* intentional fallthrough */
+		if (size != HIDPP_REPORT_SHORT_LENGTH) {
+			dev_err(&hdev->dev,
+				"Short HID++ report of bad size (%d)", size);
+			return false;
+		}
+		return logi_dj_hidpp_event(hdev, report, data, size);
 	case REPORT_ID_HIDPP_LONG:
+		if (size != HIDPP_REPORT_LONG_LENGTH) {
+			dev_err(&hdev->dev,
+				"Long HID++ report of bad size (%d)", size);
+			return false;
+		}
 		return logi_dj_hidpp_event(hdev, report, data, size);
 	}
 
-- 
2.1.3

[PATCH v2 2/3] HID: logitech-hidpp: check WTP report length

[Peter Wu]

Malicious USB devices can send bogus reports smaller than the expected
buffer size. Ensure that the length for WTP reports is valid to avoid
reading out of bounds.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
 v2: splitted original report length check patch
---
 drivers/hid/hid-logitech-hidpp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b32f751..2f1b0ac 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -805,6 +805,11 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
 
 	switch (data[0]) {
 	case 0x02:
+		if (size < 2) {
+			hid_err(hdev, "Received HID report of bad size (%d)",
+				size);
+			return 1;
+		}
 		if (hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS) {
 			input_event(wd->input, EV_KEY, BTN_LEFT,
 					!!(data[1] & 0x01));
@@ -818,6 +823,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
 			return wtp_mouse_raw_xy_event(hidpp, &data[7]);
 		}
 	case REPORT_ID_HIDPP_LONG:
+		/* size is already checked in hidpp_raw_event. */
 		if ((report->fap.feature_index != wd->mt_feature_index) ||
 		    (report->fap.funcindex_clientid != EVENT_TOUCHPAD_RAW_XY))
 			return 1;
-- 
2.1.3

[PATCH v2 3/3] HID: logitech-hidpp: separate HID++ from WTP processing

[Peter Wu]

Previously wtp_raw_event would be called through
hidpp_raw_hidpp_event (for the touchpad report) and hidpp_raw_event
(for the mouse report).

This patch removes one calling surface, making a clearer distinction
between "generic HID++ processing" (matching internal reports) and
device-specific event processing.

Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
 v2: splitted original report length check patch. Restructured code.
---
 drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2f1b0ac..3dcd59c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -942,7 +942,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 
 	/*
 	 * If the mutex is locked then we have a pending answer from a
-	 * previoulsly sent command
+	 * previously sent command.
 	 */
 	if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
 		/*
@@ -973,9 +973,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		return 1;
 	}
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
-		return wtp_raw_event(hidpp->hid_dev, data, size);
-
 	return 0;
 }
 
@@ -983,7 +980,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *data, int size)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	int r = 0;
 
+	/* Generic HID++ processing. */
 	switch (data[0]) {
 	case REPORT_ID_HIDPP_LONG:
 		if (size != HIDPP_REPORT_LONG_LENGTH) {
@@ -991,16 +990,23 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 				size);
 			return 1;
 		}
-		return hidpp_raw_hidpp_event(hidpp, data, size);
+		r = hidpp_raw_hidpp_event(hidpp, data, size);
+		break;
 	case REPORT_ID_HIDPP_SHORT:
 		if (size != HIDPP_REPORT_SHORT_LENGTH) {
 			hid_err(hdev, "received hid++ report of bad size (%d)",
 				size);
 			return 1;
 		}
-		return hidpp_raw_hidpp_event(hidpp, data, size);
+		r = hidpp_raw_hidpp_event(hidpp, data, size);
+		break;
 	}
 
+	/* If no report is available for further processing, skip calling
+	 * raw_event of subclasses. */
+	if (r != 0)
+		return r;
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_raw_event(hdev, data, size);
 
-- 
2.1.3

Re: [PATCH v2 1/3] HID: logitech-dj: check report length

[Benjamin Tissoires]

On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <peter@lekensteyn.nl> wrote:
> Malicious USB devices can send bogus reports smaller than the expected
> buffer size. Ensure that the length is valid to avoid reading out of
> bounds.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
>  v2: splitted original report length check patch
> ---

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

Cheers,
Benjamin

>  drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index c917ab6..5bc6d80 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
>
>         switch (data[0]) {
>         case REPORT_ID_DJ_SHORT:
> +               if (size != DJREPORT_SHORT_LENGTH) {
> +                       dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
> +                       return false;
> +               }
>                 return logi_dj_dj_event(hdev, report, data, size);
>         case REPORT_ID_HIDPP_SHORT:
> -               /* intentional fallthrough */
> +               if (size != HIDPP_REPORT_SHORT_LENGTH) {
> +                       dev_err(&hdev->dev,
> +                               "Short HID++ report of bad size (%d)", size);
> +                       return false;
> +               }
> +               return logi_dj_hidpp_event(hdev, report, data, size);
>         case REPORT_ID_HIDPP_LONG:
> +               if (size != HIDPP_REPORT_LONG_LENGTH) {
> +                       dev_err(&hdev->dev,
> +                               "Long HID++ report of bad size (%d)", size);
> +                       return false;
> +               }
>                 return logi_dj_hidpp_event(hdev, report, data, size);
>         }
>
> --
> 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 v2 2/3] HID: logitech-hidpp: check WTP report length

[Benjamin Tissoires]

On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <peter@lekensteyn.nl> wrote:
> Malicious USB devices can send bogus reports smaller than the expected
> buffer size. Ensure that the length for WTP reports is valid to avoid
> reading out of bounds.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
>  v2: splitted original report length check patch
> ---

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

Cheers,
Benjamin

>  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index b32f751..2f1b0ac 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -805,6 +805,11 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
>
>         switch (data[0]) {
>         case 0x02:
> +               if (size < 2) {
> +                       hid_err(hdev, "Received HID report of bad size (%d)",
> +                               size);
> +                       return 1;
> +               }
>                 if (hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS) {
>                         input_event(wd->input, EV_KEY, BTN_LEFT,
>                                         !!(data[1] & 0x01));
> @@ -818,6 +823,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
>                         return wtp_mouse_raw_xy_event(hidpp, &data[7]);
>                 }
>         case REPORT_ID_HIDPP_LONG:
> +               /* size is already checked in hidpp_raw_event. */
>                 if ((report->fap.feature_index != wd->mt_feature_index) ||
>                     (report->fap.funcindex_clientid != EVENT_TOUCHPAD_RAW_XY))
>                         return 1;
> --
> 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 v2 3/3] HID: logitech-hidpp: separate HID++ from WTP processing

[Benjamin Tissoires]

On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <peter@lekensteyn.nl> wrote:
> Previously wtp_raw_event would be called through
> hidpp_raw_hidpp_event (for the touchpad report) and hidpp_raw_event
> (for the mouse report).
>
> This patch removes one calling surface, making a clearer distinction
> between "generic HID++ processing" (matching internal reports) and
> device-specific event processing.
>
> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
>  v2: splitted original report length check patch. Restructured code.
> ---

The patch is good per-se (and I tested the whole series BTW).

I just have a minor cosmetic comment.

If you feel like sending a v3, go ahead, otherwise, this patch is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

>  drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2f1b0ac..3dcd59c 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -942,7 +942,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>
>         /*
>          * If the mutex is locked then we have a pending answer from a
> -        * previoulsly sent command
> +        * previously sent command.
>          */
>         if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
>                 /*
> @@ -973,9 +973,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>                 return 1;
>         }
>
> -       if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> -               return wtp_raw_event(hidpp->hid_dev, data, size);
> -
>         return 0;
>  }
>
> @@ -983,7 +980,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
>                 u8 *data, int size)
>  {
>         struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +       int r = 0;

I'd rather have this variable named "ret".
2 reasons: that's how it is across the file, and I dislike variable
names with only one letter (except i, j, k, n, where this is obvious).

Cheers,
Benjamin

>
> +       /* Generic HID++ processing. */
>         switch (data[0]) {
>         case REPORT_ID_HIDPP_LONG:
>                 if (size != HIDPP_REPORT_LONG_LENGTH) {
> @@ -991,16 +990,23 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
>                                 size);
>                         return 1;
>                 }
> -               return hidpp_raw_hidpp_event(hidpp, data, size);
> +               r = hidpp_raw_hidpp_event(hidpp, data, size);
> +               break;
>         case REPORT_ID_HIDPP_SHORT:
>                 if (size != HIDPP_REPORT_SHORT_LENGTH) {
>                         hid_err(hdev, "received hid++ report of bad size (%d)",
>                                 size);
>                         return 1;
>                 }
> -               return hidpp_raw_hidpp_event(hidpp, data, size);
> +               r = hidpp_raw_hidpp_event(hidpp, data, size);
> +               break;
>         }
>
> +       /* If no report is available for further processing, skip calling
> +        * raw_event of subclasses. */
> +       if (r != 0)
> +               return r;
> +
>         if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>                 return wtp_raw_event(hdev, data, size);
>
> --
> 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

[PATCH 3/3] HID: logitech-hidpp: separate HID++ from WTP processing

[Peter Wu]

Previously wtp_raw_event would be called through
hidpp_raw_hidpp_event (for the touchpad report) and hidpp_raw_event
(for the mouse report).

This patch removes one calling surface, making a clearer distinction
between "generic HID++ processing" (matching internal reports) and
device-specific event processing.

Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
 v2: splitted original report length check patch. Restructured code.
 v3: renamed var r to ret for consistency, added R-b of Benjamin

Hi Benjamin,

I am in a good mood now, so here is v3!

IMO 'r' as 'ret' is common just like 'i' is short for 'index', but for
consistency sake I'll use ret here ;)

Kind regards,
Peter
---
 drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2f1b0ac..18af2de 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -942,7 +942,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 
 	/*
 	 * If the mutex is locked then we have a pending answer from a
-	 * previoulsly sent command
+	 * previously sent command.
 	 */
 	if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
 		/*
@@ -973,9 +973,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		return 1;
 	}
 
-	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
-		return wtp_raw_event(hidpp->hid_dev, data, size);
-
 	return 0;
 }
 
@@ -983,7 +980,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *data, int size)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+	int ret = 0;
 
+	/* Generic HID++ processing. */
 	switch (data[0]) {
 	case REPORT_ID_HIDPP_LONG:
 		if (size != HIDPP_REPORT_LONG_LENGTH) {
@@ -991,16 +990,23 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
 				size);
 			return 1;
 		}
-		return hidpp_raw_hidpp_event(hidpp, data, size);
+		ret = hidpp_raw_hidpp_event(hidpp, data, size);
+		break;
 	case REPORT_ID_HIDPP_SHORT:
 		if (size != HIDPP_REPORT_SHORT_LENGTH) {
 			hid_err(hdev, "received hid++ report of bad size (%d)",
 				size);
 			return 1;
 		}
-		return hidpp_raw_hidpp_event(hidpp, data, size);
+		ret = hidpp_raw_hidpp_event(hidpp, data, size);
+		break;
 	}
 
+	/* If no report is available for further processing, skip calling
+	 * raw_event of subclasses. */
+	if (ret != 0)
+		return ret;
+
 	if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
 		return wtp_raw_event(hdev, data, size);
 
-- 
2.1.3

Re: [PATCH v2 1/3] HID: logitech-dj: check report length

[Jiri Kosina]

On Tue, 16 Dec 2014, Peter Wu wrote:

> Malicious USB devices can send bogus reports smaller than the expected
> buffer size. Ensure that the length is valid to avoid reading out of
> bounds.
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
>  v2: splitted original report length check patch

Applied to for-3.19/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/3] HID: logitech-hidpp: check WTP report length

[Jiri Kosina]

On Tue, 16 Dec 2014, Peter Wu wrote:

> Malicious USB devices can send bogus reports smaller than the expected
> buffer size. Ensure that the length for WTP reports is valid to avoid
> reading out of bounds.
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
>  v2: splitted original report length check patch

Applied to for-3.19/upstream-fixes.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 3/3] HID: logitech-hidpp: separate HID++ from WTP processing

[Jiri Kosina]

On Wed, 17 Dec 2014, Peter Wu wrote:

> Previously wtp_raw_event would be called through
> hidpp_raw_hidpp_event (for the touchpad report) and hidpp_raw_event
> (for the mouse report).
> 
> This patch removes one calling surface, making a clearer distinction
> between "generic HID++ processing" (matching internal reports) and
> device-specific event processing.
> 
> Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
>  v2: splitted original report length check patch. Restructured code.
>  v3: renamed var r to ret for consistency, added R-b of Benjamin

Applied to for-3.20/logitech.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Benjamin Tissoires]

On Dec 16 2014 or thereabouts, Peter Wu wrote:
> Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> these errors too to avoid 5 second delays when the device reports an
> error. Caught by... well, a bug in the QEMU emulation of this receiver.
> 
> Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> it has no functional difference.
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---

Jiri, it looks like this one fall off from your radar.

It's not a problem per-se, I'd like to have some feedbacks from Logitech
first, but still, there is a bug and Peter fixed it :)

Cheers,
Benjamin

>  drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2f420c0..ae23dec 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -105,6 +105,7 @@ struct hidpp_device {
>  };
>  
>  
> +/* HID++ 1.0 error codes */
>  #define HIDPP_ERROR				0x8f
>  #define HIDPP_ERROR_SUCCESS			0x00
>  #define HIDPP_ERROR_INVALID_SUBID		0x01
> @@ -119,6 +120,8 @@ struct hidpp_device {
>  #define HIDPP_ERROR_REQUEST_UNAVAILABLE		0x0a
>  #define HIDPP_ERROR_INVALID_PARAM_VALUE		0x0b
>  #define HIDPP_ERROR_WRONG_PIN_CODE		0x0c
> +/* HID++ 2.0 error codes */
> +#define HIDPP20_ERROR				0xff
>  
>  static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
>  
> @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
>  	}
>  
>  	if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> -	    response->fap.feature_index == HIDPP_ERROR) {
> +	    response->rap.sub_id == HIDPP_ERROR) {
> +		ret = response->rap.params[1];
> +		dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> +		goto exit;
> +	}
> +
> +	if (response->report_id == REPORT_ID_HIDPP_LONG &&
> +	    response->fap.feature_index == HIDPP20_ERROR) {
>  		ret = response->fap.params[1];
> -		dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
> +		dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
>  		goto exit;
>  	}
>  
> @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
>  static inline bool hidpp_match_error(struct hidpp_report *question,
>  		struct hidpp_report *answer)
>  {
> -	return (answer->fap.feature_index == HIDPP_ERROR) &&
> +	return ((answer->rap.sub_id == HIDPP_ERROR) ||
> +	    (answer->fap.feature_index == HIDPP20_ERROR)) &&
>  	    (answer->fap.funcindex_clientid == question->fap.feature_index) &&
>  	    (answer->fap.params[0] == question->fap.funcindex_clientid);
>  }
> -- 
> 2.1.3
> 

Re: [PATCH 3/3] HID: logitech-hidpp: avoid unintended fall-through

[Benjamin Tissoires]

On Dec 16 2014 or thereabouts, Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> > Add a return to avoid a fall-through. Introduced in commit
> > 57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID: logitech-hidpp: add
> > support of the first Logitech Wireless Touchpad").
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> 
> This one is reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Jiri, gentle ping on this one too :) 

Cheers,
Benjamin
> 
> >  drivers/hid/hid-logitech-hidpp.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 2315358..09eee17 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -811,6 +811,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
> >                         input_event(wd->input, EV_KEY, BTN_RIGHT,
> >                                         !!(data[1] & 0x02));
> >                         input_sync(wd->input);
> > +                       return 0;
> >                 } else {
> >                         if (size < 21)
> >                                 return 1;
> > --
> > 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 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Peter Wu]

On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote:
> Hi Peter,
> 
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> > Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> > these errors too to avoid 5 second delays when the device reports an
> > error. Caught by... well, a bug in the QEMU emulation of this receiver.
> >
> > Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> > it has no functional difference.
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> 
> I'd like to have Nestor's opinion on this. I did not manage to find on
> the documentation that HID++ 2.0 Long report error code is 0xff, so
> introducing this change without Logitech's blessing would be
> unfortunate.
> I understand this will fix your qemu problem, but I am not entirely
> sure if we do not have to check on 0xff and 0x8f in both short and
> long responses.
> 
> Cheers,
> Benjamin

Hi Benjamin,

The Logitech Unifying extension for Chrome[1] is documented quite well
and contains details which were not public before (including names and
descriptions for all registers and subIDs!).

In lib/devices/HidppFap.js you can find this logic for handling HID++
2.0 messages:

    if ((reqView.getUint8(1) == rspView.getUint8(1)) //  device index
        && (reqView.getUint8(2) == rspView.getUint8(2)) // feature index
        && (reqView.getUint8(3) == rspView.getUint8(3))) // function/event ID + software ID
    {
        result.matchResult = devices.MATCH_RESULT.SUCCESS;
    } else if ((reqView.getUint8(1) == rspView.getUint8(1)) //  device index
        && (0xFF == rspView.getUint8(2)) // Hid++ 2.0 error
        && (reqView.getUint8(2) == rspView.getUint8(3)) // feature index
        && (reqView.getUint8(3) == rspView.getUint8(4))) // function/event ID + software ID
    {
        result.errCode = rspView.getUint8(5); // FAP_ERROR
        result.matchResult = devices.MATCH_RESULT.ERROR;
    }

Looks like a sufficient proof that 0xFF is the correct number to detect
HID++ 2.0 errors right?

In HID++ 1.0 devices ("rap"), 0xFF is named as "SYNC" (with no further
comments), so this will probably not trigger false positives either.

Kind regards,
Peter

 [1]: https://chrome.google.com/webstore/detail/logitech-unifying-for-chr/agpmgihmmmfkbhckmciedmhincdggomo

> >  drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 2f420c0..ae23dec 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -105,6 +105,7 @@ struct hidpp_device {
> >  };
> >
> >
> > +/* HID++ 1.0 error codes */
> >  #define HIDPP_ERROR                            0x8f
> >  #define HIDPP_ERROR_SUCCESS                    0x00
> >  #define HIDPP_ERROR_INVALID_SUBID              0x01
> > @@ -119,6 +120,8 @@ struct hidpp_device {
> >  #define HIDPP_ERROR_REQUEST_UNAVAILABLE                0x0a
> >  #define HIDPP_ERROR_INVALID_PARAM_VALUE                0x0b
> >  #define HIDPP_ERROR_WRONG_PIN_CODE             0x0c
> > +/* HID++ 2.0 error codes */
> > +#define HIDPP20_ERROR                          0xff
> >
> >  static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
> >
> > @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> >         }
> >
> >         if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > -           response->fap.feature_index == HIDPP_ERROR) {
> > +           response->rap.sub_id == HIDPP_ERROR) {
> > +               ret = response->rap.params[1];
> > +               dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > +               goto exit;
> > +       }
> > +
> > +       if (response->report_id == REPORT_ID_HIDPP_LONG &&
> > +           response->fap.feature_index == HIDPP20_ERROR) {
> >                 ret = response->fap.params[1];
> > -               dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
> > +               dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> >                 goto exit;
> >         }
> >
> > @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
> >  static inline bool hidpp_match_error(struct hidpp_report *question,
> >                 struct hidpp_report *answer)
> >  {
> > -       return (answer->fap.feature_index == HIDPP_ERROR) &&
> > +       return ((answer->rap.sub_id == HIDPP_ERROR) ||
> > +           (answer->fap.feature_index == HIDPP20_ERROR)) &&
> >             (answer->fap.funcindex_clientid == question->fap.feature_index) &&
> >             (answer->fap.params[0] == question->fap.funcindex_clientid);
> >  }
> > --
> > 2.1.3

Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Benjamin Tissoires]

On Thu, Dec 18, 2014 at 12:26 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote:
>> Hi Peter,
>>
>> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
>> > Devices speaking HID++ 2.0 report a different error code (0xff). Detect
>> > these errors too to avoid 5 second delays when the device reports an
>> > error. Caught by... well, a bug in the QEMU emulation of this receiver.
>> >
>> > Renamed fap to rap for HID++ 1.0 errors because it is more logical,
>> > it has no functional difference.
>> >
>> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
>> > ---
>>
>> I'd like to have Nestor's opinion on this. I did not manage to find on
>> the documentation that HID++ 2.0 Long report error code is 0xff, so
>> introducing this change without Logitech's blessing would be
>> unfortunate.
>> I understand this will fix your qemu problem, but I am not entirely
>> sure if we do not have to check on 0xff and 0x8f in both short and
>> long responses.
>>
>> Cheers,
>> Benjamin
>
> Hi Benjamin,
>
> The Logitech Unifying extension for Chrome[1] is documented quite well
> and contains details which were not public before (including names and
> descriptions for all registers and subIDs!).
>
> In lib/devices/HidppFap.js you can find this logic for handling HID++
> 2.0 messages:
>
>     if ((reqView.getUint8(1) == rspView.getUint8(1)) //  device index
>         && (reqView.getUint8(2) == rspView.getUint8(2)) // feature index
>         && (reqView.getUint8(3) == rspView.getUint8(3))) // function/event ID + software ID
>     {
>         result.matchResult = devices.MATCH_RESULT.SUCCESS;
>     } else if ((reqView.getUint8(1) == rspView.getUint8(1)) //  device index
>         && (0xFF == rspView.getUint8(2)) // Hid++ 2.0 error
>         && (reqView.getUint8(2) == rspView.getUint8(3)) // feature index
>         && (reqView.getUint8(3) == rspView.getUint8(4))) // function/event ID + software ID
>     {
>         result.errCode = rspView.getUint8(5); // FAP_ERROR
>         result.matchResult = devices.MATCH_RESULT.ERROR;
>     }
>
> Looks like a sufficient proof that 0xFF is the correct number to detect
> HID++ 2.0 errors right?

Cool :)

Then the patch is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin


>
> In HID++ 1.0 devices ("rap"), 0xFF is named as "SYNC" (with no further
> comments), so this will probably not trigger false positives either.
>
> Kind regards,
> Peter
>
>  [1]: https://chrome.google.com/webstore/detail/logitech-unifying-for-chr/agpmgihmmmfkbhckmciedmhincdggomo
>
>> >  drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
>> >  1 file changed, 14 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> > index 2f420c0..ae23dec 100644
>> > --- a/drivers/hid/hid-logitech-hidpp.c
>> > +++ b/drivers/hid/hid-logitech-hidpp.c
>> > @@ -105,6 +105,7 @@ struct hidpp_device {
>> >  };
>> >
>> >
>> > +/* HID++ 1.0 error codes */
>> >  #define HIDPP_ERROR                            0x8f
>> >  #define HIDPP_ERROR_SUCCESS                    0x00
>> >  #define HIDPP_ERROR_INVALID_SUBID              0x01
>> > @@ -119,6 +120,8 @@ struct hidpp_device {
>> >  #define HIDPP_ERROR_REQUEST_UNAVAILABLE                0x0a
>> >  #define HIDPP_ERROR_INVALID_PARAM_VALUE                0x0b
>> >  #define HIDPP_ERROR_WRONG_PIN_CODE             0x0c
>> > +/* HID++ 2.0 error codes */
>> > +#define HIDPP20_ERROR                          0xff
>> >
>> >  static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
>> >
>> > @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
>> >         }
>> >
>> >         if (response->report_id == REPORT_ID_HIDPP_SHORT &&
>> > -           response->fap.feature_index == HIDPP_ERROR) {
>> > +           response->rap.sub_id == HIDPP_ERROR) {
>> > +               ret = response->rap.params[1];
>> > +               dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
>> > +               goto exit;
>> > +       }
>> > +
>> > +       if (response->report_id == REPORT_ID_HIDPP_LONG &&
>> > +           response->fap.feature_index == HIDPP20_ERROR) {
>> >                 ret = response->fap.params[1];
>> > -               dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
>> > +               dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
>> >                 goto exit;
>> >         }
>> >
>> > @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
>> >  static inline bool hidpp_match_error(struct hidpp_report *question,
>> >                 struct hidpp_report *answer)
>> >  {
>> > -       return (answer->fap.feature_index == HIDPP_ERROR) &&
>> > +       return ((answer->rap.sub_id == HIDPP_ERROR) ||
>> > +           (answer->fap.feature_index == HIDPP20_ERROR)) &&
>> >             (answer->fap.funcindex_clientid == question->fap.feature_index) &&
>> >             (answer->fap.params[0] == question->fap.funcindex_clientid);
>> >  }
>> > --
>> > 2.1.3
>

Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Jiri Kosina]

On Wed, 17 Dec 2014, Benjamin Tissoires wrote:

> > Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> > these errors too to avoid 5 second delays when the device reports an
> > error. Caught by... well, a bug in the QEMU emulation of this receiver.
> > 
> > Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> > it has no functional difference.
> > 
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> 
> Jiri, it looks like this one fall off from your radar.
> 
> It's not a problem per-se, I'd like to have some feedbacks from Logitech
> first, but still, there is a bug and Peter fixed it :)

It's actually still on my radar, but that was exactly the reason I have it 
on hold, because my understanding was that you are waiting for Logitech to 
review it.

Nestor ... ?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too

[Jiri Kosina]

On Tue, 16 Dec 2014, Peter Wu wrote:

> Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> these errors too to avoid 5 second delays when the device reports an
> error. Caught by... well, a bug in the QEMU emulation of this receiver.
> 
> Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> it has no functional difference.
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>

Applied to for-3.20/logitech.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 3/3] HID: logitech-hidpp: avoid unintended fall-through

[Jiri Kosina]

On Tue, 16 Dec 2014, Peter Wu wrote:

> Add a return to avoid a fall-through. Introduced in commit
> 57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID: logitech-hidpp: add
> support of the first Logitech Wireless Touchpad").
> 
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>

Applied to for-3.19/upstream-fixes.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length

[Jiri Kosina]

On Tue, 16 Dec 2014, Benjamin Tissoires wrote:

> This is my personal opinion and Jiri can say something different. I
> tend not to send big patches while there is a window opened. Sometimes
> Jiri has the time to get through them, sometime he does not.
> In this case, I think the patches you sent should be in the bugs fixes
> categories, and, IMO should make into 3.19-rc1 or 3.19-rc2 (especially
> the length check which could lead to CVEs if not tackled soon enough).
> For these kind of things there is no timing, and the sooner the
> better.
> That being said, make sure that you keep track of those patches in
> case they get lost for obvious reasons and be prepared to remind about
> them if they do not make their way in Jiri's tree.
> 
> Jiri, comments?

I don't mind patches being sent during a merge window, it doesn't disturb 
my workflow.

But it's always good to explicitly mark patches which are bugfixes and 
should go to -rc, so that I bump up the priority for reviewing them.

Thanks,

-- 
Jiri Kosina
SUSE Labs