linux-kernel - Re: [PATCH] HID: hid-input: allow input

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5609BDBA.6030605@synaptics.com>
Date:	Mon, 28 Sep 2015 15:22:50 -0700
From:	Andrew Duggan <aduggan@...aptics.com>
To:	David Herrmann <dh.herrmann@...il.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	Jiri Kosina <jikos@...nel.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	Jaikumar Ganesh <jaikumarg@...roid.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Sebastian Reichel <sre@...nel.org>,
	James C Boyd <jcboyd.dev@...il.com>,
	Karl Relton <karllinuxtest.relton@...world.com>,
	Olivier Gay <ogay@...itech.com>,
	Ross Skaliotis <rskaliotis@...il.com>,
	Jamie Lentin <jm@...tin.co.uk>,
	Benjamin Tissoires <benjamin.tissoires@...il.com>,
	Andreas Fleig <andreasfleig@...il.com>,
	Alexey Khoroshilov <khoroshilov@...ras.ru>,
	Peter Wu <peter@...ensteyn.nl>,
	Goffredo Baroncelli <kreijack@...ind.it>,
	Mathieu Magnaudet <mathieu.magnaudet@...il.com>,
	Brent Adam <brentadamdev@...il.com>,
	Yang Bo <linuxsea@....com>,
	Seth Forshee <seth.forshee@...onical.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	Frank Praznik <frank.praznik@...rr.com>,
	Simon Wood <simon@...gewell.org>, Antonio Ospite <ao2@....it>,
	Nikolai Kondrashov <Nikolai.Kondrashov@...hat.com>,
	JD Cole <jd@....me>,
	"open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: hid-input: allow input_configured callback return
 errors

On 09/28/2015 03:10 AM, David Herrmann wrote:
> Hi
>
> On Mon, Sep 28, 2015 at 2:23 AM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
> [...]
>>>>   }
>>>>
>>>>   static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> [snip]
>>>
>>>> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
>>>> index 2c14812..33280f3 100644
>>>> --- a/drivers/hid/hid-rmi.c
>>>> +++ b/drivers/hid/hid-rmi.c
>>>> @@ -1173,7 +1173,7 @@ static int rmi_populate(struct hid_device *hdev)
>>>>          return 0;
>>>>   }
>>>>
>>>> -static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>> +static int rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>>   {
>>>>          struct rmi_data *data = hid_get_drvdata(hdev);
>>>>          struct input_dev *input = hi->input;
>>>> @@ -1185,10 +1185,10 @@ static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>>          hid_dbg(hdev, "Opening low level driver\n");
>>>>          ret = hid_hw_open(hdev);
>>>>          if (ret)
>>>> -               return;
>>>> +               return ret;
>>>>
>>>>          if (!(data->device_flags & RMI_DEVICE))
>>>> -               return;
>>>> +               return -ENXIO;

We should return 0 here. Otherwise, this will break certain keyboards on 
composite USB devices which share a VID and PID with our touchpad. If 
the RMI_DEVICE flag is not set then hid-rmi will pass those reports onto 
hid-input for processing.

>>>>          /* Allow incoming hid reports */
>>>>          hid_device_io_start(hdev);
>>>> @@ -1228,7 +1228,9 @@ static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>>          input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 0x0f, 0, 0);
>>>>          input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 0x0f, 0, 0);
>>>>
>>>> -       input_mt_init_slots(input, data->max_fingers, INPUT_MT_POINTER);
>>>> +       ret = input_mt_init_slots(input, data->max_fingers, INPUT_MT_POINTER);
>>>> +       if (ret < 0)
>>>> +               goto exit;
>>>>
>>>>          if (data->button_count) {
>>>>                  __set_bit(EV_KEY, input->evbit);
>>>> @@ -1244,6 +1246,7 @@ static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>>>   exit:
>>>>          hid_device_io_stop(hdev);
>>>>          hid_hw_close(hdev);
>>>> +       return ret;
>>> rmi_probe() has an explicit comment that it *wants* hid_probe() to
>>> continue on failure, to make sure hidraw is loaded. Not sure what to
>>> make with that, but please either remove the comment in rmi_probe() or
>>> make sure to always return 0 from rmi_input_configured().
>> I think that comment is erroneous since the fact that we could not
>> attach hidinput interface should not affect hidraw in any shape or form.
> The comment might indeed be correct. If rmi_input_configured() failed,
> probing is continued, but the device-internal state might be
> different. rmi_probe() checks that, and explicitly continues device
> probing in that case (if it didn't, the device would indeed be
> rejected).
>
> Sorry for the confusion. Your changes to rmi do look correct.

I will fix the comment, it does incorrectly imply that returning an 
error from rmi_probe() would disconnect hidraw. It is also hard to 
understand without the context of the previous version. If you look at 
the change (daebdd7) it was the call to hid_hw_stop() which disconnected 
hidraw and not the return code, if rmi_populate() can't find F11 on the 
device.

Otherwise, the changes to rmi look correct.

Andrew

--
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/