| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANq1E4SyECC2unw9hAxf5Z4Xm82ogvVJZapJTX_DLrO-SqPBag@mail.gmail.com>
Date: Sun, 16 Feb 2014 17:43:02 +0100
From: David Herrmann <dh.herrmann@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...il.com>
Cc: Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Jiri Kosina <jkosina@...e.cz>,
"open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 03/14] HID: core: implement generic .request()
Hi
On Thu, Feb 13, 2014 at 4:21 PM, Benjamin Tissoires
<benjamin.tissoires@...il.com> wrote:
> On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@...il.com> wrote:
>> Hi
>>
>> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
>> <benjamin.tissoires@...hat.com> wrote:
>>> .request() can be emulated through .raw_request()
>>> we can implement this emulation in hid-core, and make .request
>>> not mandatory for transport layer drivers.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
>>> ---
>>> drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>> include/linux/hid.h | 5 ++++-
>>> 2 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 18fe49b..f36778a 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>>> }
>>> EXPORT_SYMBOL_GPL(hid_output_report);
>>>
>>> +static int hid_report_len(struct hid_report *report)
>>> +{
>>> + return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>
>> Just for clarity, this is equivalent to the following, right?
>>
>> return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;
>
> yes, it should (at least that's what I understand too :)
>
>>
>> I always have to read that shifting code twice to get it.. Maybe add a
>> comment explaining the different entries here.
>
> good idea.
>
>>
>>> +}
>>> +
>>> /*
>>> * Allocator for buffer that is going to be passed to hid_output_report()
>>> */
>>> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>>> * of implement() working on 8 byte chunks
>>> */
>>>
>>> - int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>> + int len = hid_report_len(report);
>>>
>>> return kmalloc(len, flags);
>>> }
>>> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>>> return report;
>>> }
>>>
>>> +/*
>>> + * Implement a generic .request() callback, using .raw_request()
>>> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>>> + */
>>> +void __hid_request(struct hid_device *hid, struct hid_report *report,
>>> + int reqtype)
>>> +{
>>> + char *buf;
>>> + int ret;
>>> + int len;
>>> +
>>> + if (!hid->ll_driver->raw_request)
>>> + return;
>>> +
>>> + buf = hid_alloc_report_buf(report, GFP_KERNEL);
>>> + if (!buf)
>>> + return;
>>> +
>>> + len = hid_report_len(report);
>
> actually, after sending the patches, I was wondering if we should use
> the +7 in hid_report_len.
> "len" is used in .raw_request(), and the +7 was only for the implement(), right?
>
> So maybe a device can reject this because the size of the report is too big...
>
> Jiri, David, any ideas?
Yeah, we should allocate the +7 size, but we shouldn't use it as
"length" argument. We should just silently guarantee the buffer is big
enough.
Thanks
David
> Cheers,
> Benjamin
>
>>> +
>>> + if (reqtype == HID_REQ_SET_REPORT)
>>> + hid_output_report(report, buf);
>>> +
>>> + ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
>>> + report->type, reqtype);
>>> + if (ret < 0) {
>>> + dbg_hid("unable to complete request: %d\n", ret);
>>> + goto out;
>>> + }
>>> +
>>> + if (reqtype == HID_REQ_GET_REPORT)
>>> + hid_input_report(hid, report->type, buf, ret, 0);
>>> +
>>> +out:
>>> + kfree(buf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__hid_request);
>>> +
>>
>> Looks good to me.
>>
>> Reviewed-by: David Herrmann <dh.herrmann@...il.com>
>>
>> Thanks
>> David
>>
>>> int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>> int interrupt)
>>> {
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index a837ede..09fbbd7 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>>> unsigned int hidinput_count_leds(struct hid_device *hid);
>>> __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>>> void hid_output_report(struct hid_report *report, __u8 *data);
>>> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>>> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>>> struct hid_device *hid_allocate_device(void);
>>> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>>> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>>> struct hid_report *report, int reqtype)
>>> {
>>> if (hdev->ll_driver->request)
>>> - hdev->ll_driver->request(hdev, report, reqtype);
>>> + return hdev->ll_driver->request(hdev, report, reqtype);
>>> +
>>> + __hid_request(hdev, report, reqtype);
>>> }
>>>
>>> /**
>>> --
>>> 1.8.3.1
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists