Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751013AbaLPPVM (ORCPT ); Tue, 16 Dec 2014 10:21:12 -0500 Received: from lekensteyn.nl ([178.21.112.251]:46559 "EHLO lekensteyn.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750791AbaLPPVK (ORCPT ); Tue, 16 Dec 2014 10:21:10 -0500 From: Peter Wu To: Benjamin Tissoires Cc: Jiri Kosina , Nestor Lopez Casado , linux-input , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length Date: Tue, 16 Dec 2014 16:20:58 +0100 Message-ID: <9001812.kk9brShn75@al> User-Agent: KMail/4.14.3 (Linux/3.17.0-rc4-custom-00168-g7ec62d4; KDE/4.14.3; x86_64; ; ) In-Reply-To: References: <1418691016-30681-1-git-send-email-peter@lekensteyn.nl> <1418691016-30681-3-git-send-email-peter@lekensteyn.nl> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Spam-Score: 0.0 (/) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote: > On Mon, Dec 15, 2014 at 7:50 PM, 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. > > > > 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 > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/