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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Peter,
On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> ---
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> > ---
>
> 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
Hi Peter,
On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> ---
This one is reviewed-by: Benjamin Tissoires <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Tue, Dec 16, 2014 at 10:20 AM, Peter Wu <[email protected]> wrote:
> On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote:
>> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
>> > ---
>> > 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
>
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 <[email protected]>
Signed-off-by: Peter Wu <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <[email protected]> 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 <[email protected]>
> ---
> v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
> v2: splitted original report length check patch
> ---
Reviewed-by: Benjamin Tissoires <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <[email protected]> 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 <[email protected]>
> ---
> v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
> v2: splitted original report length check patch
> ---
Reviewed-by: Benjamin Tissoires <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 16, 2014 at 10:55 AM, Peter Wu <[email protected]> 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 <[email protected]>
> Signed-off-by: Peter Wu <[email protected]>
> ---
> 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 <[email protected]>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
Signed-off-by: Peter Wu <[email protected]>
Reviewed-by: Benjamin Tissoires <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Peter Wu <[email protected]>
> Reviewed-by: Benjamin Tissoires <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
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
>
On Dec 16 2014 or thereabouts, Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> > ---
>
> This one is reviewed-by: Benjamin Tissoires <[email protected]>
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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote:
> Hi Peter,
>
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <[email protected]> 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 <[email protected]>
> > ---
>
> 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
On Thu, Dec 18, 2014 at 12:26 PM, Peter Wu <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> > ---
>>
>> 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 <[email protected]>
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
>
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 <[email protected]>
> > ---
>
> 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
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 <[email protected]>
Applied to for-3.20/logitech.
--
Jiri Kosina
SUSE Labs
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 <[email protected]>
Applied to for-3.19/upstream-fixes.
Thanks,
--
Jiri Kosina
SUSE Labs
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