* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Maxime Ripard @ 2023-08-23 7:44 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: syzbot, davidgow, dmitry.torokhov, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <878ra3m5my.fsf@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 3415 bytes --]
Hi Rahul,
On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> > Hi,
> >
> > So, we discussed it this morning with Benjamin, and I think the culprit
> > is that the uclogic driver will allocate a char array with devm_kzalloc
> > in uclogic_input_configured()
> > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> > and will assign input_dev->name to that pointer.
> >
> > When the device is removed, the devm-allocated array is freed, and the
> > input framework will send a uevent in input_dev_uevent() using the
> > input_dev->name field:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> >
> > So it's a classic dangling pointer situation.
> >
> > And even though it was revealed by that patch, I think the issue is
> > unrelated. The fundamental issue seems to be that the usage of devm in
> > that situation is wrong.
> >
> > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> > and KOBJ_REMOVE will be called after remove.
> >
> > For example, in __device_release_driver() (with the driver remove hook
> > being called in device_remove() and devres_release_all() being called in
> > device_unbind_cleanup()):
> > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> >
> > So, it looks to me that, with or without the patch we merged recently,
> > the core has always sent uevent after device-managed resources were
> > freed. Thus, the uclogic (and any other input driver) was wrong in
> > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> > fields in that struct).
> >
> > Note that freeing input_dev->name in remove would have been just as bad.
> >
> > Looking at the code quickly, at least hid-playstation,
> > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> > affected by the same issue.
>
> I agree with this analysis overall. At least in hid-nvidia-shield, I can
> not use devm for allocating the input name string and explicitly free it
> after calling input_unregister_device. In this scenario, the name string
> would have been freed explicitly after input_put_device was called
> (since the input device is not devres managed). input_put_device would
> drop the reference count to zero and the device would be cleaned up at
> that point triggering KOBJ_REMOVE and firing off that final
> input_dev_uevent.
>
> I think this can be done for a number of the drivers as a workaround
> till this issue is properly resolved. If this seems appropriate, I can
> send out a series later in the day. This is just a workaround till the
> discussion below converges (which I am interested in).
I'm sorry, I don't know the input framework well enough to understand
what you had in mind exactly. Could you send a patch with your
suggestion for the hid-nvidia-shield so we can discuss this further?
That being said, I think that the current design around name, phys and
uniq is fairly treacherous to drivers and we should aim for a solution
that prevents that issue from being possible at all.
I was inclined to go for a char array for each to get rid of the pointer
entirely, but Benjamin raised some concerns over the structure size so
it's probably not a great solution.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: use print_hex_dump_debug to print report descriptor
From: Riwen Lu @ 2023-08-23 8:03 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: jikos, benjamin.tissoires, dmitry.torokhov, linux, hdegoede,
rrangel, u.kleine-koenig, linux-input, linux-kernel, Riwen Lu
In-Reply-To: <87msyq5m7y.fsf@protonmail.com>
在 2023/8/17 12:25, Rahul Rameshbabu 写道:
>
> On Wed, 16 Aug, 2023 16:38:19 +0800 "Riwen Lu" <luriwen@hotmail.com> wrote:
>> From: Riwen Lu <luriwen@kylinos.cn>
>>
>> The format '%*ph' print up to 64 bytes long as a hex string with ' '
>> sepatator. Usually the size of report descriptor is larger than 64
>> bytes, so consider using print_hex_dump_debug to print out all of it for
>> better debugging.
>>
>> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
>>
>> ---
>> v1->v2:
>> - Add a prefix for the hex dump.
>> ---
>> drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index efbba0465eef..fd82e9042da5 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -772,7 +772,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>> }
>> }
>>
>> - i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
>> + i2c_hid_dbg(ihid, "Report Descriptor\n");
>
> Instead of just indicating that the report descriptor dump begins with
> the above print, I think it makes more sense for the print to be changed
> to a pair of begin/end or "cut here" prints similar to what you see in
> oops messages. This will help individuals reading reports copied by bug
> reporters validate that the complete descriptor dump is present.
>
> Something along the lines of what is done in sound/soc/sof/debug.c.
>
I don't think it's necessary to add a pair of begin/end prints like
that. However, I can print out the size of the report descriptor through
i2c_hid_dbg. And print_hex_dump_debug prints each line with a "Report
Descriptor" prefix and an offset, so it's easy to know if the descriptor
dump is complete.
The dump message is similar to the following.
i2c_hid i2c-PNP0C50:00: Report Descriptor size: 0x144
Report Descriptor: 00000000: 05 01 09 02 a1 01 85 01 09 01 a1 00 05 09 19 01
Report Descriptor: 00000010: 29 02 15 00 25 01 75 01 95 02 81 02 95 06 81 03
Report Descriptor: 00000020: 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03
Report Descriptor: 00000030: 81 06 c0 c0 05 0d 09 05 a1 01 85 04 09 22 a1 02
Report Descriptor: 00000040: 15 00 25 01 09 47 09 42 95 02 75 01 81 02 95 01
Report Descriptor: 00000050: 75 03 25 02 09 51 81 02 75 01 95 03 81 03 05 01
Report Descriptor: 00000060: 15 00 26 5b 05 75 10 55 0d 65 11 09 30 35 00 46
Report Descriptor: 00000070: 46 2a 95 01 81 02 46 59 17 26 f4 02 09 31 81 02
Report Descriptor: 00000080: 05 0d 15 00 25 64 95 03 c0 55 0c 66 01 10 47 ff
Report Descriptor: 00000090: ff 00 00 27 ff ff 00 00 75 10 95 01 09 56 81 02
Report Descriptor: 000000a0: 09 54 25 7f 95 01 75 08 81 02 05 09 09 01 25 01
Report Descriptor: 000000b0: 75 01 95 01 81 02 95 07 81 03 09 c5 75 20 95 01
Report Descriptor: 000000c0: 81 03 05 0d 85 02 09 55 09 59 75 04 95 02 25 0f
Report Descriptor: 000000d0: b1 02 85 07 09 60 75 01 95 01 15 00 25 01 b1 02
Report Descriptor: 000000e0: 95 07 b1 03 06 00 ff 85 08 09 c5 15 00 26 ff 00
Report Descriptor: 000000f0: 75 08 96 00 01 b1 02 c0 05 0d 09 0e a1 01 85 03
Report Descriptor: 00000100: 09 22 a1 02 09 52 15 00 25 0a 75 08 95 01 b1 02
Report Descriptor: 00000110: c0 09 22 a1 00 85 05 09 57 09 58 75 01 95 02 25
Report Descriptor: 00000120: 03 b1 02 95 06 b1 03 c0 c0 06 00 ff 09 01 a1 01
Report Descriptor: 00000130: 15 00 26 ff 00 75 08 85 06 95 3f 09 01 81 02 09
Report Descriptor: 00000140: 01 91 02 c0
Thanks.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/sof/debug.c?id=4853c74bd7ab7fdb83f319bd9ace8a08c031e9b6#n407
>
>> + print_hex_dump_debug("Report Descriptor: ", DUMP_PREFIX_OFFSET, 16, 1,
>> + rdesc, rsize, false);
>>
>
> --
> Thanks,
>
> Rahul Rameshbabu
>
^ permalink raw reply
* Re: [PATCH v3 29/42] dt-bindings: rtc: Add ST M48T86
From: Krzysztof Kozlowski @ 2023-08-23 10:16 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-29-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add YAML bindings for ST M48T86 / Dallas DS12887 RTC.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] HID: cp2112: make the irqchip immutable
From: Linus Walleij @ 2023-08-23 12:14 UTC (permalink / raw)
To: Bartosz Golaszewski, Marc Zyngier
Cc: Andy Shevchenko, Jiri Kosina, Benjamin Tissoires, linux-gpio,
linux-kernel, linux-input, Bartosz Golaszewski
In-Reply-To: <20230822152244.214394-1-brgl@bgdev.pl>
On Tue, Aug 22, 2023 at 5:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This make the GPIO irqchip exposed by the CP2112 driver use an immutable
> irq_chip struct thus addressing the following warning on probe:
>
> (cp2112_gpio): not an immutable chip, please consider fixing it!
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Thanks for fixing this. FWIW:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Marc Z, have you checked how we stand with immutable GPIO
irqchips? We should be able to smoke it out to default behaviour
soon I think.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Dmitry Torokhov @ 2023-08-23 12:51 UTC (permalink / raw)
To: Maxime Ripard
Cc: Rahul Rameshbabu, syzbot, davidgow, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <nqrdxdcvuhvcs2syjfhcp5fbk4ckgzibc3izxknvsrka3kres6@ae65hqx67c4u>
On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:
> Hi Rahul,
>
> On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> > > Hi,
> > >
> > > So, we discussed it this morning with Benjamin, and I think the culprit
> > > is that the uclogic driver will allocate a char array with devm_kzalloc
> > > in uclogic_input_configured()
> > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> > > and will assign input_dev->name to that pointer.
> > >
> > > When the device is removed, the devm-allocated array is freed, and the
> > > input framework will send a uevent in input_dev_uevent() using the
> > > input_dev->name field:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> > >
> > > So it's a classic dangling pointer situation.
> > >
> > > And even though it was revealed by that patch, I think the issue is
> > > unrelated. The fundamental issue seems to be that the usage of devm in
> > > that situation is wrong.
> > >
> > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> > > and KOBJ_REMOVE will be called after remove.
> > >
> > > For example, in __device_release_driver() (with the driver remove hook
> > > being called in device_remove() and devres_release_all() being called in
> > > device_unbind_cleanup()):
> > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> > >
> > > So, it looks to me that, with or without the patch we merged recently,
> > > the core has always sent uevent after device-managed resources were
> > > freed. Thus, the uclogic (and any other input driver) was wrong in
> > > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> > > fields in that struct).
> > >
> > > Note that freeing input_dev->name in remove would have been just as bad.
> > >
> > > Looking at the code quickly, at least hid-playstation,
> > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> > > affected by the same issue.
> >
> > I agree with this analysis overall. At least in hid-nvidia-shield, I can
> > not use devm for allocating the input name string and explicitly free it
> > after calling input_unregister_device. In this scenario, the name string
> > would have been freed explicitly after input_put_device was called
> > (since the input device is not devres managed). input_put_device would
> > drop the reference count to zero and the device would be cleaned up at
> > that point triggering KOBJ_REMOVE and firing off that final
> > input_dev_uevent.
> >
> > I think this can be done for a number of the drivers as a workaround
> > till this issue is properly resolved. If this seems appropriate, I can
> > send out a series later in the day. This is just a workaround till the
> > discussion below converges (which I am interested in).
>
> I'm sorry, I don't know the input framework well enough to understand
> what you had in mind exactly. Could you send a patch with your
> suggestion for the hid-nvidia-shield so we can discuss this further?
>
> That being said, I think that the current design around name, phys and
> uniq is fairly treacherous to drivers and we should aim for a solution
> that prevents that issue from being possible at all.
>
> I was inclined to go for a char array for each to get rid of the pointer
> entirely, but Benjamin raised some concerns over the structure size so
> it's probably not a great solution.
I think everything is much simpler, with uclogic driver being in the
wrong here: devm resource needs to be attached to the right device
(instance of HID) rather than to the input device itself (which should
never have any driver resources attached since it never has a driver).
Something like this:
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index f67835f9ed4c..f234a7c97360 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
if (suffix) {
len = strlen(hdev->name) + 2 + strlen(suffix);
- name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
+ name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
if (name) {
snprintf(name, len, "%s %s", hdev->name, suffix);
hi->input->name = name;
In general, drivers should attach devm resources they allocate to the
instance of device they are binding to, and nothing else.
Thanks.
--
Dmitry
^ permalink raw reply related
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Maxime Ripard @ 2023-08-23 13:16 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rahul Rameshbabu, syzbot, davidgow, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <ZOYAtI5KttKQnLWs@google.com>
[-- Attachment #1: Type: text/plain, Size: 5137 bytes --]
Hi Dmitry,
On Wed, Aug 23, 2023 at 05:51:00AM -0700, Dmitry Torokhov wrote:
> On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:
> > On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> > > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> > > > Hi,
> > > >
> > > > So, we discussed it this morning with Benjamin, and I think the culprit
> > > > is that the uclogic driver will allocate a char array with devm_kzalloc
> > > > in uclogic_input_configured()
> > > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> > > > and will assign input_dev->name to that pointer.
> > > >
> > > > When the device is removed, the devm-allocated array is freed, and the
> > > > input framework will send a uevent in input_dev_uevent() using the
> > > > input_dev->name field:
> > > >
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> > > >
> > > > So it's a classic dangling pointer situation.
> > > >
> > > > And even though it was revealed by that patch, I think the issue is
> > > > unrelated. The fundamental issue seems to be that the usage of devm in
> > > > that situation is wrong.
> > > >
> > > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> > > > and KOBJ_REMOVE will be called after remove.
> > > >
> > > > For example, in __device_release_driver() (with the driver remove hook
> > > > being called in device_remove() and devres_release_all() being called in
> > > > device_unbind_cleanup()):
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> > > >
> > > > So, it looks to me that, with or without the patch we merged recently,
> > > > the core has always sent uevent after device-managed resources were
> > > > freed. Thus, the uclogic (and any other input driver) was wrong in
> > > > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> > > > fields in that struct).
> > > >
> > > > Note that freeing input_dev->name in remove would have been just as bad.
> > > >
> > > > Looking at the code quickly, at least hid-playstation,
> > > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> > > > affected by the same issue.
> > >
> > > I agree with this analysis overall. At least in hid-nvidia-shield, I can
> > > not use devm for allocating the input name string and explicitly free it
> > > after calling input_unregister_device. In this scenario, the name string
> > > would have been freed explicitly after input_put_device was called
> > > (since the input device is not devres managed). input_put_device would
> > > drop the reference count to zero and the device would be cleaned up at
> > > that point triggering KOBJ_REMOVE and firing off that final
> > > input_dev_uevent.
> > >
> > > I think this can be done for a number of the drivers as a workaround
> > > till this issue is properly resolved. If this seems appropriate, I can
> > > send out a series later in the day. This is just a workaround till the
> > > discussion below converges (which I am interested in).
> >
> > I'm sorry, I don't know the input framework well enough to understand
> > what you had in mind exactly. Could you send a patch with your
> > suggestion for the hid-nvidia-shield so we can discuss this further?
> >
> > That being said, I think that the current design around name, phys and
> > uniq is fairly treacherous to drivers and we should aim for a solution
> > that prevents that issue from being possible at all.
> >
> > I was inclined to go for a char array for each to get rid of the pointer
> > entirely, but Benjamin raised some concerns over the structure size so
> > it's probably not a great solution.
>
> I think everything is much simpler, with uclogic driver being in the
> wrong here: devm resource needs to be attached to the right device
> (instance of HID) rather than to the input device itself (which should
> never have any driver resources attached since it never has a driver).
> Something like this:
>
> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index f67835f9ed4c..f234a7c97360 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
>
> if (suffix) {
> len = strlen(hdev->name) + 2 + strlen(suffix);
> - name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
> + name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
> if (name) {
> snprintf(name, len, "%s %s", hdev->name, suffix);
> hi->input->name = name;
>
> In general, drivers should attach devm resources they allocate to the
> instance of device they are binding to, and nothing else.
I'm not sure that's enough unfortunately. The fundamental issue here
seems to be that input_dev_uevent follows a pointer that can be
allocated by the driver, and will be free'd before the last call to
input_dev_uevent.
And I think that's true for both devices here
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Dmitry Torokhov @ 2023-08-23 14:55 UTC (permalink / raw)
To: Maxime Ripard
Cc: Rahul Rameshbabu, syzbot, davidgow, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <y3hfjc227wouzraa4hyltyfd4athig5rmna2uwjqy6v23yzkad@wdmzmochmsve>
Hi Maxime,
On Wed, Aug 23, 2023 at 03:16:02PM +0200, Maxime Ripard wrote:
> Hi Dmitry,
>
> On Wed, Aug 23, 2023 at 05:51:00AM -0700, Dmitry Torokhov wrote:
> > On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:
> > > On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> > > > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> > > > > Hi,
> > > > >
> > > > > So, we discussed it this morning with Benjamin, and I think the culprit
> > > > > is that the uclogic driver will allocate a char array with devm_kzalloc
> > > > > in uclogic_input_configured()
> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> > > > > and will assign input_dev->name to that pointer.
> > > > >
> > > > > When the device is removed, the devm-allocated array is freed, and the
> > > > > input framework will send a uevent in input_dev_uevent() using the
> > > > > input_dev->name field:
> > > > >
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> > > > >
> > > > > So it's a classic dangling pointer situation.
> > > > >
> > > > > And even though it was revealed by that patch, I think the issue is
> > > > > unrelated. The fundamental issue seems to be that the usage of devm in
> > > > > that situation is wrong.
> > > > >
> > > > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> > > > > and KOBJ_REMOVE will be called after remove.
> > > > >
> > > > > For example, in __device_release_driver() (with the driver remove hook
> > > > > being called in device_remove() and devres_release_all() being called in
> > > > > device_unbind_cleanup()):
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> > > > >
> > > > > So, it looks to me that, with or without the patch we merged recently,
> > > > > the core has always sent uevent after device-managed resources were
> > > > > freed. Thus, the uclogic (and any other input driver) was wrong in
> > > > > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> > > > > fields in that struct).
> > > > >
> > > > > Note that freeing input_dev->name in remove would have been just as bad.
> > > > >
> > > > > Looking at the code quickly, at least hid-playstation,
> > > > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> > > > > affected by the same issue.
> > > >
> > > > I agree with this analysis overall. At least in hid-nvidia-shield, I can
> > > > not use devm for allocating the input name string and explicitly free it
> > > > after calling input_unregister_device. In this scenario, the name string
> > > > would have been freed explicitly after input_put_device was called
> > > > (since the input device is not devres managed). input_put_device would
> > > > drop the reference count to zero and the device would be cleaned up at
> > > > that point triggering KOBJ_REMOVE and firing off that final
> > > > input_dev_uevent.
> > > >
> > > > I think this can be done for a number of the drivers as a workaround
> > > > till this issue is properly resolved. If this seems appropriate, I can
> > > > send out a series later in the day. This is just a workaround till the
> > > > discussion below converges (which I am interested in).
> > >
> > > I'm sorry, I don't know the input framework well enough to understand
> > > what you had in mind exactly. Could you send a patch with your
> > > suggestion for the hid-nvidia-shield so we can discuss this further?
> > >
> > > That being said, I think that the current design around name, phys and
> > > uniq is fairly treacherous to drivers and we should aim for a solution
> > > that prevents that issue from being possible at all.
> > >
> > > I was inclined to go for a char array for each to get rid of the pointer
> > > entirely, but Benjamin raised some concerns over the structure size so
> > > it's probably not a great solution.
> >
> > I think everything is much simpler, with uclogic driver being in the
> > wrong here: devm resource needs to be attached to the right device
> > (instance of HID) rather than to the input device itself (which should
> > never have any driver resources attached since it never has a driver).
> > Something like this:
> >
> > diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> > index f67835f9ed4c..f234a7c97360 100644
> > --- a/drivers/hid/hid-uclogic-core.c
> > +++ b/drivers/hid/hid-uclogic-core.c
> > @@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
> >
> > if (suffix) {
> > len = strlen(hdev->name) + 2 + strlen(suffix);
> > - name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
> > + name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
> > if (name) {
> > snprintf(name, len, "%s %s", hdev->name, suffix);
> > hi->input->name = name;
> >
> > In general, drivers should attach devm resources they allocate to the
> > instance of device they are binding to, and nothing else.
>
> I'm not sure that's enough unfortunately. The fundamental issue here
> seems to be that input_dev_uevent follows a pointer that can be
> allocated by the driver, and will be free'd before the last call to
> input_dev_uevent.
Yes, this is a fundamental property of C pointers - you should not free
them before exiting last code section that may reference them. For input
devices it means that pointers should not be freed until after
input_unregister_device() is called.
I.e. you have sequence like this:
driver_data = kzalloc(...);
driver_data->input_name = kstrdup(...);
driver_data->input_phys = kstrdup(...);
input = input_allocate_device();
input->name = driver_data->input_name;
input->phys = driver_data->input_phys;
input_register_device(input);
...
input_unregister_device(input);
kfree(driver_data->input_name);
kfree(driver_data->input_phys);
kfree(driver_data);
devm typically helps with resources being freed at the right time, but
for that the managed resources should be attached to the *correct
device*, with correct device being one the driver is binding to, not any
random device structure.
>
> And I think that's true for both devices here
Yes, it looks like the shield is also using wrong device.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Maxime Ripard @ 2023-08-23 16:30 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rahul Rameshbabu, syzbot, davidgow, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <ZOYd1R3Bx2IToWfb@google.com>
[-- Attachment #1: Type: text/plain, Size: 6637 bytes --]
On Wed, Aug 23, 2023 at 07:55:17AM -0700, Dmitry Torokhov wrote:
> On Wed, Aug 23, 2023 at 03:16:02PM +0200, Maxime Ripard wrote:
> > On Wed, Aug 23, 2023 at 05:51:00AM -0700, Dmitry Torokhov wrote:
> > > On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:
> > > > On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> > > > > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > Hi,
> > > > > >
> > > > > > So, we discussed it this morning with Benjamin, and I think the culprit
> > > > > > is that the uclogic driver will allocate a char array with devm_kzalloc
> > > > > > in uclogic_input_configured()
> > > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> > > > > > and will assign input_dev->name to that pointer.
> > > > > >
> > > > > > When the device is removed, the devm-allocated array is freed, and the
> > > > > > input framework will send a uevent in input_dev_uevent() using the
> > > > > > input_dev->name field:
> > > > > >
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> > > > > >
> > > > > > So it's a classic dangling pointer situation.
> > > > > >
> > > > > > And even though it was revealed by that patch, I think the issue is
> > > > > > unrelated. The fundamental issue seems to be that the usage of devm in
> > > > > > that situation is wrong.
> > > > > >
> > > > > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> > > > > > and KOBJ_REMOVE will be called after remove.
> > > > > >
> > > > > > For example, in __device_release_driver() (with the driver remove hook
> > > > > > being called in device_remove() and devres_release_all() being called in
> > > > > > device_unbind_cleanup()):
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> > > > > >
> > > > > > So, it looks to me that, with or without the patch we merged recently,
> > > > > > the core has always sent uevent after device-managed resources were
> > > > > > freed. Thus, the uclogic (and any other input driver) was wrong in
> > > > > > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> > > > > > fields in that struct).
> > > > > >
> > > > > > Note that freeing input_dev->name in remove would have been just as bad.
> > > > > >
> > > > > > Looking at the code quickly, at least hid-playstation,
> > > > > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> > > > > > affected by the same issue.
> > > > >
> > > > > I agree with this analysis overall. At least in hid-nvidia-shield, I can
> > > > > not use devm for allocating the input name string and explicitly free it
> > > > > after calling input_unregister_device. In this scenario, the name string
> > > > > would have been freed explicitly after input_put_device was called
> > > > > (since the input device is not devres managed). input_put_device would
> > > > > drop the reference count to zero and the device would be cleaned up at
> > > > > that point triggering KOBJ_REMOVE and firing off that final
> > > > > input_dev_uevent.
> > > > >
> > > > > I think this can be done for a number of the drivers as a workaround
> > > > > till this issue is properly resolved. If this seems appropriate, I can
> > > > > send out a series later in the day. This is just a workaround till the
> > > > > discussion below converges (which I am interested in).
> > > >
> > > > I'm sorry, I don't know the input framework well enough to understand
> > > > what you had in mind exactly. Could you send a patch with your
> > > > suggestion for the hid-nvidia-shield so we can discuss this further?
> > > >
> > > > That being said, I think that the current design around name, phys and
> > > > uniq is fairly treacherous to drivers and we should aim for a solution
> > > > that prevents that issue from being possible at all.
> > > >
> > > > I was inclined to go for a char array for each to get rid of the pointer
> > > > entirely, but Benjamin raised some concerns over the structure size so
> > > > it's probably not a great solution.
> > >
> > > I think everything is much simpler, with uclogic driver being in the
> > > wrong here: devm resource needs to be attached to the right device
> > > (instance of HID) rather than to the input device itself (which should
> > > never have any driver resources attached since it never has a driver).
> > > Something like this:
> > >
> > > diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> > > index f67835f9ed4c..f234a7c97360 100644
> > > --- a/drivers/hid/hid-uclogic-core.c
> > > +++ b/drivers/hid/hid-uclogic-core.c
> > > @@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
> > >
> > > if (suffix) {
> > > len = strlen(hdev->name) + 2 + strlen(suffix);
> > > - name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
> > > + name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
> > > if (name) {
> > > snprintf(name, len, "%s %s", hdev->name, suffix);
> > > hi->input->name = name;
> > >
> > > In general, drivers should attach devm resources they allocate to the
> > > instance of device they are binding to, and nothing else.
> >
> > I'm not sure that's enough unfortunately. The fundamental issue here
> > seems to be that input_dev_uevent follows a pointer that can be
> > allocated by the driver, and will be free'd before the last call to
> > input_dev_uevent.
>
> Yes, this is a fundamental property of C pointers - you should not free
> them before exiting last code section that may reference them. For input
> devices it means that pointers should not be freed until after
> input_unregister_device() is called.
>
> I.e. you have sequence like this:
>
> driver_data = kzalloc(...);
> driver_data->input_name = kstrdup(...);
> driver_data->input_phys = kstrdup(...);
> input = input_allocate_device();
> input->name = driver_data->input_name;
> input->phys = driver_data->input_phys;
> input_register_device(input);
> ...
>
> input_unregister_device(input);
> kfree(driver_data->input_name);
> kfree(driver_data->input_phys);
> kfree(driver_data);
>
> devm typically helps with resources being freed at the right time, but
> for that the managed resources should be attached to the *correct
> device*, with correct device being one the driver is binding to, not any
> random device structure.
Thanks, I see what you mean now. It looks good to me indeed :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Rahul Rameshbabu @ 2023-08-23 17:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Maxime Ripard, syzbot, davidgow, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <ZOYd1R3Bx2IToWfb@google.com>
On Wed, 23 Aug, 2023 07:55:17 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Maxime,
>
> On Wed, Aug 23, 2023 at 03:16:02PM +0200, Maxime Ripard wrote:
>> Hi Dmitry,
>>
>> On Wed, Aug 23, 2023 at 05:51:00AM -0700, Dmitry Torokhov wrote:
>> > On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:
>> > > On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
>> > > > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
>> > > > > Hi,
>> > > > >
>> > > > > So, we discussed it this morning with Benjamin, and I think the culprit
>> > > > > is that the uclogic driver will allocate a char array with devm_kzalloc
>> > > > > in uclogic_input_configured()
>> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
>> > > > > and will assign input_dev->name to that pointer.
>> > > > >
>> > > > > When the device is removed, the devm-allocated array is freed, and the
>> > > > > input framework will send a uevent in input_dev_uevent() using the
>> > > > > input_dev->name field:
>> > > > >
>> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
>> > > > >
>> > > > > So it's a classic dangling pointer situation.
>> > > > >
>> > > > > And even though it was revealed by that patch, I think the issue is
>> > > > > unrelated. The fundamental issue seems to be that the usage of devm in
>> > > > > that situation is wrong.
>> > > > >
>> > > > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
>> > > > > and KOBJ_REMOVE will be called after remove.
>> > > > >
>> > > > > For example, in __device_release_driver() (with the driver remove hook
>> > > > > being called in device_remove() and devres_release_all() being called in
>> > > > > device_unbind_cleanup()):
>> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
>> > > > >
>> > > > > So, it looks to me that, with or without the patch we merged recently,
>> > > > > the core has always sent uevent after device-managed resources were
>> > > > > freed. Thus, the uclogic (and any other input driver) was wrong in
>> > > > > allocating its input_dev name with devm_kzalloc (or the phys and uniq
>> > > > > fields in that struct).
>> > > > >
>> > > > > Note that freeing input_dev->name in remove would have been just as bad.
>> > > > >
>> > > > > Looking at the code quickly, at least hid-playstation,
>> > > > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
>> > > > > affected by the same issue.
>> > > >
>> > > > I agree with this analysis overall. At least in hid-nvidia-shield, I can
>> > > > not use devm for allocating the input name string and explicitly free it
>> > > > after calling input_unregister_device. In this scenario, the name string
>> > > > would have been freed explicitly after input_put_device was called
>> > > > (since the input device is not devres managed). input_put_device would
>> > > > drop the reference count to zero and the device would be cleaned up at
>> > > > that point triggering KOBJ_REMOVE and firing off that final
>> > > > input_dev_uevent.
>> > > >
>> > > > I think this can be done for a number of the drivers as a workaround
>> > > > till this issue is properly resolved. If this seems appropriate, I can
>> > > > send out a series later in the day. This is just a workaround till the
>> > > > discussion below converges (which I am interested in).
>> > >
>> > > I'm sorry, I don't know the input framework well enough to understand
>> > > what you had in mind exactly. Could you send a patch with your
>> > > suggestion for the hid-nvidia-shield so we can discuss this further?
>> > >
>> > > That being said, I think that the current design around name, phys and
>> > > uniq is fairly treacherous to drivers and we should aim for a solution
>> > > that prevents that issue from being possible at all.
>> > >
>> > > I was inclined to go for a char array for each to get rid of the pointer
>> > > entirely, but Benjamin raised some concerns over the structure size so
>> > > it's probably not a great solution.
>> >
>> > I think everything is much simpler, with uclogic driver being in the
>> > wrong here: devm resource needs to be attached to the right device
>> > (instance of HID) rather than to the input device itself (which should
>> > never have any driver resources attached since it never has a driver).
>> > Something like this:
>> >
>> > diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
>> > index f67835f9ed4c..f234a7c97360 100644
>> > --- a/drivers/hid/hid-uclogic-core.c
>> > +++ b/drivers/hid/hid-uclogic-core.c
>> > @@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
>> >
>> > if (suffix) {
>> > len = strlen(hdev->name) + 2 + strlen(suffix);
>> > - name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
>> > + name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
>> > if (name) {
>> > snprintf(name, len, "%s %s", hdev->name, suffix);
>> > hi->input->name = name;
>> >
>> > In general, drivers should attach devm resources they allocate to the
>> > instance of device they are binding to, and nothing else.
>>
>> I'm not sure that's enough unfortunately. The fundamental issue here
>> seems to be that input_dev_uevent follows a pointer that can be
>> allocated by the driver, and will be free'd before the last call to
>> input_dev_uevent.
>
> Yes, this is a fundamental property of C pointers - you should not free
> them before exiting last code section that may reference them. For input
> devices it means that pointers should not be freed until after
> input_unregister_device() is called.
>
> I.e. you have sequence like this:
>
> driver_data = kzalloc(...);
> driver_data->input_name = kstrdup(...);
> driver_data->input_phys = kstrdup(...);
> input = input_allocate_device();
> input->name = driver_data->input_name;
> input->phys = driver_data->input_phys;
> input_register_device(input);
> ...
>
> input_unregister_device(input);
> kfree(driver_data->input_name);
> kfree(driver_data->input_phys);
> kfree(driver_data);
>
>
> devm typically helps with resources being freed at the right time, but
> for that the managed resources should be attached to the *correct
> device*, with correct device being one the driver is binding to, not any
> random device structure.
>
>>
>> And I think that's true for both devices here
>
> Yes, it looks like the shield is also using wrong device.
This is a problem in shield too. I'll submit a patch. I'll take a look
at other drivers as well to see if any of them run into this issue.
idev->name = devm_kasprintf(&idev->dev, GFP_KERNEL, "%s %s", hdev->name,
name_suffix);
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Dmitry Torokhov @ 2023-08-23 17:56 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: Maxime Ripard, syzbot, davidgow, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <87zg2h3d25.fsf@nvidia.com>
On Wed, Aug 23, 2023 at 10:04:34AM -0700, Rahul Rameshbabu wrote:
> On Wed, 23 Aug, 2023 07:55:17 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > Hi Maxime,
> >
> > On Wed, Aug 23, 2023 at 03:16:02PM +0200, Maxime Ripard wrote:
> >> Hi Dmitry,
> >>
> >> On Wed, Aug 23, 2023 at 05:51:00AM -0700, Dmitry Torokhov wrote:
> >> > On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:
> >> > > On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> >> > > > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> >> > > > > Hi,
> >> > > > >
> >> > > > > So, we discussed it this morning with Benjamin, and I think the culprit
> >> > > > > is that the uclogic driver will allocate a char array with devm_kzalloc
> >> > > > > in uclogic_input_configured()
> >> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> >> > > > > and will assign input_dev->name to that pointer.
> >> > > > >
> >> > > > > When the device is removed, the devm-allocated array is freed, and the
> >> > > > > input framework will send a uevent in input_dev_uevent() using the
> >> > > > > input_dev->name field:
> >> > > > >
> >> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> >> > > > >
> >> > > > > So it's a classic dangling pointer situation.
> >> > > > >
> >> > > > > And even though it was revealed by that patch, I think the issue is
> >> > > > > unrelated. The fundamental issue seems to be that the usage of devm in
> >> > > > > that situation is wrong.
> >> > > > >
> >> > > > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> >> > > > > and KOBJ_REMOVE will be called after remove.
> >> > > > >
> >> > > > > For example, in __device_release_driver() (with the driver remove hook
> >> > > > > being called in device_remove() and devres_release_all() being called in
> >> > > > > device_unbind_cleanup()):
> >> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> >> > > > >
> >> > > > > So, it looks to me that, with or without the patch we merged recently,
> >> > > > > the core has always sent uevent after device-managed resources were
> >> > > > > freed. Thus, the uclogic (and any other input driver) was wrong in
> >> > > > > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> >> > > > > fields in that struct).
> >> > > > >
> >> > > > > Note that freeing input_dev->name in remove would have been just as bad.
> >> > > > >
> >> > > > > Looking at the code quickly, at least hid-playstation,
> >> > > > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> >> > > > > affected by the same issue.
> >> > > >
> >> > > > I agree with this analysis overall. At least in hid-nvidia-shield, I can
> >> > > > not use devm for allocating the input name string and explicitly free it
> >> > > > after calling input_unregister_device. In this scenario, the name string
> >> > > > would have been freed explicitly after input_put_device was called
> >> > > > (since the input device is not devres managed). input_put_device would
> >> > > > drop the reference count to zero and the device would be cleaned up at
> >> > > > that point triggering KOBJ_REMOVE and firing off that final
> >> > > > input_dev_uevent.
> >> > > >
> >> > > > I think this can be done for a number of the drivers as a workaround
> >> > > > till this issue is properly resolved. If this seems appropriate, I can
> >> > > > send out a series later in the day. This is just a workaround till the
> >> > > > discussion below converges (which I am interested in).
> >> > >
> >> > > I'm sorry, I don't know the input framework well enough to understand
> >> > > what you had in mind exactly. Could you send a patch with your
> >> > > suggestion for the hid-nvidia-shield so we can discuss this further?
> >> > >
> >> > > That being said, I think that the current design around name, phys and
> >> > > uniq is fairly treacherous to drivers and we should aim for a solution
> >> > > that prevents that issue from being possible at all.
> >> > >
> >> > > I was inclined to go for a char array for each to get rid of the pointer
> >> > > entirely, but Benjamin raised some concerns over the structure size so
> >> > > it's probably not a great solution.
> >> >
> >> > I think everything is much simpler, with uclogic driver being in the
> >> > wrong here: devm resource needs to be attached to the right device
> >> > (instance of HID) rather than to the input device itself (which should
> >> > never have any driver resources attached since it never has a driver).
> >> > Something like this:
> >> >
> >> > diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> >> > index f67835f9ed4c..f234a7c97360 100644
> >> > --- a/drivers/hid/hid-uclogic-core.c
> >> > +++ b/drivers/hid/hid-uclogic-core.c
> >> > @@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
> >> >
> >> > if (suffix) {
> >> > len = strlen(hdev->name) + 2 + strlen(suffix);
> >> > - name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
> >> > + name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
> >> > if (name) {
> >> > snprintf(name, len, "%s %s", hdev->name, suffix);
> >> > hi->input->name = name;
> >> >
> >> > In general, drivers should attach devm resources they allocate to the
> >> > instance of device they are binding to, and nothing else.
> >>
> >> I'm not sure that's enough unfortunately. The fundamental issue here
> >> seems to be that input_dev_uevent follows a pointer that can be
> >> allocated by the driver, and will be free'd before the last call to
> >> input_dev_uevent.
> >
> > Yes, this is a fundamental property of C pointers - you should not free
> > them before exiting last code section that may reference them. For input
> > devices it means that pointers should not be freed until after
> > input_unregister_device() is called.
> >
> > I.e. you have sequence like this:
> >
> > driver_data = kzalloc(...);
> > driver_data->input_name = kstrdup(...);
> > driver_data->input_phys = kstrdup(...);
> > input = input_allocate_device();
> > input->name = driver_data->input_name;
> > input->phys = driver_data->input_phys;
> > input_register_device(input);
> > ...
> >
> > input_unregister_device(input);
> > kfree(driver_data->input_name);
> > kfree(driver_data->input_phys);
> > kfree(driver_data);
> >
> >
> > devm typically helps with resources being freed at the right time, but
> > for that the managed resources should be attached to the *correct
> > device*, with correct device being one the driver is binding to, not any
> > random device structure.
> >
> >>
> >> And I think that's true for both devices here
> >
> > Yes, it looks like the shield is also using wrong device.
>
> This is a problem in shield too. I'll submit a patch. I'll take a look
> at other drivers as well to see if any of them run into this issue.
>
> idev->name = devm_kasprintf(&idev->dev, GFP_KERNEL, "%s %s", hdev->name,
> name_suffix);
I did a quick scan in drivers/hid and it looks like hid-multitouch.c
also has this issue. So that makes 3 - hid-multitouch, uclogic and the
shield drivers.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: cp2112: make the irqchip immutable
From: Marc Zyngier @ 2023-08-23 18:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Bartosz Golaszewski, Andy Shevchenko, Jiri Kosina,
Benjamin Tissoires, linux-gpio, linux-kernel, linux-input,
Bartosz Golaszewski
In-Reply-To: <CACRpkdZWKDsHtYHqWSqz6QYp4TKT4OskWO7v6w=2v3NhvYWc4w@mail.gmail.com>
On Wed, 23 Aug 2023 13:14:59 +0100,
Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Aug 22, 2023 at 5:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > This make the GPIO irqchip exposed by the CP2112 driver use an immutable
> > irq_chip struct thus addressing the following warning on probe:
> >
> > (cp2112_gpio): not an immutable chip, please consider fixing it!
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Thanks for fixing this. FWIW:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Marc Z, have you checked how we stand with immutable GPIO
> irqchips? We should be able to smoke it out to default behaviour
> soon I think.
I haven't recently checked, but I'm still in favour of actively
make the new behaviour the only one supported. The sooner we break
things, the better...
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply
* [PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name
From: Rahul Rameshbabu @ 2023-08-24 6:13 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Dmitry Torokhov, Maxime Ripard,
Rahul Rameshbabu
Maxime Ripard analyzed the following situation involving a use-after-free caused
by incorrect devres management.
1. input_dev name allocated as a resource referring to the same input_dev
instance
2. The input_dev is eventually unregistered
3. Unregistering the device first involves releasing devres managed resources
tied to the input_dev
4. A uevent is then fired for the input_dev, referencing various members of
the input_dev including the name
5. This leads to a use-after-free in the context of the triggered uevent
Dmitry Torokhov pointed out that the correct pattern for devm usage with the
input_dev would be to allocate the resource referencing the underlying device
that was probed by the driver than referencing the input subdevice instance. In
the case of hid drivers, the name resource will only be freed when devres
management reclaims resources for the hid_device. This will be after the
input_dev was unregistered and the uevent referencing the name was invoked.
This patch series applies the analysis done to correct problematic HID drivers.
Link: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Rahul Rameshbabu (3):
HID: uclogic: Correct devm device reference for hidinput input_dev
name
HID: multitouch: Correct devm device reference for hidinput input_dev
name
HID: nvidia-shield: Reference hid_device devm allocation of input_dev
name
drivers/hid/hid-multitouch.c | 13 +++----------
drivers/hid/hid-nvidia-shield.c | 2 +-
drivers/hid/hid-uclogic-core.c | 13 +++----------
3 files changed, 7 insertions(+), 21 deletions(-)
--
2.40.1
^ permalink raw reply
* [PATCH HID 1/3] HID: uclogic: Correct devm device reference for hidinput input_dev name
From: Rahul Rameshbabu @ 2023-08-24 6:14 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Dmitry Torokhov, Maxime Ripard,
Rahul Rameshbabu, syzbot+3a0ebe8a52b89c63739d
In-Reply-To: <20230824061308.222021-1-sergeantsagara@protonmail.com>
Reference the HID device rather than the input device for the devm
allocation of the input_dev name. Referencing the input_dev would lead to a
use-after-free when the input_dev was unregistered and subsequently fires a
uevent that depends on the name. At the point of firing the uevent, the
name would be freed by devres management.
Use devm_kasprintf to simplify the logic for allocating memory and
formatting the input_dev name string.
Reported-by: syzbot+3a0ebe8a52b89c63739d@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/
Reported-by: Maxime Ripard <mripard@kernel.org>
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Fixes: cce2dbdf258e ("HID: uclogic: name the input nodes based on their tool")
Suggested-by: Maxime Ripard <mripard@kernel.org>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
drivers/hid/hid-uclogic-core.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index f67835f9ed4c..ad74cbc9a0aa 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -85,10 +85,8 @@ static int uclogic_input_configured(struct hid_device *hdev,
{
struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
struct uclogic_params *params = &drvdata->params;
- char *name;
const char *suffix = NULL;
struct hid_field *field;
- size_t len;
size_t i;
const struct uclogic_params_frame *frame;
@@ -146,14 +144,9 @@ static int uclogic_input_configured(struct hid_device *hdev,
}
}
- if (suffix) {
- len = strlen(hdev->name) + 2 + strlen(suffix);
- name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
- if (name) {
- snprintf(name, len, "%s %s", hdev->name, suffix);
- hi->input->name = name;
- }
- }
+ if (suffix)
+ hi->input->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+ "%s %s", hdev->name, suffix);
return 0;
}
--
2.40.1
^ permalink raw reply related
* [PATCH HID 2/3] HID: multitouch: Correct devm device reference for hidinput input_dev name
From: Rahul Rameshbabu @ 2023-08-24 6:14 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Dmitry Torokhov, Maxime Ripard,
Rahul Rameshbabu
In-Reply-To: <20230824061308.222021-1-sergeantsagara@protonmail.com>
Reference the HID device rather than the input device for the devm
allocation of the input_dev name. Referencing the input_dev would lead to a
use-after-free when the input_dev was unregistered and subsequently fires a
uevent that depends on the name. At the point of firing the uevent, the
name would be freed by devres management.
Use devm_kasprintf to simplify the logic for allocating memory and
formatting the input_dev name string.
Reported-by: Maxime Ripard <mripard@kernel.org>
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Fixes: c08d46aa805b ("HID: multitouch: devm conversion")
Suggested-by: Maxime Ripard <mripard@kernel.org>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
drivers/hid/hid-multitouch.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index e31be0cb8b85..521b2ffb4244 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1594,7 +1594,6 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app)
static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
{
struct mt_device *td = hid_get_drvdata(hdev);
- char *name;
const char *suffix = NULL;
struct mt_report_data *rdata;
struct mt_application *mt_application = NULL;
@@ -1645,15 +1644,9 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
break;
}
- if (suffix) {
- name = devm_kzalloc(&hi->input->dev,
- strlen(hdev->name) + strlen(suffix) + 2,
- GFP_KERNEL);
- if (name) {
- sprintf(name, "%s %s", hdev->name, suffix);
- hi->input->name = name;
- }
- }
+ if (suffix)
+ hi->input->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+ "%s %s", hdev->name, suffix);
return 0;
}
--
2.40.1
^ permalink raw reply related
* [PATCH HID 3/3] HID: nvidia-shield: Reference hid_device devm allocation of input_dev name
From: Rahul Rameshbabu @ 2023-08-24 6:14 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Dmitry Torokhov, Maxime Ripard,
Rahul Rameshbabu
In-Reply-To: <20230824061308.222021-1-sergeantsagara@protonmail.com>
From: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Use hid_device for devm allocation of the input_dev name to avoid a
use-after-free. input_unregister_device would trigger devres cleanup of all
resources associated with the input_dev, free-ing the name. The name would
subsequently be used in a uevent fired at the end of unregistering the
input_dev.
Reported-by: Maxime Ripard <mripard@kernel.org>
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Fixes: 09308562d4af ("HID: nvidia-shield: Initial driver implementation with Thunderstrike support")
Suggested-by: Maxime Ripard <mripard@kernel.org>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
drivers/hid/hid-nvidia-shield.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index a928ad2be62d..084179a6be86 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -164,7 +164,7 @@ static struct input_dev *shield_allocate_input_dev(struct hid_device *hdev,
idev->id.product = hdev->product;
idev->id.version = hdev->version;
idev->uniq = hdev->uniq;
- idev->name = devm_kasprintf(&idev->dev, GFP_KERNEL, "%s %s", hdev->name,
+ idev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s %s", hdev->name,
name_suffix);
if (!idev->name)
goto err_name;
--
2.40.1
^ permalink raw reply related
* Re: [PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name
From: Maxime Ripard @ 2023-08-24 9:03 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: linux-input, Benjamin Tissoires, Dmitry Torokhov, Jiri Kosina,
Maxime Ripard
In-Reply-To: <20230824061308.222021-1-sergeantsagara@protonmail.com>
On Thu, 24 Aug 2023 06:13:52 +0000, Rahul Rameshbabu wrote:
> Maxime Ripard analyzed the following situation involving a use-after-free caused
> by incorrect devres management.
>
> 1. input_dev name allocated as a resource referring to the same input_dev
> instance
>
> [ ... ]
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Thanks!
Maxime
^ permalink raw reply
* [PATCH v4 0/4] Input: support overlay objects on touchscreens
From: Javier Carrasco @ 2023-08-24 13:17 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
Some touchscreens are shipped with a physical layer on top of them where
a number of buttons and a resized touchscreen surface might be available.
In order to generate proper key events by overlay buttons and adjust the
touch events to a clipped surface, these patches offer a documented,
device-tree-based solution by means of helper functions.
An implementation for a specific touchscreen driver is also included.
The functions in ts-overlay provide a simple workflow to acquire
physical objects from the device tree, map them into the device driver
structures as overlay objects and generate events according to
the object descriptions.
This feature has been tested with a JT240MHQS-E3 display, which consists
of an st1624 as the base touchscreen and an overlay with two buttons and
a frame that clips its effective surface mounted on it.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Changes in v4:
- General: rename "touchscreen" to "touch" to include other consumers.
- PATCH 1/4: move touch-overlay feature to input core.
- PATCH 1/4, 3/4: set key caps and report key events without consumer's
intervention.
- PATCH 2/4: add missing 'required' field with the required properties.
- Link to v3: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v3-0-b4fb7fc4bab7@wolfvision.net
Changes in v3:
- General: rename "virtobj" and "virtual" to "overlay"
- PATCH 1/4: Make feature bool instead of tristate (selected by
supported touchscreens)
- Link to v2: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v2-0-f68a6bfe7a0f@wolfvision.net
Changes in v2:
- PATCH 1/4: remove preprocessor directives (the module is selected by
the drivers that support the feature). Typo in the commit message.
- PATCH 2/4: more detailed documentation. Images and examples were added.
- PATCH 3/4: select ts-virtobj automatically.
- Link to v1: https://lore.kernel.org/r/20230510-feature-ts_virtobj_patch-v1-0-5ae5e81bc264@wolfvision.net
---
Javier Carrasco (4):
Input: touch-overlay - Add touchscreen overlay object handling
dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties
Input: st1232 - add touch overlays handling
dt-bindings: input: touchscreen: st1232: add example with touch-overlay
.../input/touchscreen/sitronix,st1232.yaml | 40 +++
.../bindings/input/touchscreen/touchscreen.yaml | 152 ++++++++
MAINTAINERS | 7 +
drivers/input/Makefile | 2 +-
drivers/input/touch-overlay.c | 399 +++++++++++++++++++++
drivers/input/touchscreen/st1232.c | 70 +++-
include/linux/input/touch-overlay.h | 34 ++
7 files changed, 689 insertions(+), 15 deletions(-)
---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230510-feature-ts_virtobj_patch-e267540aae74
Best regards,
--
Javier Carrasco <javier.carrasco@wolfvision.net>
^ permalink raw reply
* [PATCH v4 3/4] Input: st1232 - add touch overlays handling
From: Javier Carrasco @ 2023-08-24 13:17 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v4-0-5c6c0fc1eed6@wolfvision.net>
Use touch-overlay to support overlay objects such as buttons and resized
frames defined in the device tree.
If an overlay-touchscreen is provided, ignore touch events outside of
the area defined by its properties. Otherwise, transform the event
coordinates to the overlay-touchscreen x and y-axis if required.
If buttons are provided, register an additional device to report key
events separately. A key event will be generated if the coordinates
of a touch event are within the area defined by the button properties.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
drivers/input/touchscreen/st1232.c | 70 ++++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 6475084aee1b..d22f7d57f468 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -22,6 +22,7 @@
#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/types.h>
+#include <linux/input/touch-overlay.h>
#define ST1232_TS_NAME "st1232-ts"
#define ST1633_TS_NAME "st1633-ts"
@@ -56,6 +57,8 @@ struct st1232_ts_data {
struct touchscreen_properties prop;
struct dev_pm_qos_request low_latency_req;
struct gpio_desc *reset_gpio;
+ struct input_dev *overlay_keypad;
+ struct touch_overlay_map *overlay_map;
const struct st_chip_info *chip_info;
int read_buf_len;
u8 *read_buf;
@@ -130,6 +133,7 @@ static int st1232_ts_read_resolution(struct st1232_ts_data *ts, u16 *max_x,
static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
{
struct input_dev *input = ts->input_dev;
+ struct input_dev *keypad = ts->overlay_keypad;
struct input_mt_pos pos[ST_TS_MAX_FINGERS];
u8 z[ST_TS_MAX_FINGERS];
int slots[ST_TS_MAX_FINGERS];
@@ -138,14 +142,20 @@ static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
for (i = 0; i < ts->chip_info->max_fingers; i++) {
u8 *buf = &ts->read_buf[i * 4];
+ bool contact = buf[0] & BIT(7);
+ unsigned int x, y;
- if (buf[0] & BIT(7)) {
- unsigned int x = ((buf[0] & 0x70) << 4) | buf[1];
- unsigned int y = ((buf[0] & 0x07) << 8) | buf[2];
-
- touchscreen_set_mt_pos(&pos[n_contacts],
- &ts->prop, x, y);
+ if (contact) {
+ x = ((buf[0] & 0x70) << 4) | buf[1];
+ y = ((buf[0] & 0x07) << 8) | buf[2];
+ }
+ if (touch_overlay_process_event(ts->overlay_map, keypad,
+ contact ? &x : NULL,
+ contact ? &y : NULL, i))
+ continue;
+ if (contact) {
+ touchscreen_set_mt_pos(&pos[n_contacts], &ts->prop, x, y);
/* st1232 includes a z-axis / touch strength */
if (ts->chip_info->have_z)
z[n_contacts] = ts->read_buf[i + 6];
@@ -226,6 +236,7 @@ static int st1232_ts_probe(struct i2c_client *client)
const struct st_chip_info *match;
struct st1232_ts_data *ts;
struct input_dev *input_dev;
+ struct input_dev *overlay_keypad;
u16 max_x, max_y;
int error;
@@ -263,8 +274,13 @@ static int st1232_ts_probe(struct i2c_client *client)
if (!input_dev)
return -ENOMEM;
+ overlay_keypad = devm_input_allocate_device(&client->dev);
+ if (!overlay_keypad)
+ return -ENOMEM;
+
ts->client = client;
ts->input_dev = input_dev;
+ ts->overlay_keypad = overlay_keypad;
ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL,
GPIOD_OUT_HIGH);
@@ -286,24 +302,37 @@ static int st1232_ts_probe(struct i2c_client *client)
input_dev->name = "st1232-touchscreen";
input_dev->id.bustype = BUS_I2C;
+ overlay_keypad->name = "st1232-keypad";
+ overlay_keypad->id.bustype = BUS_I2C;
/* Wait until device is ready */
error = st1232_ts_wait_ready(ts);
if (error)
return error;
- /* Read resolution from the chip */
- error = st1232_ts_read_resolution(ts, &max_x, &max_y);
- if (error) {
- dev_err(&client->dev,
- "Failed to read resolution: %d\n", error);
- return error;
- }
-
if (ts->chip_info->have_z)
input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
ts->chip_info->max_area, 0, 0);
+ /* map overlay objects if defined in the device tree */
+ ts->overlay_map = touch_overlay_map_overlay(ts->overlay_keypad);
+ if (IS_ERR(ts->overlay_map))
+ return PTR_ERR(ts->overlay_map);
+
+ if (touch_overlay_mapped_touchscreen(ts->overlay_map)) {
+ /* Read resolution from the overlay touchscreen if defined */
+ touch_overlay_get_touchscreen_abs(ts->overlay_map,
+ &max_x, &max_y);
+ } else {
+ /* Read resolution from the chip */
+ error = st1232_ts_read_resolution(ts, &max_x, &max_y);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to read resolution: %d\n", error);
+ return error;
+ }
+ }
+
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, max_x, 0, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
@@ -335,6 +364,19 @@ static int st1232_ts_probe(struct i2c_client *client)
return error;
}
+ /* Register keypad input device if overlay buttons were defined */
+ if (touch_overlay_mapped_buttons(ts->overlay_map)) {
+ error = input_register_device(ts->overlay_keypad);
+ if (error) {
+ dev_err(&client->dev,
+ "Unable to register %s input device\n",
+ overlay_keypad->name);
+ return error;
+ }
+ } else {
+ input_free_device(ts->overlay_keypad);
+ }
+
i2c_set_clientdata(client, ts);
return 0;
--
2.39.2
^ permalink raw reply related
* [PATCH v4 4/4] dt-bindings: input: touchscreen: st1232: add example with touch-overlay
From: Javier Carrasco @ 2023-08-24 13:17 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v4-0-5c6c0fc1eed6@wolfvision.net>
The st1232 driver supports the overlay-touchscreen and overlay-buttons
objects defined in the generic touchscreen bindings and implemented in
the touch-overlay module. Add an example where nodes for an overlay
touchscreen and overlay buttons are defined.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
.../input/touchscreen/sitronix,st1232.yaml | 40 ++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
index 1d8ca19fd37a..857b611f84c2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
@@ -48,3 +48,43 @@ examples:
gpios = <&gpio1 166 0>;
};
};
+ - |
+ #include <dt-bindings/input/linux-event-codes.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@55 {
+ compatible = "sitronix,st1232";
+ reg = <0x55>;
+ interrupts = <2 0>;
+ gpios = <&gpio1 166 0>;
+
+ overlay-touchscreen {
+ x-origin = <0>;
+ x-size = <240>;
+ y-origin = <40>;
+ y-size = <280>;
+ };
+
+ overlay-buttons {
+ button-light {
+ label = "Camera light";
+ linux,code = <KEY_LIGHTS_TOGGLE>;
+ x-origin = <40>;
+ x-size = <40>;
+ y-origin = <0>;
+ y-size = <40>;
+ };
+
+ button-power {
+ label = "Power";
+ linux,code = <KEY_POWER>;
+ x-origin = <160>;
+ x-size = <40>;
+ y-origin = <0>;
+ y-size = <40>;
+ };
+ };
+ };
+ };
--
2.39.2
^ permalink raw reply related
* [PATCH v4 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties
From: Javier Carrasco @ 2023-08-24 13:17 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v4-0-5c6c0fc1eed6@wolfvision.net>
The overlay-touchscreen object defines an area within the touchscreen
where touch events are reported and their coordinates get converted to
the overlay origin. This object avoids getting events from areas that
are physically hidden by overlay frames.
For touchscreens where overlay buttons on the touchscreen surface are
provided, the overlay-buttons object contains a node for every button
and the key event that should be reported when pressed.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
.../bindings/input/touchscreen/touchscreen.yaml | 152 +++++++++++++++++++++
1 file changed, 152 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
index 895592da9626..d90cbb4932b5 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
@@ -80,6 +80,158 @@ properties:
touchscreen-y-plate-ohms:
description: Resistance of the Y-plate in Ohms
+ overlay-touchscreen:
+ description: Clipped touchscreen area
+
+ This object can be used to describe a frame that restricts the area
+ within touch events are reported, ignoring the events that occur outside
+ this area. This is of special interest if the touchscreen is shipped
+ with a physical overlay on top of it with a frame that hides some part
+ of the original touchscreen area.
+
+ The x-origin and y-origin properties of this object define the offset of
+ a new origin from where the touchscreen events are referenced.
+ This offset is applied to the events accordingly. The x-size and y-size
+ properties define the size of the overlay-touchscreen (effective area).
+
+ The following example shows the new touchscreen area and the new origin
+ (0',0') for the touch events generated by the device.
+
+ Touchscreen (full area)
+ ┌────────────────────────────────────────┐
+ │ ┌───────────────────────────────┐ │
+ │ │ │ │
+ │ ├ y-size │ │
+ │ │ │ │
+ │ │ overlay-touchscreen │ │
+ │ │ │ │
+ │ │ │ │
+ │ │ x-size │ │
+ │ ┌└──────────────┴────────────────┘ │
+ │(0',0') │
+ ┌└────────────────────────────────────────┘
+ (0,0)
+
+ where (0',0') = (0+x-origin,0+y-origin)
+
+ type: object
+
+ properties:
+ x-origin:
+ description: horizontal origin of the clipped area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ y-origin:
+ description: vertical origin of the clipped area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ x-size:
+ description: horizontal resolution of the clipped area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ y-size:
+ description: vertical resolution of the clipped area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ required:
+ - x-origin
+ - y-origin
+ - x-size
+ - y-size
+
+ overlay-buttons:
+ description: list of nodes defining the buttons on the touchscreen
+
+ This object can be used to describe buttons on the touchscreen area,
+ reporting the touch events on their surface as key events instead of
+ the original touch events.
+
+ This is of special interest if the touchscreen is shipped with a
+ physical overlay on top of it where a number of buttons with some
+ predefined functionality are printed. In that case a specific behavior
+ is expected from those buttons instead of raw touch events.
+
+ The overlay-buttons properties define a per-button area as well as an
+ origin relative to the real touchscreen origin. Touch events within the
+ button area are reported as the key event defined in the linux,code
+ property. Given that the key events do not provide coordinates, the
+ button origin is only used to place the button area on the touchscreen
+ surface. Any event outside the overlay-buttons object is reported as a
+ touch event with no coordinate transformation.
+
+ The following example shows a touchscreen with a single button on it
+
+ Touchscreen (full area)
+ ┌───────────────────────────────────┐
+ │ │
+ │ │
+ │ ┌─────────┐ │
+ │ │button 0 │ │
+ │ │KEY_POWER│ │
+ │ └─────────┘ │
+ │ │
+ │ │
+ ┌└───────────────────────────────────┘
+ (0,0)
+
+ The overlay-buttons object can be combined with the overlay-touchscreen
+ object as shown in the following example. In that case only the events
+ within the overlay-touchscreen object are reported as touch events.
+
+ Touchscreen (full area)
+ ┌─────────┬──────────────────────────────┐
+ │ │ │
+ │ │ ┌───────────────────────┐ │
+ │ button 0│ │ │ │
+ │KEY_POWER│ │ │ │
+ │ │ │ │ │
+ ├─────────┤ │ overlay-touchscreen │ │
+ │ │ │ │ │
+ │ │ │ │ │
+ │ button 1│ │ │ │
+ │ KEY_INFO│ ┌└───────────────────────┘ │
+ │ │(0',0') │
+ ┌└─────────┴──────────────────────────────┘
+ (0,0)
+
+ type: object
+
+ patternProperties:
+ '^button-':
+ type: object
+ description:
+ Each button (key) is represented as a sub-node.
+
+ properties:
+ label:
+ $ref: /schemas/types.yaml#/definitions/string
+ description: descriptive name of the button
+
+ linux,code: true
+
+ x-origin:
+ description: horizontal origin of the button area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ y-origin:
+ description: vertical origin of the button area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ x-size:
+ description: horizontal resolution of the button area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ y-size:
+ description: vertical resolution of the button area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ required:
+ - linux,code
+ - x-origin
+ - y-origin
+ - x-size
+ - y-size
+
dependencies:
touchscreen-size-x: [ touchscreen-size-y ]
touchscreen-size-y: [ touchscreen-size-x ]
--
2.39.2
^ permalink raw reply related
* [PATCH v4 1/4] Input: touch-overlay - Add touchscreen overlay object handling
From: Javier Carrasco @ 2023-08-24 13:17 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v4-0-5c6c0fc1eed6@wolfvision.net>
Some touch devices provide mechanical overlays with different objects
like buttons or clipped touchscreen surfaces.
In order to support these objects, add a series of helper functions
to the input subsystem to transform them into overlay objects via
device tree nodes.
These overlay objects consume the raw touch events and report the
expected input events depending on the object properties.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
MAINTAINERS | 7 +
drivers/input/Makefile | 2 +-
drivers/input/touch-overlay.c | 399 ++++++++++++++++++++++++++++++++++++
include/linux/input/touch-overlay.h | 34 +++
4 files changed, 441 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3be1bdfe8ecc..dbc0d315d3d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21625,6 +21625,13 @@ W: https://github.com/srcres258/linux-doc
T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
F: Documentation/translations/zh_TW/
+TOUCH OVERLAY OBJECTS
+M: Javier Carrasco <javier.carrasco@wolfvision.net>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: drivers/input/touch-overlay.c
+F: include/linux/input/touch-overlay.h
+
TTY LAYER
M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
M: Jiri Slaby <jirislaby@kernel.org>
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index c78753274921..393e9f4d00dc 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -7,7 +7,7 @@
obj-$(CONFIG_INPUT) += input-core.o
input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
-input-core-y += touchscreen.o
+input-core-y += touchscreen.o touch-overlay.o
obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
obj-$(CONFIG_INPUT_SPARSEKMAP) += sparse-keymap.o
diff --git a/drivers/input/touch-overlay.c b/drivers/input/touch-overlay.c
new file mode 100644
index 000000000000..007dbd994474
--- /dev/null
+++ b/drivers/input/touch-overlay.c
@@ -0,0 +1,399 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Helper functions for overlay objects on touchscreens
+ *
+ * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
+ */
+
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touch-overlay.h>
+#include <linux/module.h>
+#include <linux/property.h>
+
+enum touch_overlay_valid_objects {
+ TOUCH_OVERLAY_TS,
+ TOUCH_OVERLAY_BTN,
+};
+
+static const char *const object_names[] = {
+ [TOUCH_OVERLAY_TS] = "overlay-touchscreen",
+ [TOUCH_OVERLAY_BTN] = "overlay-buttons",
+};
+
+struct touch_overlay_segment {
+ u32 x_origin;
+ u32 y_origin;
+ u32 x_size;
+ u32 y_size;
+};
+
+struct touch_overlay_button {
+ struct touch_overlay_segment segment;
+ u32 key;
+ bool pressed;
+ int slot;
+};
+
+static int touch_overlay_get_segment_props(struct fwnode_handle *segment_node,
+ struct touch_overlay_segment *segment)
+{
+ int error;
+
+ error = fwnode_property_read_u32(segment_node, "x-origin",
+ &segment->x_origin);
+ if (error < 0)
+ return error;
+
+ error = fwnode_property_read_u32(segment_node, "y-origin",
+ &segment->y_origin);
+ if (error < 0)
+ return error;
+
+ error = fwnode_property_read_u32(segment_node, "x-size",
+ &segment->x_size);
+ if (error < 0)
+ return error;
+
+ error = fwnode_property_read_u32(segment_node, "y-size",
+ &segment->y_size);
+ if (error < 0)
+ return error;
+
+ return 0;
+}
+
+static int
+touch_overlay_get_button_properties(struct device *dev,
+ struct fwnode_handle *overlay_node,
+ struct touch_overlay_button *btn)
+{
+ struct fwnode_handle *btn_node;
+ int error;
+ int j = 0;
+
+ fwnode_for_each_child_node(overlay_node, btn_node) {
+ error = touch_overlay_get_segment_props(btn_node,
+ &btn[j].segment);
+ if (error < 0)
+ goto button_put;
+
+ error = fwnode_property_read_u32(btn_node, "linux,code",
+ &btn[j].key);
+ if (error < 0)
+ goto button_put;
+
+ dev_dbg(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
+ btn[j].segment.x_origin, btn[j].segment.y_origin,
+ btn[j].segment.x_size, btn[j].segment.y_size, btn[j].key);
+ j++;
+ }
+
+ return 0;
+
+button_put:
+ fwnode_handle_put(btn_node);
+ return error;
+}
+
+static void touch_overlay_set_button_caps(struct touch_overlay_map *map,
+ struct input_dev *dev)
+{
+ int i;
+
+ for (i = 0; i < map->button_count; i++)
+ input_set_capability(dev, EV_KEY, map->buttons[i].key);
+}
+
+static int touch_overlay_count_buttons(struct device *dev)
+{
+ struct fwnode_handle *overlay;
+ struct fwnode_handle *button;
+ int count = 0;
+
+ overlay = device_get_named_child_node(dev,
+ object_names[TOUCH_OVERLAY_BTN]);
+ if (!overlay)
+ return 0;
+
+ fwnode_for_each_child_node(overlay, button)
+ count++;
+ fwnode_handle_put(overlay);
+
+ return count;
+}
+
+static int touch_overlay_map_touchscreen(struct device *dev,
+ struct touch_overlay_map *map)
+{
+ struct fwnode_handle *ts_node;
+ int error = 0;
+
+ ts_node = device_get_named_child_node(dev,
+ object_names[TOUCH_OVERLAY_TS]);
+ if (!ts_node)
+ return 0;
+
+ map->touchscreen =
+ devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
+ if (!map->touchscreen) {
+ error = -ENOMEM;
+ goto handle_put;
+ }
+ error = touch_overlay_get_segment_props(ts_node, map->touchscreen);
+ if (error < 0)
+ goto handle_put;
+
+ map->overlay_touchscreen = true;
+ dev_dbg(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
+ map->touchscreen->x_origin, map->touchscreen->y_origin,
+ map->touchscreen->x_size, map->touchscreen->y_size);
+
+handle_put:
+ fwnode_handle_put(ts_node);
+
+ return error;
+}
+
+static int touch_overlay_map_buttons(struct device *dev,
+ struct touch_overlay_map *map)
+{
+ struct fwnode_handle *button;
+ u32 count;
+ int error = 0;
+
+ count = touch_overlay_count_buttons(dev);
+ if (!count)
+ return 0;
+
+ map->buttons = devm_kcalloc(dev, count,
+ sizeof(*map->buttons), GFP_KERNEL);
+ if (!map->buttons) {
+ error = -ENOMEM;
+ goto map_buttons_ret;
+ }
+ button = device_get_named_child_node(dev,
+ object_names[TOUCH_OVERLAY_BTN]);
+ if (unlikely(!button)) {
+ error = -ENODEV;
+ goto map_buttons_ret;
+ }
+
+ error = touch_overlay_get_button_properties(dev, button,
+ map->buttons);
+
+ if (error < 0)
+ goto map_buttons_put;
+
+ map->button_count = count;
+
+map_buttons_put:
+ fwnode_handle_put(button);
+map_buttons_ret:
+ return error;
+}
+
+static bool touch_overlay_defined_objects(struct device *dev)
+{
+ struct fwnode_handle *obj_node;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(object_names); i++) {
+ obj_node = device_get_named_child_node(dev, object_names[i]);
+ if (obj_node) {
+ fwnode_handle_put(obj_node);
+ return true;
+ }
+ fwnode_handle_put(obj_node);
+ }
+
+ return false;
+}
+
+/**
+ * touch_overlay_map_overlay - map overlay objects from the device tree and set
+ * key capabilities if buttons are defined.
+ * @keypad: pointer to the already allocated input_dev.
+ *
+ * Returns a pointer to the object mapping struct.
+ *
+ * If a keypad input device is provided and overlay buttons are defined,
+ * its button capabilities are set accordingly.
+ */
+struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad)
+{
+ struct device *dev = keypad->dev.parent;
+ struct touch_overlay_map *map = NULL;
+ int error;
+
+ if (!touch_overlay_defined_objects(dev))
+ return NULL;
+
+ map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
+ if (!map) {
+ error = -ENOMEM;
+ goto map_error;
+ }
+ error = touch_overlay_map_touchscreen(dev, map);
+ if (error < 0)
+ goto map_error;
+
+ error = touch_overlay_map_buttons(dev, map);
+ if (error < 0)
+ goto map_error;
+
+ touch_overlay_set_button_caps(map, keypad);
+
+ return map;
+
+map_error:
+ return ERR_PTR(error);
+}
+EXPORT_SYMBOL(touch_overlay_map_overlay);
+
+/**
+ * touch_overlay_get_touchscreen_abs - get abs size from the overlay node
+ * @map: pointer to the struct that holds the object mapping
+ * @x: horizontal abs
+ * @y: vertical abs
+ *
+ */
+void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map, u16 *x,
+ u16 *y)
+{
+ *x = map->touchscreen->x_size - 1;
+ *y = map->touchscreen->y_size - 1;
+}
+EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
+
+static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
+ u32 x, u32 y)
+{
+ if (!seg)
+ return false;
+
+ if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
+ y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
+ return true;
+
+ return false;
+}
+
+/**
+ * touch_overlay_mapped_touchscreen - check if an overlay touchscreen is mapped
+ * @map: pointer to the struct that holds the object mapping
+ *
+ * Returns true if an overlay touchscreen is mapped or false otherwise.
+ */
+bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map)
+{
+ if (!map || !map->overlay_touchscreen)
+ return false;
+
+ return true;
+}
+EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
+
+/**
+ * touch_overlay_mapped_buttons - check if overlay buttons are mapped
+ * @map: pointer to the struct that holds the object mapping
+ *
+ * Returns true if overlay buttons mapped or false otherwise.
+ */
+bool touch_overlay_mapped_buttons(struct touch_overlay_map *map)
+{
+ if (!map || !map->button_count)
+ return false;
+
+ return true;
+}
+EXPORT_SYMBOL(touch_overlay_mapped_buttons);
+
+static bool touch_overlay_mt_on_touchscreen(struct touch_overlay_map *map,
+ u32 *x, u32 *y)
+{
+ bool contact = x && y;
+
+ if (!touch_overlay_mapped_touchscreen(map))
+ return true;
+
+ /* Let the caller handle events with no coordinates (release) */
+ if (!contact)
+ return false;
+
+ if (touch_overlay_segment_event(map->touchscreen, *x, *y)) {
+ *x -= map->touchscreen->x_origin;
+ *y -= map->touchscreen->y_origin;
+ return true;
+ }
+
+ return false;
+}
+
+static bool touch_overlay_button_event(struct input_dev *input,
+ struct touch_overlay_button *button,
+ const u32 *x, const u32 *y, u32 slot)
+{
+ bool contact = x && y;
+
+ if (!contact && button->pressed && button->slot == slot) {
+ button->pressed = false;
+ input_report_key(input, button->key, false);
+ input_sync(input);
+ return true;
+ } else if (contact && touch_overlay_segment_event(&button->segment,
+ *x, *y)) {
+ button->pressed = true;
+ button->slot = slot;
+ input_report_key(input, button->key, true);
+ input_sync(input);
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * touch_overlay_process_event - process input events according to the overlay
+ * mapping. This function acts as a filter to release the calling driver from
+ * the events that are either related to overlay buttons or out of the overlay
+ * touchscreen area if defined.
+ * @map: pointer to the struct that holds the object mapping
+ * @input: pointer to the input device associated to the event
+ * @x: pointer to the x coordinate (NULL if not available - no contact)
+ * @y: pointer to the y coordinate (NULL if not available - no contact)
+ * @slot: slot associated to the event
+ *
+ * Returns true if the event was processed (reported for valid key events
+ * and dropped for events outside the overlay touchscreen area) or false
+ * if the event must be processed by the caller. In that case this function
+ * shifts the (x,y) coordinates to the overlay touchscreen axis if required
+ */
+bool touch_overlay_process_event(struct touch_overlay_map *map,
+ struct input_dev *input,
+ u32 *x, u32 *y, u32 slot)
+{
+ int i;
+
+ if (!map)
+ return false;
+
+ /* buttons must be prioritized over overlay touchscreens to account for
+ * overlappings e.g. a button inside the touchscreen area
+ */
+ if (touch_overlay_mapped_buttons(map)) {
+ for (i = 0; i < map->button_count; i++) {
+ if (touch_overlay_button_event(input, &map->buttons[i],
+ x, y, slot))
+ return true;
+ }
+ }
+ /* valid touch events on the overlay touchscreen are left for the client
+ * to be processed/reported according to its (possibly) unique features
+ */
+ return !touch_overlay_mt_on_touchscreen(map, x, y);
+}
+EXPORT_SYMBOL(touch_overlay_process_event);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
new file mode 100644
index 000000000000..3e0db813dc34
--- /dev/null
+++ b/include/linux/input/touch-overlay.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
+ */
+
+#ifndef _TOUCH_OVERLAY
+#define _TOUCH_OVERLAY
+
+#include <linux/types.h>
+
+struct input_dev;
+struct device;
+
+struct touch_overlay_map {
+ struct touch_overlay_segment *touchscreen;
+ bool overlay_touchscreen;
+ struct touch_overlay_button *buttons;
+ u32 button_count;
+};
+
+struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad);
+
+void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map,
+ u16 *x, u16 *y);
+
+bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map);
+
+bool touch_overlay_mapped_buttons(struct touch_overlay_map *map);
+
+bool touch_overlay_process_event(struct touch_overlay_map *map,
+ struct input_dev *input,
+ u32 *x, u32 *y, u32 slot);
+
+#endif
--
2.39.2
^ permalink raw reply related
* [PATCH] input: docs: pxrc: remove reference to phoenix-sim
From: Marcus Folkesson @ 2023-08-24 13:55 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet
Cc: linux-input, linux-doc, linux-kernel, Marcus Folkesson
The reference undeniably points to something unrelated nowadays.
Remove it.
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
Documentation/input/devices/pxrc.rst | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
index ca11f646bae8..5a86df4ad079 100644
--- a/Documentation/input/devices/pxrc.rst
+++ b/Documentation/input/devices/pxrc.rst
@@ -5,7 +5,7 @@ pxrc - PhoenixRC Flight Controller Adapter
:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
This driver let you use your own RC controller plugged into the
-adapter that comes with PhoenixRC [1]_ or other compatible adapters.
+adapter that comes with PhoenixRC or other compatible adapters.
The adapter supports 7 analog channels and 1 digital input switch.
@@ -41,7 +41,7 @@ Manual Testing
==============
To test this driver's functionality you may use `input-event` which is part of
-the `input layer utilities` suite [2]_.
+the `input layer utilities` suite [1]_.
For example::
@@ -53,5 +53,4 @@ To print all input events from input `devnr`.
References
==========
-.. [1] http://www.phoenix-sim.com/
-.. [2] https://www.kraxel.org/cgit/input/
+.. [1] https://www.kraxel.org/cgit/input/
---
base-commit: 7214161111af87aeee721ab1b2808fcb4421aff7
change-id: 20230824-pxrc-doc-1addbaa2250f
Best regards,
--
Marcus Folkesson <marcus.folkesson@gmail.com>
^ permalink raw reply related
* Re: [PATCH] input: docs: pxrc: remove reference to phoenix-sim
From: Marcus Folkesson @ 2023-08-24 15:50 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet; +Cc: linux-input, linux-doc, linux-kernel
In-Reply-To: <20230824-pxrc-doc-v1-1-038b75a2ef05@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 296 bytes --]
On Thu, Aug 24, 2023 at 03:55:50PM +0200, Marcus Folkesson wrote:
> The reference undeniably points to something unrelated nowadays.
> Remove it.
>
Sorry, I forgot:
Suggested-by: Mark Olsson <mark@markolsson.se>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name
From: Benjamin Tissoires @ 2023-08-24 16:19 UTC (permalink / raw)
To: linux-input, Rahul Rameshbabu
Cc: Benjamin Tissoires, Jiri Kosina, Dmitry Torokhov, Maxime Ripard
In-Reply-To: <20230824061308.222021-1-sergeantsagara@protonmail.com>
On Thu, 24 Aug 2023 06:13:52 +0000, Rahul Rameshbabu wrote:
> Maxime Ripard analyzed the following situation involving a use-after-free caused
> by incorrect devres management.
>
> 1. input_dev name allocated as a resource referring to the same input_dev
> instance
> 2. The input_dev is eventually unregistered
> 3. Unregistering the device first involves releasing devres managed resources
> tied to the input_dev
> 4. A uevent is then fired for the input_dev, referencing various members of
> the input_dev including the name
> 5. This leads to a use-after-free in the context of the triggered uevent
>
> [...]
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.6/devm-fixes), thanks!
[1/3] HID: uclogic: Correct devm device reference for hidinput input_dev name
https://git.kernel.org/hid/hid/c/dd613a4e45f8
[2/3] HID: multitouch: Correct devm device reference for hidinput input_dev name
https://git.kernel.org/hid/hid/c/479439463529
[3/3] HID: nvidia-shield: Reference hid_device devm allocation of input_dev name
https://git.kernel.org/hid/hid/c/197d3143520f
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
From: Julius Zint @ 2023-08-24 17:35 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Jingoo Han, Jiri Kosina, Benjamin Tissoires,
Helge Deller, Thomas Weißschuh, linux-kernel, dri-devel,
linux-input, linux-fbdev
In-Reply-To: <20230821163631.GA214013@aspen.lan>
On 21.08.23 18:36, Daniel Thompson wrote:
>> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>> If you have a LCD backlight adjustable by LED class driver, say Y
>> to enable this driver.
>>
>> +config BACKLIGHT_HID
>> + tristate "VESA VCP HID Backlight Driver"
>> + depends on HID
>> + help
>> + If you have an external display with VESA compliant HID brightness
>> + controls then say Y to enable this backlight driver. Currently the
>> + only supported device is the Apple Studio Display.
> This contradicts the description which says you write the driver to the
> standard but only tested on Apple Studio Display. There is no need to
> spell what has been tested in the Kconfig text. Remove the final
> sentence!
Will remove it in v4.
>> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
>> new file mode 100644
>> index 000000000000..b40f8f412ee2
>> --- /dev/null
>> +++ b/drivers/video/backlight/hid_bl.c
>> <snip>
>> +static void hid_bl_remove(struct hid_device *hdev)
>> +{
>> + struct backlight_device *bl;
>> + struct hid_bl_data *data;
>> +
>> + hid_dbg(hdev, "remove\n");
> This message probably should be removed (if you want to know if a function was
> executed use ftrace).
>
>
>> + bl = hid_get_drvdata(hdev);
>> + data = bl_get_data(bl);
>> +
>> + devm_backlight_device_unregister(&hdev->dev, bl);
>> + hid_hw_close(hdev);
>> + hid_hw_stop(hdev);
>> + hid_set_drvdata(hdev, NULL);
>> + devm_kfree(&hdev->dev, data);
>> +}
>> +
>> +static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
>> +{
>> + struct hid_field *field;
>> + int result;
>> +
>> + field = data->input_field;
>> + hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
>> + hid_hw_wait(data->hdev);
>> + result = *field->new_value;
>> + hid_dbg(data->hdev, "get brightness: %d\n", result);
> To be honest I'm a little dubious about *all* the hid_dbg() calls. They
> add very little value (e.g. they are useful to get the driver working
> but not that important to keeping it working). As such I don't think
> they are worth the clutter in a CONFIG_DYNAMIC_DEBUG kernel.
>
> Note this is strictly for the hid_dbg() stuff... the hid_err() stuff in
> the probe error paths are much more useful!
You are right, I will remove all hid_dbg calls in v4.
Thank you very much for the review.
Julius
^ 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