* Re: [PATCH 1/7] HID: provide a helper for validating hid reports
From: Benjamin Tissoires @ 2013-09-09 12:33 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-2-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> Many drivers need to validate the characteristics of their HID report
> during initialization to avoid misusing the reports. This adds a common
> helper to perform validation of the report exisitng, the field existing,
> and the expected number of values within the field.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
>
> ---
Thanks for the v2.
I'm happy with this one.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 2/7] HID: zeroplus: validate output report details
From: Benjamin Tissoires @ 2013-09-09 12:36 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-3-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> The zeroplus HID driver was not checking the size of allocated values
> in fields it used. A HID device could send a malicious output report
> that would cause the driver to write beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 1442.728680] usb 1-1: New USB device found, idVendor=0c12, idProduct=0005
> ...
> [ 1466.243173] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2889
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
I don't have the report descriptor available, but the patch seems ok.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 3/7] HID: sony: validate HID output report details
From: Benjamin Tissoires @ 2013-09-09 12:39 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-4-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> This driver must validate the availability of the HID output report and
> its size before it can write LED states via buzz_set_leds(). This stops
> a heap overflow that is possible if a device provides a malicious HID
> output report:
>
> [ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
> ...
> [ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>
> CVE-2013-2890
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
The tests shows that the device is still working with the fix :)
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* HID input dealing with multiple collections?
From: Breton M. Saunders @ 2013-09-09 12:56 UTC (permalink / raw)
To: linux-input
Hello,
I've written a USB device which supports multiple input devices:
* A touch surface digitizer (following Microsoft's specification)
* A pen surface digitizer
* A mouse emulator
In windows each of these interfaces are exposed as a separate top
level collection, e.g.:
Usage_Page(Generic Desktop)
Usage(Mouse)
Collection(Application)
...
End_Collection
Usage_Page(Digitzer)
Usage(Touch_Screen)
Collection(Application)
...
End_Collection
Usage(Digitizer)
Usage(Pen)
Collection(Application)
...
End_Collection
Now this descriptor works well in Windows; all reporting channels are
exposed correctly as hid devices and mouse/multitouch/pen input all
works correctly as expected. In linux, however, this is not the case.
What I see is that if I drop out the touch/pen functionality then the
mouse support works correctly. Likewise, if I drop any pair of other
top level collections such that I have exactly one top level collection
the system works correctly.
Am I missing something here - as in: Have I made an error in my usage
of top level collections, or is linux-input's hid support incapable of
dealing with multiple top level collections?
Thanks in advance for any advice on this!
-bms20
^ permalink raw reply
* Re: [PATCH 4/7] HID: steelseries: validate output report details
From: Benjamin Tissoires @ 2013-09-09 13:02 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-5-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> steelseries HID driver to write beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410
> ...
> [ 182.050547] BUG kmalloc-256 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2891
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-steelseries.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index d164911..29f328f 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev,
> goto err_free;
> }
>
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) {
> + ret = -ENODEV;
> + goto err_free;
There is a problem here in case of a failure:
hid_parse() allocates a lot of memory and sets the flag
HID_STAT_PARSED, but we are leaving the probe() without clearing all
this stuff.
In hid_parse(), when a failure is detected, we call
hid_close_report(), so we also should close the report in the same way
here (but hid_close_report() is a static function).
Cheers,
Benjamin
> + }
> +
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> if (ret) {
> hid_err(hdev, "hw start failed\n");
> --
> 1.7.9.5
>
> --
> 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
^ permalink raw reply
* Re: [PATCH] update my generaltouch driver for linux by luosong
From: Jiri Kosina @ 2013-09-09 13:04 UTC (permalink / raw)
To: android; +Cc: linux-input, Henrik Rydberg, Benjamin Tissoires
In-Reply-To: <2013090912331785917915@generaltouch.com>
On Mon, 9 Sep 2013, android wrote:
> I am a software engineer from GeneralTouch Technology Co., Ltd.
>
> I want to add some driver patches to the linux kernel .
>
> I do these jobs in hid-ids.h and hid-multitouch.c
Adding Henrik and Benjamon to CC for the hid-multitouch driver.
> The main changes in hid driver are like those:
> (1)add our new products into kernel driver
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0102 0x0102
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0101 0x0101
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0106 0x0106
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
> (2) correct previous bug
> - MT_QUIRK_SLOT_IS_CONTACTNUMBER
> + MT_QUIRK_SLOT_IS_CONTACTID
This needs explanation / clarification in the changelog.
> the content of patch is shown below:
>
> From 5db217392e661695058606c7919be7fa6509f1e4 Mon Sep 17 00:00:00 2001
> From: luosong android@generaltouch.com
This doesn't look like a RFC-compliant from, I think.
> Date: Mon, 9 Sep 2013 02:30:10 +0800
> Subject: [PATCH] update my generaltouch driver for linux by luosong
Please insert changelog (description of the changes) and Signed-off-by:
line here, as documented in Documentation/SubmittingPatches
> ---
> drivers/hid/hid-ids.h | 5 +++++
> drivers/hid/hid-multitouch.c | 19 +++++++++++++++++--
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index ffe4c7a..ca78f09 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -332,6 +332,11 @@
> #define USB_VENDOR_ID_GENERAL_TOUCH 0x0dfc
> #define USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS 0x0003
> #define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PWT_TENFINGERS 0x0100
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0102 0x0102
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0101 0x0101
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0106 0x0106
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
>
> #define USB_VENDOR_ID_GLAB 0x06c2
> #define USB_DEVICE_ID_4_PHIDGETSERVO_30 0x0038
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index cb0e361..9558dde 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -244,12 +244,12 @@ static struct mt_class mt_classes[] = {
> { .name = MT_CLS_GENERALTOUCH_TWOFINGERS,
> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
> MT_QUIRK_VALID_IS_INRANGE |
> - MT_QUIRK_SLOT_IS_CONTACTNUMBER,
> + MT_QUIRK_SLOT_IS_CONTACTID,
> .maxcontacts = 2
> },
> { .name = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
> - MT_QUIRK_SLOT_IS_CONTACTNUMBER
> + MT_QUIRK_SLOT_IS_CONTACTID
> },
>
> { .name = MT_CLS_FLATFROG,
> @@ -1191,6 +1191,21 @@ static const struct hid_device_id mt_devices[] = {
> { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PWT_TENFINGERS) },
> + { .driver_data = MT_CLS_GENERALTOUCH_TWOFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0101) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0102) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0106) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A) },
Your mail client seems to be whitespace-corrupting patches (it ate the
tabs at least).
Could you please fix all the above and resubmit?
Thanks a lot,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Benjamin Tissoires @ 2013-09-09 13:12 UTC (permalink / raw)
To: Breton M. Saunders; +Cc: linux-input
In-Reply-To: <522DC594.2000305@brettnet.eu>
Hi,
On Mon, Sep 9, 2013 at 2:56 PM, Breton M. Saunders <bms20@brettnet.eu> wrote:
> Hello,
>
> I've written a USB device which supports multiple input devices:
> * A touch surface digitizer (following Microsoft's specification)
> * A pen surface digitizer
> * A mouse emulator
>
> In windows each of these interfaces are exposed as a separate top level
> collection, e.g.:
> Usage_Page(Generic Desktop)
> Usage(Mouse)
> Collection(Application)
> ...
> End_Collection
> Usage_Page(Digitzer)
> Usage(Touch_Screen)
> Collection(Application)
> ...
> End_Collection
> Usage(Digitizer)
> Usage(Pen)
> Collection(Application)
> ...
> End_Collection
>
> Now this descriptor works well in Windows; all reporting channels are
> exposed correctly as hid devices and mouse/multitouch/pen input all works
> correctly as expected. In linux, however, this is not the case. What I see
> is that if I drop out the touch/pen functionality then the mouse support
> works correctly. Likewise, if I drop any pair of other top level
> collections such that I have exactly one top level collection the system
> works correctly.
>
> Am I missing something here - as in: Have I made an error in my usage of
> top level collections, or is linux-input's hid support incapable of dealing
> with multiple top level collections?
If it works under Windows, there are huge chances that your report
descriptor is good.
However, under Linux, we use the report IDs to split (if required) the
different inputs.
The good thing for you is that if I understand correctly, the device
you are talking about should be handled through hid-multitouch. Since
kernel v3.10, we have introduced the support of devices showing both a
digitizer and a pen (they will show up as two input devices).
I would encourage you to test a 3.10 kernel, and/or send me some
traces of your devices by using hid-replay[1]. This way I will be able
to test your device on my laptop, and eventually figure out any
problem.
Cheers,
Benjamin
[1] http://bentiss.github.io/hid-replay-docs/
^ permalink raw reply
* Re: [PATCH 5/7] HID: LG: validate HID output report details
From: Benjamin Tissoires @ 2013-09-09 13:22 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-6-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> lg, lg3, and lg4 HID drivers to write beyond the output report allocation
> during an event, causing a heap overflow:
>
> [ 325.245240] usb 1-1: New USB device found, idVendor=046d, idProduct=c287
> ...
> [ 414.518960] BUG kmalloc-4096 (Not tainted): Redzone overwritten
>
> Additionally, while lg2 did correctly validate the report details, it was
> cleaned up and shortened.
>
> CVE-2013-2893
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
To me, the patch looks good.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 6/7] HID: lenovo-tpkbd: validate output report details
From: Benjamin Tissoires @ 2013-09-09 13:28 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-7-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> lenovo-tpkbd HID driver to write just beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 76.109807] usb 1-1: New USB device found, idVendor=17ef, idProduct=6009
> ...
> [ 80.462540] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2894
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
As mentioned by Josh on the previous thread, this patch triggers a bug
for the users:
https://bugzilla.redhat.com/show_bug.cgi?id=1003998
I am writing the comments I have given the bug:
> drivers/hid/hid-lenovo-tpkbd.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
> index 07837f5..f458e76 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo-tpkbd.c
> @@ -339,7 +339,15 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
> struct tpkbd_data_pointer *data_pointer;
> size_t name_sz = strlen(dev_name(dev)) + 16;
> char *name_mute, *name_micmute;
> - int ret;
> + int i, ret;
> +
> + /* Validate required reports. */
> + for (i = 0; i < 4; i++) {
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 4, i, 1))
The report descriptor shows that the report ID 4 is a FEATURE, not an OUTPUT.
> + return -ENODEV;
There is a problem in the probe() function when tpkbd_probe_tp()
fails: it directly bails out instead of calling hid_hw_stop(), thus,
the input devices are still there while nobody has a pointer to them.
This way, ODEBUG is triggered. But this should be address in an other
patch I would say.
Cheers,
Benjamin
> + }
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
> + return -ENODEV;
>
> if (sysfs_create_group(&hdev->dev.kobj,
> &tpkbd_attr_group_pointer)) {
> --
> 1.7.9.5
>
> --
> 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
^ permalink raw reply
* Re: [PATCH 7/7] HID: logitech-dj: validate output report details
From: Benjamin Tissoires @ 2013-09-09 13:44 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Jiri Kosina, Henrik Rydberg
In-Reply-To: <1378312645-27736-8-git-send-email-keescook@chromium.org>
On 04/09/13 18:37, Kees Cook wrote:
> A HID device could send a malicious output report that would cause the
> logitech-dj HID driver to leak kernel memory contents to the device, or
> trigger a NULL dereference during initialization:
>
> [ 304.424553] usb 1-1: New USB device found, idVendor=046d, idProduct=c52b
> ...
> [ 304.780467] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
> [ 304.781409] IP: [<ffffffff815d50aa>] logi_dj_recv_send_report.isra.11+0x1a/0x90
>
> CVE-2013-2895
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-logitech-dj.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index cd33084..404f2d0 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -461,7 +461,7 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> struct hid_report *report;
> struct hid_report_enum *output_report_enum;
> u8 *data = (u8 *)(&dj_report->device_index);
> - int i;
> + unsigned int i, length;
>
> output_report_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
> report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
> @@ -471,7 +471,9 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> return -ENODEV;
> }
>
> - for (i = 0; i < report->field[0]->report_count; i++)
> + length = min_t(size_t, sizeof(*dj_report) - 1,
> + report->field[0]->report_count);
> + for (i = 0; i < length; i++)
I would rather simply do:
for (i = 0; i < DJREPORT_SHORT_LENGTH - 1; i++)
> report->field[0]->value[i] = data[i];
>
> hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> @@ -783,6 +785,12 @@ static int logi_dj_probe(struct hid_device *hdev,
> goto hid_parse_fail;
> }
>
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
> + 0, 3)) {
and here:
if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
0, DJREPORT_SHORT_LENGTH - 1)) {
3 seems to come from nowhere, and is also wrong because in logi_dj_recv_switch_to_dj_mode(), we write 4 bytes.
Besides this small nitpick, there is the same problem that I spotted in hid-steelseries, in case of a failure, the hid device is not cleaned correctly.
Cheers,
Benjamin
> + retval = -ENODEV;
> + goto hid_parse_fail;
> + }
> +
> /* Starts the usb device and connects to upper interfaces hiddev and
> * hidraw */
> retval = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>
^ permalink raw reply
* Re: [PATCH v2 0/7] HID: validate report details
From: Benjamin Tissoires @ 2013-09-09 13:48 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg
In-Reply-To: <1378312645-27736-1-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> These patches introduce a validation function for HID devices that do
> direct report value accesses, solving a number of heap smashing flaws.
>
> This version changes to using an field-index-based checker for the new
> "hid_validate_values()" which requires callers to loop across fields if
> they use more than one field.
I am globally happy with the patch series.
I have some concerns about patches 4 6 and 7, but the other can be
applied right now.
Kees, if you want to switch to something else, I can handle the v3 for
these three patches: I have some logitech-dj devices and a tester for
the lenovo one.
Cheers,
Benjamin
>
> -Kees
>
> --
> 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
^ permalink raw reply
* Re: [PATCH 11/14] HID: multitouch: validate feature report details
From: Benjamin Tissoires @ 2013-09-09 13:50 UTC (permalink / raw)
To: Kees Cook; +Cc: Benjamin Tissoires, Jiri Kosina, linux-input, Henrik Rydberg
In-Reply-To: <CAGXu5j+LHg-kFBXRzXfpLmztHjVCn6jiKkX5w1x3-a4is_RLqw@mail.gmail.com>
On Wed, Sep 4, 2013 at 6:31 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Aug 30, 2013 at 11:27 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires
>> <benjamin.tissoires@gmail.com> wrote:
>>> On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires
>>>> <benjamin.tissoires@redhat.com> wrote:
>>>>> Hi Kees,
>>>>>
>>>>> I would be curious to have the HID report descriptors (maybe off list)
>>>>> to understand how things can be that bad.
>>>>
>>>> Certainly! I'll send them your way. I did have to get pretty creative
>>>> to tickle these conditions.
>>>>
>>>>> On overall, I'd prefer all those checks to be in hid-core so that we
>>>>> have the guarantee that we don't have to open a new CVE each time a
>>>>> specific hid driver do not check for these ranges.
>>>>
>>>> I pondered doing this, but it seemed like something that needed wider
>>>> discussion, so I thought I'd start with just the dump of fixes. It
>>>> seems like the entire HID report interface should use access functions
>>>> to enforce range checking -- perhaps further enforced by making the
>>>> structure opaque to the drivers.
>>>
>>> The problem with access functions with range checking is when they are
>>> used at each input report. This will overload the kernel processing
>>> for static checks.
>>>
>>>>
>>>>> More specific comments inlined:
>>>>>
>>>>> On 28/08/13 22:31, Jiri Kosina wrote:
>>>>>> From: Kees Cook <keescook@chromium.org>
>>>>>>
>>>>>> When working on report indexes, always validate that they are in bounds.
>>>>>> Without this, a HID device could report a malicious feature report that
>>>>>> could trick the driver into a heap overflow:
>>>>>>
>>>>>> [ 634.885003] usb 1-1: New USB device found, idVendor=0596, idProduct=0500
>>>>>> ...
>>>>>> [ 676.469629] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>>>>>>
>>>>>> CVE-2013-2897
>>>>>>
>>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>>> Cc: stable@kernel.org
>>>>>> ---
>>>>>> drivers/hid/hid-multitouch.c | 25 ++++++++++++++++++++-----
>>>>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>>>>>> index cb0e361..2aa275e 100644
>>>>>> --- a/drivers/hid/hid-multitouch.c
>>>>>> +++ b/drivers/hid/hid-multitouch.c
>>>>>> @@ -330,9 +330,18 @@ static void mt_feature_mapping(struct hid_device *hdev,
>>>>>> break;
>>>>>> }
>>>>>> }
>>>>>> + /* Ignore if value index is out of bounds. */
>>>>>> + if (td->inputmode_index < 0 ||
>>>>>
>>>>> td->inputmode_index can not be less than 0
>>>>
>>>> Well, it certainly _shouldn't_ be less than zero. :) However, it is
>>>> defined as s8, and gets set from an int:
>>>>
>>>> for (i=0; i < field->maxusage; i++) {
>>>> if (field->usage[i].hid == usage->hid) {
>>>> td->inputmode_index = i;
>>>> break;
>>>> }
>>>> }
>>>>
>>>> Both "i" and "maxusage" are int, and I can generate a large maxusage
>>>> and usage array where the first matching hid equality happens when i
>>>> is >127, causing inputmode_index to wrap.
>>>
>>> ouch. Sorry. This piece of code (the current code) is junk. We should
>>> switch to usage->usage_index and change from __s8 to __s16 the
>>> declaration ASAP.
>>>
>>>>
>>>>>> + td->inputmode_index >= field->report_count) {
>>>>>
>>>>> if this is really required, we could just change the for loop above to
>>>>> go from 0 to field->report_count instead.
>>>>
>>>> That's certainly true, but since usage count and report count are not
>>>> directly associated, I don't know if there are devices that will freak
>>>> out with this restriction.
>>>
>>> Actually, I just again another long time understanding all the mess of
>>> having usage count and report count different in hid-core.
>>>
>>> I think it is a bug at some point (but it looks like I tend to be
>>> wrong on a lot of things recently :-P ), because when you look at the
>>> actual code of hid_input_field() in hid-core.c, we never go further
>>> than field->report_count when dealing with input report.
>>> So my guess is that we are declaring extra usages that are never used
>>> to parse incoming data.
>>>
>>>>
>>>>> However, I think we could just rely on usage->usage_index to get the
>>>>> actual index.
>>>>> I'll do more tests today and tell you later.
>>>>>
>>>>>> + dev_err(&hdev->dev, "HID_DG_INPUTMODE out of range\n");
>>>>>> + td->inputmode = -1;
>>>>>> + }
>>>>>>
>>>>>> break;
>>>>>> case HID_DG_CONTACTMAX:
>>>>>> + /* Ignore if value count is out of bounds. */
>>>>>> + if (field->report_count < 1)
>>>>>> + break;
>>>>>
>>>>> If you can trigger this, I would say that the fix should be in hid-core,
>>>>> not in every driver. A null report_count should not be allowed by the
>>>>> HID protocol.
>>>>
>>>> Again, I have no problem with that idea, but there seem to be lots of
>>>> general cases where this may not be possible (i.e. a HID with 0 OUTPUT
>>>> or FEATURE reports). I didn't see a sensible way to approach this in
>>>> core without declaring a "I require $foo many OUTPUT reports, $bar
>>>> many INPUT, and $baz many FEATURE" for each driver. I instead opted
>>>> for the report validation function instead.
>>>>
>>>
>>> I think we can just add this check in both hidinput_configure_usage()
>>> and report_features() (both in hid-input.c) so that we don't call the
>>> *_mapping() callbacks with non sense fields.
>
> Can you suggest a patch for this? (And please include reference to
> CVE-2013-2897 in the commit.)
Sure, I'll do that.
Cheers,
Benjamin
>
> I'll send a v2 of the rest of the series.
>
> Thanks!
>
> -Kees
>
>>> This way, the specific hid drivers will know only the correct fields,
>>> and don't need to check this. So patches 9, 11 and 13 can be amended.
>>>
>>> It looks to me that you mismatched field->report_count and the actual
>>> report_count in your answer: field->report_count is the number of
>>> consecutive fields of field->report_size that are present for the
>>> parsing of the report. It has nothing to do with the total report
>>> count.
>>>
>>>>>> td->maxcontact_report_id = field->report->id;
>>>>>> td->maxcontacts = field->value[0];
>>>>>> if (!td->maxcontacts &&
>>>>>> @@ -743,15 +752,21 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
>>>>>> unsigned count;
>>>>>> int r, n;
>>>>>>
>>>>>> + if (report->maxfield == 0)
>>>>>> + return;
>>>>>> +
>>>>>> /*
>>>>>> * Includes multi-packet support where subsequent
>>>>>> * packets are sent with zero contactcount.
>>>>>> */
>>>>>> - if (td->cc_index >= 0) {
>>>>>> - struct hid_field *field = report->field[td->cc_index];
>>>>>> - int value = field->value[td->cc_value_index];
>>>>>> - if (value)
>>>>>> - td->num_expected = value;
>>>>>> + if (td->cc_index >= 0 && td->cc_index < report->maxfield) {
>>>>>> + field = report->field[td->cc_index];
>>>>>
>>>>> looks like we previously overwrote the definition of field :(
>>>>>
>>>>>> + if (td->cc_value_index >= 0 &&
>>>>>> + td->cc_value_index < field->report_count) {
>>>>>> + int value = field->value[td->cc_value_index];
>>>>>> + if (value)
>>>>>> + td->num_expected = value;
>>>>>> + }
>>>>>
>>>>> I can not see why td->cc_index and td->cc_value_index could have bad
>>>>> values. They are initially created by hid-core, and are not provided by
>>>>> the device.
>>>>> Anyway, if you are able to produce bad values with them, I'd rather have
>>>>> those checks during the assignment of td->cc_index (in
>>>>> mt_touch_input_mapping()), instead of checking them for each report.
>>>>
>>>> Well, the problem comes again from there not being a hard link between
>>>> the usage array and the value array:
>>>>
>>>> td->cc_value_index = usage->usage_index;
>>>> ...
>>>> int value = field->value[td->cc_value_index];
>>>>
>>>> And all of this would be irrelevant if there was an access function
>>>> for field->value[].
>>>
>>> True, with the current implementation, td->cc_value_index can be
>>> greater than field->report_count. Still, moving this check in
>>> mt_touch_input_mapping() will allow us to make it only once.
>>>
>>>>
>>>> Thanks for the review!
>>>
>>> you are welcome :)
>>
>> Okay, so, where does the whole patch series stand? It seems like the
>> checks are all worth adding; and my fixes are, I think, "minimal" so
>> having them go into the tree (so that they land in stable) seems like
>> a good idea. The work beyond that could be done on top of the existing
>> patches? There seems to be a lot of redesign ideas, but those probably
>> aren't so great for stable, and I'm probably not the best person to
>> write them, since everything I know about HID code I learned two weeks
>> ago. :)
>>
>> Given the 12 flaws, what do you see as the best way forward?
>
> --
> Kees Cook
> Chrome OS Security
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Breton M. Saunders @ 2013-09-09 14:01 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <CAN+gG=ELptZsD37P9XMAgbLCFt-yZVbgmbGrk+mLvT6G2raKLQ@mail.gmail.com>
On 09/09/13 14:12, Benjamin Tissoires wrote:
> Hi,
>
> On Mon, Sep 9, 2013 at 2:56 PM, Breton M. Saunders <bms20@brettnet.eu> wrote:
>> Hello,
>>
>> I've written a USB device which supports multiple input devices:
>> * A touch surface digitizer (following Microsoft's specification)
>> * A pen surface digitizer
>> * A mouse emulator
>>
>> In windows each of these interfaces are exposed as a separate top level
>> collection, e.g.:
>> Usage_Page(Generic Desktop)
>> Usage(Mouse)
>> Collection(Application)
>> ...
>> End_Collection
>> Usage_Page(Digitzer)
>> Usage(Touch_Screen)
>> Collection(Application)
>> ...
>> End_Collection
>> Usage(Digitizer)
>> Usage(Pen)
>> Collection(Application)
>> ...
>> End_Collection
>>
>> Now this descriptor works well in Windows; all reporting channels are
>> exposed correctly as hid devices and mouse/multitouch/pen input all works
>> correctly as expected. In linux, however, this is not the case. What I see
>> is that if I drop out the touch/pen functionality then the mouse support
>> works correctly. Likewise, if I drop any pair of other top level
>> collections such that I have exactly one top level collection the system
>> works correctly.
>>
>> Am I missing something here - as in: Have I made an error in my usage of
>> top level collections, or is linux-input's hid support incapable of dealing
>> with multiple top level collections?
> If it works under Windows, there are huge chances that your report
> descriptor is good.
>
> However, under Linux, we use the report IDs to split (if required) the
> different inputs.
> The good thing for you is that if I understand correctly, the device
> you are talking about should be handled through hid-multitouch. Since
> kernel v3.10, we have introduced the support of devices showing both a
> digitizer and a pen (they will show up as two input devices).
>
> I would encourage you to test a 3.10 kernel, and/or send me some
> traces of your devices by using hid-replay[1]. This way I will be able
> to test your device on my laptop, and eventually figure out any
> problem.
>
> Cheers,
Hi Benjamin,
Thanks for the quick reply.
3.10 is a no-go for me; I need to get this working on a vanilla (or
upgraded) version of Ubuntu 12.04; so something more like kernel version
3.2. (Unfortunately, its what people are using in the field).
I will, however, install a more modern kernel to test against; esp.
since I note your HID-replay tool requires 3.6 to test with.
Would the correct approach here be to implement a composite USB
device and have separate USB interfaces for mouse, touch and pen for
compatibility with older kernel revisions?
Cheers,
-Brett
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Benjamin Tissoires @ 2013-09-09 14:16 UTC (permalink / raw)
To: Breton M. Saunders; +Cc: linux-input
In-Reply-To: <522DD4AA.3020501@brettnet.eu>
On Mon, Sep 9, 2013 at 4:01 PM, Breton M. Saunders <bms20@brettnet.eu> wrote:
> On 09/09/13 14:12, Benjamin Tissoires wrote:
>>
>> Hi,
>>
>> On Mon, Sep 9, 2013 at 2:56 PM, Breton M. Saunders <bms20@brettnet.eu>
>> wrote:
>>>
>>> Hello,
>>>
>>> I've written a USB device which supports multiple input devices:
>>> * A touch surface digitizer (following Microsoft's specification)
>>> * A pen surface digitizer
>>> * A mouse emulator
>>>
>>> In windows each of these interfaces are exposed as a separate top
>>> level
>>> collection, e.g.:
>>> Usage_Page(Generic Desktop)
>>> Usage(Mouse)
>>> Collection(Application)
>>> ...
>>> End_Collection
>>> Usage_Page(Digitzer)
>>> Usage(Touch_Screen)
>>> Collection(Application)
>>> ...
>>> End_Collection
>>> Usage(Digitizer)
>>> Usage(Pen)
>>> Collection(Application)
>>> ...
>>> End_Collection
>>>
>>> Now this descriptor works well in Windows; all reporting channels are
>>> exposed correctly as hid devices and mouse/multitouch/pen input all works
>>> correctly as expected. In linux, however, this is not the case. What I
>>> see
>>> is that if I drop out the touch/pen functionality then the mouse support
>>> works correctly. Likewise, if I drop any pair of other top level
>>> collections such that I have exactly one top level collection the system
>>> works correctly.
>>>
>>> Am I missing something here - as in: Have I made an error in my usage
>>> of
>>> top level collections, or is linux-input's hid support incapable of
>>> dealing
>>> with multiple top level collections?
>>
>> If it works under Windows, there are huge chances that your report
>> descriptor is good.
>>
>> However, under Linux, we use the report IDs to split (if required) the
>> different inputs.
>> The good thing for you is that if I understand correctly, the device
>> you are talking about should be handled through hid-multitouch. Since
>> kernel v3.10, we have introduced the support of devices showing both a
>> digitizer and a pen (they will show up as two input devices).
>>
>> I would encourage you to test a 3.10 kernel, and/or send me some
>> traces of your devices by using hid-replay[1]. This way I will be able
>> to test your device on my laptop, and eventually figure out any
>> problem.
>>
>> Cheers,
>
> Hi Benjamin,
>
> Thanks for the quick reply.
>
> 3.10 is a no-go for me; I need to get this working on a vanilla (or
> upgraded) version of Ubuntu 12.04; so something more like kernel version
> 3.2. (Unfortunately, its what people are using in the field).
You can still try to use my backport available here:
https://github.com/bentiss/hid-multitouch
it _should_ work on a 3.2. You only have to add your VID/PID in
hid-multitouch to make it work.
>
> I will, however, install a more modern kernel to test against; esp. since
> I note your HID-replay tool requires 3.6 to test with.
well, it requires the module uhid, which is available in 3.6, but also
in my backport :)
run "modprobe uhid_compat" and run hid-replay against the
/dev/hidraw-compatN device.
Anyway, if this is too much of a pain, just send me the report
descriptor in hexadecimal (and some hexadecimal captures of your
events), I'll do the formatting myself.
>
> Would the correct approach here be to implement a composite USB device and
> have separate USB interfaces for mouse, touch and pen for compatibility with
> older kernel revisions?
I would say no because I don't know what would be the behavior of
Windows. But if you don't care about Windows and if this solution
works, it's entirely up to you.
Cheers,
Benjamin
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Breton M. Saunders @ 2013-09-09 15:40 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <CAN+gG=F9OmeXM_s_MX1_q+H=hAwZB518Lx_6FZd42Z8PX=SpJw@mail.gmail.com>
HI Benjamin,
> You can still try to use my backport available here:
> https://github.com/bentiss/hid-multitouch
Great - thanks for that! It may be a solution.
>
> it _should_ work on a 3.2. You only have to add your VID/PID in
> hid-multitouch to make it work.
Unfortunately not. It seems that writes on the device to the interrupt
endpoint never complete; I haven't had the USB analyzer on this yet, but
I suspect that the host running kernel 3.2 is not polling the endpoint.
I did upgrade my Mint13 box to Ubuntu's latest kernel 3.11; and the
multitouch support looks pretty good. It worked out of the box.
However, the mouse channel support isn't working - if I cat /dev/mouse1
or /dev/mouse2 I see no data coming over when I report mouse events from
my device.
>
>> I will, however, install a more modern kernel to test against; esp. since
>> I note your HID-replay tool requires 3.6 to test with.
> well, it requires the module uhid, which is available in 3.6, but also
> in my backport :)
> run "modprobe uhid_compat" and run hid-replay against the
> /dev/hidraw-compatN device.
>
> Anyway, if this is too much of a pain, just send me the report
> descriptor in hexadecimal (and some hexadecimal captures of your
> events), I'll do the formatting myself.
Let me check if I can do this; I may get permission to open up the
descriptors too.
>> Would the correct approach here be to implement a composite USB device and
>> have separate USB interfaces for mouse, touch and pen for compatibility with
>> older kernel revisions?
> I would say no because I don't know what would be the behavior of
> Windows. But if you don't care about Windows and if this solution
> works, it's entirely up to you.
>
I may have to resort to that approach. I believe Microsoft added the
mouse top level collection to support windows xp; so it may also just
work out of the box if a separate mouse appears on the same interface.
Cheers,
-Brett
^ permalink raw reply
* [BUG?] HID: uhid: add devname module alias
From: Tom Gundersen @ 2013-09-09 15:43 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Jiri Kosina, David Herrmann, Kay Sievers,
linux-input@vger.kernel.org, LKML
Hi Marcel,
The above commit (60cbd53 in mainline) doesn't appear to work for me.
I.e., depmod does not create an entry in modules.devname and hence no
device node is created on boot.
If I understand correctly, you'd also need to create the correct
"char-major-<MAJOR>-<MINOR>" alias (which I don't think you can do as
long as you are using MISC_DYNAMIC_MINOR).
Or am I missing something?
Cheers,
Tom
^ permalink raw reply
* Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Dmitry Torokhov @ 2013-09-09 16:12 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <522C5F96.20001-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, Sep 08, 2013 at 12:29:26PM +0100, Jonathan Cameron wrote:
> On 09/01/13 12:17, Zubair Lutfullah wrote:
> > Enable shared IRQ to allow ADC to share IRQ line from
> > parent MFD core. Only FIFO0 IRQs are for TSC and handled
> > on the TSC side.
> >
> > Step mask would be updated from cached variable only previously.
> > In rare cases when both TSC and ADC are used, the cached
> > variable gets mixed up.
> > The step mask is written with the required mask every time.
> >
> > Rachna Patil (TI) laid ground work for shared IRQ.
> >
> > Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Whilst I would have prefered an mfd under these drivers and elegant handling
> of the interrupts, I guess if this works it is at least not terribly invasive.
>
> However, this does need an Ack from Dmitry before I can take it (or for
> Dmitry to take it himself?)
I completely agree with Jonathan, more elegant handling would be nice
but current one will do for now.
Since most of the work on the driver is going through IIO tree I think
it would make sense for this patch to go through it as well.
Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: ALPS Touchpad- Improve the performance of alps v5-protocol's touchpad
From: Dmitry Torokhov @ 2013-09-09 16:26 UTC (permalink / raw)
To: yunkang.tang@cn.alps.com; +Cc: linux-input@vger.kernel.org
In-Reply-To: <89B48266C4EEC447B3A3A031C8DE21870263AF88@SIXPRD0410MB394.apcprd04.prod.outlook.com>
Hi Tommy,
On Thu, Sep 05, 2013 at 05:50:48AM +0000, yunkang.tang@cn.alps.com wrote:
> <Change list>
> - Calculate the device's dimension in alps_identify().
> - Change the logic of packet decoding.
> - Add the new data process logic.
It appears that your mailer damaged the patch converting all tabs to
single spaces. Please consider using an alternative MTA (mutt, or maybe
git send-email) to post your patches.
It also apperas that there are several changes (support for never
hardware, tweaks to the older protocols) going rolled into a single
patch, would it be possible to split them into separate patches,
please?
> - Change the dev2's name from "PS/2 Mouse" to "ALPS Touchpad".
Hmm, the secondary device is never a touchpad but either trackpoint (for
confirmed dualpoint devices) or "something" we do not quite know what.
Why are you marking it as a touchpad?
Also, you forgot to add your "Signed-off-by: ..." as per
Documentation/SubmittingPatches
Thanks!
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] Input: ALPS Touchpad- Improve the performance of alps v5-protocol's touchpad
From: Dmitry Torokhov @ 2013-09-09 16:27 UTC (permalink / raw)
To: yunkang.tang@cn.alps.com; +Cc: linux-input@vger.kernel.org
In-Reply-To: <89B48266C4EEC447B3A3A031C8DE21870263AF93@SIXPRD0410MB394.apcprd04.prod.outlook.com>
Hi Tommy,
On Thu, Sep 05, 2013 at 05:50:55AM +0000, yunkang.tang@cn.alps.com wrote:
> <Change list>
> - Add the macro definition for v5 device.
>
> --- linux-3.11/drivers/input/mouse/alps.h.orig 2013-09-04 19:51:33.837135870 +0800
> +++ linux-3.11/drivers/input/mouse/alps.h 2013-09-05 21:01:57.074314890 +0800
> @@ -18,6 +18,10 @@
> #define ALPS_PROTO_V4 4
> #define ALPS_PROTO_V5 5
>
> +#define DOLPHIN_COUNT_PER_ELECTRODE 64
> +#define DOLPHIN_PROFILE_XOFFSET 8 /* [DOLPHIN] The number of the x-electrode offset value */
> +#define DOLPHIN_PROFILE_YOFFSET 1 /* [DOLPHIN] The number of the y-electrode offset value */
> +
> /**
> * struct alps_model_info - touchpad ID table
> * @signature: E7 response string to match.
> @@ -69,7 +73,7 @@ struct alps_nibble_commands {
> * @y: Y position for ST.
> * @z: Z position for ST.
> * @first_mp: Packet is the first of a multi-packet report.
> - * @is_mp: Packet is part of a multi-packet report.
> + * @is_mp: Packet is the last of a multi-packet report.
> * @left: Left touchpad button is active.
> * @right: Right touchpad button is active.
> * @middle: Middle touchpad button is active.
> @@ -145,7 +149,7 @@ struct alps_data {
>
> int (*hw_init)(struct psmouse *psmouse);
> void (*process_packet)(struct psmouse *psmouse);
> - void (*decode_fields)(struct alps_fields *f, unsigned char *p);
> + void (*decode_fields)(struct alps_fields *f, unsigned char *p, struct psmouse *psmouse);
> void (*set_abs_params)(struct alps_data *priv, struct input_dev *dev1);
>
> int prev_fin;
Please do not separate changes in header files form changes in .c files
because patches need to be complete. If I were to apply this patch alone
it would break the compilation.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] HID: uhid: allocate static minor
From: David Herrmann @ 2013-09-09 16:33 UTC (permalink / raw)
To: linux-input
Cc: Tom Gundersen, Kay Sievers, Marcel Holtmann, linux-kernel,
David Herrmann
udev has this nice feature of creating "dead" /dev/<node> device-nodes if
it finds a devnode:<node> modalias. Once the node is accessed, the kernel
automatically loads the module that provides the node. However, this
requires udev to know the major:minor code to use for the node. This
feature was introduced by:
commit 578454ff7eab61d13a26b568f99a89a2c9edc881
Author: Kay Sievers <kay.sievers@vrfy.org>
Date: Thu May 20 18:07:20 2010 +0200
driver core: add devname module aliases to allow module on-demand auto-loading
However, uhid uses dynamic minor numbers so this doesn't actually work. We
need to load uhid to know which minor it's going to use.
Hence, allocate a static minor (just like uinput does) and we're good
to go.
Reported-by: Tom Gundersen <teg@jklm.no>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 3 ++-
include/linux/miscdevice.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 5bf2fb7..93b00d7 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -615,7 +615,7 @@ static const struct file_operations uhid_fops = {
static struct miscdevice uhid_misc = {
.fops = &uhid_fops,
- .minor = MISC_DYNAMIC_MINOR,
+ .minor = UHID_MINOR,
.name = UHID_NAME,
};
@@ -634,4 +634,5 @@ module_exit(uhid_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
MODULE_DESCRIPTION("User-space I/O driver support for HID subsystem");
+MODULE_ALIAS_MISCDEV(UHID_MINOR);
MODULE_ALIAS("devname:" UHID_NAME);
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 09c2300..cb35835 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -45,6 +45,7 @@
#define MAPPER_CTRL_MINOR 236
#define LOOP_CTRL_MINOR 237
#define VHOST_NET_MINOR 238
+#define UHID_MINOR 239
#define MISC_DYNAMIC_MINOR 255
struct device;
--
1.8.4
^ permalink raw reply related
* Re: [BUG?] HID: uhid: add devname module alias
From: David Herrmann @ 2013-09-09 16:38 UTC (permalink / raw)
To: Tom Gundersen
Cc: Marcel Holtmann, Jiri Kosina, Kay Sievers,
linux-input@vger.kernel.org, LKML
In-Reply-To: <CAG-2HqXVR5rFCa0XGmFpJcPWgPBKKaELgmFxhWKxxOYQRc-m2A@mail.gmail.com>
Hi Tom
On Mon, Sep 9, 2013 at 5:43 PM, Tom Gundersen <teg@jklm.no> wrote:
> Hi Marcel,
>
> The above commit (60cbd53 in mainline) doesn't appear to work for me.
> I.e., depmod does not create an entry in modules.devname and hence no
> device node is created on boot.
>
> If I understand correctly, you'd also need to create the correct
> "char-major-<MAJOR>-<MINOR>" alias (which I don't think you can do as
> long as you are using MISC_DYNAMIC_MINOR).
>
> Or am I missing something?
Nope, that's correct as discussed on IRC. Patch pending as:
Subject: [PATCH] HID: uhid: allocate static minor
Message-Id: <1378744434-10883-1-git-send-email-dh.herrmann@gmail.com>
Thanks
David
^ permalink raw reply
* Re: [PATCH] HID: uhid: allocate static minor
From: Tom Gundersen @ 2013-09-09 16:51 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input@vger.kernel.org, Kay Sievers, Marcel Holtmann, LKML
In-Reply-To: <1378744434-10883-1-git-send-email-dh.herrmann@gmail.com>
On Mon, Sep 9, 2013 at 6:33 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> udev has this nice feature of creating "dead" /dev/<node> device-nodes if
> it finds a devnode:<node> modalias. Once the node is accessed, the kernel
> automatically loads the module that provides the node. However, this
> requires udev to know the major:minor code to use for the node. This
> feature was introduced by:
>
> commit 578454ff7eab61d13a26b568f99a89a2c9edc881
> Author: Kay Sievers <kay.sievers@vrfy.org>
> Date: Thu May 20 18:07:20 2010 +0200
>
> driver core: add devname module aliases to allow module on-demand auto-loading
>
> However, uhid uses dynamic minor numbers so this doesn't actually work. We
> need to load uhid to know which minor it's going to use.
>
> Hence, allocate a static minor (just like uinput does) and we're good
> to go.
>
> Reported-by: Tom Gundersen <teg@jklm.no>
Thanks, this works (I only tested the creation of the device node, not
the actual module loading).
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/hid/uhid.c | 3 ++-
> include/linux/miscdevice.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 5bf2fb7..93b00d7 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -615,7 +615,7 @@ static const struct file_operations uhid_fops = {
>
> static struct miscdevice uhid_misc = {
> .fops = &uhid_fops,
> - .minor = MISC_DYNAMIC_MINOR,
> + .minor = UHID_MINOR,
> .name = UHID_NAME,
> };
>
> @@ -634,4 +634,5 @@ module_exit(uhid_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
> MODULE_DESCRIPTION("User-space I/O driver support for HID subsystem");
> +MODULE_ALIAS_MISCDEV(UHID_MINOR);
> MODULE_ALIAS("devname:" UHID_NAME);
> diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> index 09c2300..cb35835 100644
> --- a/include/linux/miscdevice.h
> +++ b/include/linux/miscdevice.h
> @@ -45,6 +45,7 @@
> #define MAPPER_CTRL_MINOR 236
> #define LOOP_CTRL_MINOR 237
> #define VHOST_NET_MINOR 238
> +#define UHID_MINOR 239
> #define MISC_DYNAMIC_MINOR 255
>
> struct device;
> --
> 1.8.4
>
^ permalink raw reply
* Re: [PATCH v2 0/7] HID: validate report details
From: Kees Cook @ 2013-09-09 21:48 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg
In-Reply-To: <CAN+gG=HrA7LcAS=kyVHjK_TPo83kd80aZJfv78CJ+01a+UFKDA@mail.gmail.com>
On Mon, Sep 9, 2013 at 6:48 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
>> These patches introduce a validation function for HID devices that do
>> direct report value accesses, solving a number of heap smashing flaws.
>>
>> This version changes to using an field-index-based checker for the new
>> "hid_validate_values()" which requires callers to loop across fields if
>> they use more than one field.
>
> I am globally happy with the patch series.
> I have some concerns about patches 4 6 and 7, but the other can be
> applied right now.
>
> Kees, if you want to switch to something else, I can handle the v3 for
> these three patches: I have some logitech-dj devices and a tester for
> the lenovo one.
That would be fantastic, thank you. I don't have any tools to test
these in the "expected" case. :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* [PATCH] Input: synaptics_usb - Scale stick axes evenly
From: Andrew Deason @ 2013-09-10 3:30 UTC (permalink / raw)
To: linux-input
Currently, we report relative x and y coordinates when receiving data
from the pointing stick. We try to divide each value by 16, since the
raw values are very large, and send the pointer flying across the
screen. Currently we try to do this by shifting the relative
coordinates for the pointing stick over by 4 bits by doing, e.g.:
x = (s16)(be16_to_cpup((__be16 *)&synusb->data[2]) << 3) >> 7;
The raw value from usb is 13-bits wide, so I assume that we shift left
by 3 then right by 7 in order to effectively shift right by 4 while
preserving sign.
However, this is inconsistent with different directions. For example,
an x value of 1 would indicate movement to the right by 1 unit, and
shifting right by 4 reduces this value to 0. But a value of -1
(movement left by 1 unit) is 0x1fff, and running this through the
above code yields a result of -1, not 0.
The result of this is that very tiny movements in the right and up
directions result in no movement, but very tiny movements in the left
and down directions do result in movement, resulting in a confusing
user experience.
So instead of all of this, just figure out what the raw value is, and
divide it by 16 explicitly.
Signed-off-by: Andrew Deason <adeason@dson.org>
---
To be clear, I'm still finishing a larger change to how we report stick
movement, per
<http://permalink.gmane.org/gmane.linux.kernel.input/31777>. But in the
meantime, this simple change removes one of the more aggravating
characteristics of the existing code.
---
drivers/input/mouse/synaptics_usb.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
index 64cf34e..2c4a3c1 100644
--- a/drivers/input/mouse/synaptics_usb.c
+++ b/drivers/input/mouse/synaptics_usb.c
@@ -108,8 +108,10 @@ static void synusb_report_stick(struct synusb *synusb)
unsigned int pressure;
pressure = synusb->data[6];
- x = (s16)(be16_to_cpup((__be16 *)&synusb->data[2]) << 3) >> 7;
- y = (s16)(be16_to_cpup((__be16 *)&synusb->data[4]) << 3) >> 7;
+ x = (s16)(be16_to_cpup((__be16 *)&synusb->data[2]) << 3) >> 3;
+ y = (s16)(be16_to_cpup((__be16 *)&synusb->data[4]) << 3) >> 3;
+ x /= 16;
+ y /= 16;
if (pressure > 0) {
input_report_rel(input_dev, REL_X, x);
--
1.8.4.rc3
^ permalink raw reply related
* Webmail Account Warning.
From: Administrator:: Account Warning. @ 2013-09-10 10:54 UTC (permalink / raw)
Administrator Webmail Account Warning.
This mail is from All Administrator Webmail Account; we wish to bring to
your notice the Condition of your email account.
We have just noticed that you have exceeded your email Database limit of
500 MB quota and your email IP is causing conflict because it is been
accessed in different server location. You need to Upgrade and expand your
email quota limit before you can continue to use your email.
Update your email quota limit to 2.6 GB, use the below web link:
http://accountwarning.yolasite.com/contact-us.php
Failure to do this will result to email deactivation within 24 hours
Thank you for your understanding.
Copyright 2013 Help Desk
Administrator Webmail Support.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox