* Re: [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers
From: Silvan Jegen @ 2026-05-30 20:03 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Vicki Pfau, dmitry.torokhov, linux-input
In-Reply-To: <20260514005353.EF8D8C19425@smtp.kernel.org>
sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble.
> - [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition.
> - [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers.
> - [Low] Implicit type promotion in the max() macro causes a compiler warning.
> - [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors.
> --
>
> commit d72f0f8e0dadde2cacc27050b92088c2e5cbea0e
> Author: Vicki Pfau <vi@endrift.com>
> Subject: HID: nintendo: Add rumble support for Switch 2 controllers
>
> This commit introduces rumble support for Switch 2 controllers, handling both HD
> Rumble linear resonant actuators and eccentric rotating mass haptics. It adds
> state machines for rumble scheduling and output reporting via hdev.
>
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 26ef589f652c6..a7fe74d982475 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -850,10 +850,10 @@ config NINTENDO_FF
> > depends on HID_NINTENDO
> > select INPUT_FF_MEMLESS
> > help
> > - Say Y here if you have a Nintendo Switch controller and want to enable
> > - force feedback support for it. This works for both joy-cons, the pro
> > - controller, and the NSO N64 controller. For the pro controller, both
> > - rumble motors can be controlled individually.
> > + Say Y here if you have a Nintendo Switch or Switch 2 controller and want
> > + to enable force feedback support for it. This works for Joy-Cons, the Pro
> > + Controllers, and the NSO N64 and GameCube controller. For the Pro
> > + Controller, both rumble motors can be controlled individually.
>
> This isn't a bug, but is the help text accurate regarding individual motor
> control? The implementation in switch2_rumble_work() appears to encode the
> exact same rumble configuration to both the left and right motor offsets,
> which would make both motors vibrate identically.
As pointers to different offsets into the rumble_buffer are passed,
I assume that the vibrations will actually not be identical.
>
> > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> > index 2193afcdefabc..9801799a90758 100644
> > --- a/drivers/hid/hid-nintendo.c
> > +++ b/drivers/hid/hid-nintendo.c
>
> [ ... ]
>
> > @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type)
> > return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR;
> > }
> >
> > +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> > +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5])
> > +{
> > + buffer[0] = rumble->hi_freq;
> > + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2);
> > + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4);
> > + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6);
> > + buffer[4] = rumble->lo_amp >> 2;
> > +}
> > +
> > +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> > +{
> > + struct switch2_controller *ns2 = input_get_drvdata(dev);
> > +
> > + if (effect->type != FF_RUMBLE)
> > + return 0;
> > +
> > + guard(spinlock_irqsave)(&ns2->rumble_lock);
> > + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) {
> > + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude,
> > + effect->u.rumble.weak_magnitude >> 1);
>
> Will this max calculation trigger a compiler warning? The strict type checking
> in the max macro might fail because effect->u.rumble.weak_magnitude >> 1
> promotes to an int, while strong_magnitude is a u16.
Apparently it is not strict enough to emit a warning on my machine at
least ...
>
> > + } else {
> > + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16;
> > + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16;
> > + }
> > +
> > + if (ns2->hdev)
> > + schedule_delayed_work(&ns2->rumble_work, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static void switch2_rumble_work(struct work_struct *work)
> > +{
> > + struct switch2_controller *ns2 = container_of(to_delayed_work(work),
> > + struct switch2_controller, rumble_work);
> > + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64());
> > + unsigned long flags;
> > + bool active;
> > + int ret;
>
> [ ... ]
>
> > + if (active) {
> > + unsigned long interval = msecs_to_jiffies(4);
> > +
> > + if (!ns2->last_rumble_work)
> > + ns2->last_rumble_work = current_ms;
> > + else
> > + ns2->last_rumble_work += interval;
> > + schedule_delayed_work(&ns2->rumble_work,
> > + ns2->last_rumble_work + interval - current_ms);
>
> Does this code underflow the scheduled delay?
>
> The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(),
> but interval is calculated in jiffies using msecs_to_jiffies(). Since
> ns2->last_rumble_work is initialized to milliseconds but incremented by
> jiffies, subtracting current_ms could yield a negative value.
>
> On systems where HZ is not 1000, could this underflow the delay parameter and
> schedule the next work millions of jiffies in the future?
It's definitely suspicious that we are adding jiffies to milliseconds. We
should probably fix that.
>
> > + } else {
> > + ns2->last_rumble_work = 0;
> > + }
> > + spin_unlock_irqrestore(&ns2->rumble_lock, flags);
> > +
> > + if (!ns2->hdev) {
> > + cancel_delayed_work(&ns2->rumble_work);
> > + ret = -ENODEV;
> > + } else {
> > + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64);
> > + }
> > +
> > + if (ret < 0)
> > + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret);
>
> If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call.
> Could this result in a NULL pointer dereference, as the hid_dbg macro passes
> ns2->hdev to dev_dbg()?
Since we set `ret = -ENODEV` when ns2->hdev is NULL we would indeed end
up calling hid_dbg() with a NULL argument.
>
> Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there
> a time-of-check to time-of-use race where the compiler might reload the pointer
> between the check and hid_hw_output_report()?
>
> [ ... ]
>
> > @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id
> >
> > switch2_leds_create(ns2);
> >
> > +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> > + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) {
> > + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ;
> > + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ;
> > + }
>
> Are we checking ns2->ctlr_type before it is dynamically initialized?
>
> During probe, ns2->ctlr_type appears to be 0 since the struct is freshly
> allocated and the type is discovered later during the init state machine.
> This would cause the high and low frequencies to always be written.
As switch2_init_controller is called from the other driver's probe
function, this might be fine. To avoid writing these values if the
initialisation has not happened yet, we should probably guard against
a 0 value.
Cheers,
Silvan
>
> Since hd and sd share a union, could this corrupt the sd.error and
> sd.amplitude fields for actual GameCube controllers?
>
> > + spin_lock_init(&ns2->rumble_lock);
> > + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work);
> > +#endif
> > hid_set_drvdata(hdev, ns2);
^ permalink raw reply
* [PATCH 5.10 048/589] HID: core: clamp report_size in s32ton() to avoid undefined shift
From: Greg Kroah-Hartman @ 2026-05-30 15:58 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, linux-input, Jiri Kosina
In-Reply-To: <20260530160224.570625122@linuxfoundation.org>
5.10-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 69c02ffde6ed4d535fa4e693a9e572729cad3d0d upstream.
s32ton() shifts by n-1 where n is the field's report_size, a value that
comes directly from a HID device. The HID parser bounds report_size
only to <= 256, so a broken HID device can supply a report descriptor
with a wide field that triggers shift exponents up to 256 on a 32-bit
type when an output report is built via hid_output_field() or
hid_set_field().
Commit ec61b41918587 ("HID: core: fix shift-out-of-bounds in
hid_report_raw_event") added the same n > 32 clamp to the function
snto32(), but s32ton() was never given the same fix as I guess syzbot
hadn't figured out how to fuzz a device the same way.
Fix this up by just clamping the max value of n, just like snto32()
does.
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-core.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1354,6 +1354,9 @@ static u32 s32ton(__s32 value, unsigned
if (!value || !n)
return 0;
+ if (n > 32)
+ n = 32;
+
a = value >> (n - 1);
if (a && a != -1)
return value < 0 ? 1 << (n - 1) : (1 << (n - 1)) - 1;
^ permalink raw reply
* [PATCH 5.10 047/589] HID: alps: fix NULL pointer dereference in alps_raw_event()
From: Greg Kroah-Hartman @ 2026-05-30 15:58 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, Masaki Ota, linux-input, Jiri Kosina
In-Reply-To: <20260530160224.570625122@linuxfoundation.org>
5.10-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 1badfc4319224820d5d890f8eab6aa52e4e83339 upstream.
Commit ecfa6f34492c ("HID: Add HID_CLAIMED_INPUT guards in raw_event
callbacks missing them") attempted to fix up the HID drivers that had
missed the previous fix that was done in 2ff5baa9b527 ("HID: appleir:
Fix potential NULL dereference at raw event handle"), but the alps
driver was missed.
Fix this up by properly checking in the hid-alps driver that it had been
claimed correctly before attempting to process the raw event.
Fixes: 73196ebe134d ("HID: alps: add support for Alps T4 Touchpad device")
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: Masaki Ota <masaki.ota@jp.alps.com>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-alps.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -437,6 +437,9 @@ static int alps_raw_event(struct hid_dev
int ret = 0;
struct alps_dev *hdata = hid_get_drvdata(hdev);
+ if (!(hdev->claimed & HID_CLAIMED_INPUT) || !hdata->input)
+ return 0;
+
switch (hdev->product) {
case HID_PRODUCT_ID_T4_BTNLESS:
ret = t4_raw_event(hdata, data, size);
^ permalink raw reply
* Re: [PATCH 2/4] HID: core: introduce hid_safe_input_report()
From: Carlos Llamas @ 2026-05-30 17:43 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Bastien Nocera, Benjamin Tissoires, Jiri Kosina,
Filipe Laíns, Ping Cheng, Jason Gerecke, Viresh Kumar,
Johan Hovold, Alex Elder, Greg Kroah-Hartman, Lee Jones,
linux-input, linux-kernel, greybus-dev, linux-staging, linux-usb,
stable
In-Reply-To: <CAO-hwJ+EgC0pM6L6vGFEaRFt2Nwj5b-CCf_5e5VkvrXgdHrjNg@mail.gmail.com>
On Thu, Apr 16, 2026 at 04:46:28PM +0200, Benjamin Tissoires wrote:
> On Thu, Apr 16, 2026 at 11:41 AM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Wed, 2026-04-15 at 11:38 +0200, Benjamin Tissoires wrote:
> > > hid_input_report() is used in too many places to have a commit that
> > > doesn't cross subsystem borders. Instead of changing the API,
> > > introduce
> > > a new one when things matters in the transport layers:
> > > - usbhid
> > > - i2chid
> > >
> > > This effectively revert to the old behavior for those two transport
> > > layers.
> > >
> > > Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing
> > > bogus memset()")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > > ---
> > > drivers/hid/hid-core.c | 21 +++++++++++++++++++++
> > > drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++++---
> > > drivers/hid/usbhid/hid-core.c | 11 ++++++-----
> > > include/linux/hid.h | 2 ++
> > > 4 files changed, 33 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index a806820df7e5..cb0ad99e7a0a 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -2191,6 +2191,27 @@ int hid_input_report(struct hid_device *hid,
> > > enum hid_report_type type, u8 *data
> > > }
> > > EXPORT_SYMBOL_GPL(hid_input_report);
> > >
> > > +/**
> > > + * hid_safe_input_report - report data from lower layer (usb, bt...)
> > > + *
> > > + * @hid: hid device
> > > + * @type: HID report type (HID_*_REPORT)
> > > + * @data: report contents
> > > + * @bufsize: allocated size of the data buffer
> > > + * @size: useful size of data parameter
> > > + * @interrupt: distinguish between interrupt and control transfers
> > > + *
> > > + * This is data entry for lower layers.
> >
> > You probably want to explain why it should be used instead of
> > hid_input_report() in this doc blurb, and modify the hid_input_report()
> > docs to mention that this should be used.
>
> Good point. Sending v2 ASAP.
>
> >
> > Maybe hid_input_report() should also be marked as deprecated somehow,
> > to avoid new users?
>
> Well, it's not entirely deprecated because, for instance, in uhid we
> only have the buffer with the provided size around. So we can't be
> less restrictive in that precise case, and then switching to _safe
> will not change a bit.
>
> Cheers,
> Benjamin
Hi Benjamin, our CI started failing with commit 0a3fe972a7cb ("HID:
core: Mitigate potential OOB by removing bogus memset()"), so I was
hoping your patchset would fix this.
However, I just realized our call path goes through uhid precisely,
which still triggers the EINVAL error since uhid as not converted to
hid_safe_input_report().
My vague understanding though, is that uhid_event uses a static buffer
in ev->data[UHID_DATA_MAX], so maybe we can use that through
uhid_dev_input{2}()?
I ran the following path through our CI and it fixed our issue, so I
wanted to get your thoughts on this.
Carlos Llamas
---
drivers/hid/uhid.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 524b53a3c87b..37b60c3aaf66 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -595,8 +595,8 @@ static int uhid_dev_input(struct uhid_device *uhid, struct uhid_event *ev)
if (!READ_ONCE(uhid->running))
return -EINVAL;
- hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data,
- min_t(size_t, ev->u.input.size, UHID_DATA_MAX), 0);
+ hid_safe_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data, UHID_DATA_MAX,
+ min_t(size_t, ev->u.input.size, UHID_DATA_MAX), 0);
return 0;
}
@@ -606,8 +606,8 @@ static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
if (!READ_ONCE(uhid->running))
return -EINVAL;
- hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input2.data,
- min_t(size_t, ev->u.input2.size, UHID_DATA_MAX), 0);
+ hid_safe_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input2.data, UHID_DATA_MAX,
+ min_t(size_t, ev->u.input2.size, UHID_DATA_MAX), 0);
return 0;
}
^ permalink raw reply related
* [PATCH 5.15 057/776] HID: core: clamp report_size in s32ton() to avoid undefined shift
From: Greg Kroah-Hartman @ 2026-05-30 15:56 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, linux-input, Jiri Kosina
In-Reply-To: <20260530160240.228940103@linuxfoundation.org>
5.15-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 69c02ffde6ed4d535fa4e693a9e572729cad3d0d upstream.
s32ton() shifts by n-1 where n is the field's report_size, a value that
comes directly from a HID device. The HID parser bounds report_size
only to <= 256, so a broken HID device can supply a report descriptor
with a wide field that triggers shift exponents up to 256 on a 32-bit
type when an output report is built via hid_output_field() or
hid_set_field().
Commit ec61b41918587 ("HID: core: fix shift-out-of-bounds in
hid_report_raw_event") added the same n > 32 clamp to the function
snto32(), but s32ton() was never given the same fix as I guess syzbot
hadn't figured out how to fuzz a device the same way.
Fix this up by just clamping the max value of n, just like snto32()
does.
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-core.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1354,6 +1354,9 @@ static u32 s32ton(__s32 value, unsigned
if (!value || !n)
return 0;
+ if (n > 32)
+ n = 32;
+
a = value >> (n - 1);
if (a && a != -1)
return value < 0 ? 1 << (n - 1) : (1 << (n - 1)) - 1;
^ permalink raw reply
* [PATCH 5.15 056/776] HID: alps: fix NULL pointer dereference in alps_raw_event()
From: Greg Kroah-Hartman @ 2026-05-30 15:56 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, Masaki Ota, linux-input, Jiri Kosina
In-Reply-To: <20260530160240.228940103@linuxfoundation.org>
5.15-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 1badfc4319224820d5d890f8eab6aa52e4e83339 upstream.
Commit ecfa6f34492c ("HID: Add HID_CLAIMED_INPUT guards in raw_event
callbacks missing them") attempted to fix up the HID drivers that had
missed the previous fix that was done in 2ff5baa9b527 ("HID: appleir:
Fix potential NULL dereference at raw event handle"), but the alps
driver was missed.
Fix this up by properly checking in the hid-alps driver that it had been
claimed correctly before attempting to process the raw event.
Fixes: 73196ebe134d ("HID: alps: add support for Alps T4 Touchpad device")
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: Masaki Ota <masaki.ota@jp.alps.com>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-alps.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -437,6 +437,9 @@ static int alps_raw_event(struct hid_dev
int ret = 0;
struct alps_dev *hdata = hid_get_drvdata(hdev);
+ if (!(hdev->claimed & HID_CLAIMED_INPUT) || !hdata->input)
+ return 0;
+
switch (hdev->product) {
case HID_PRODUCT_ID_T4_BTNLESS:
ret = t4_raw_event(hdata, data, size);
^ permalink raw reply
* [PATCH 6.1 061/969] HID: core: clamp report_size in s32ton() to avoid undefined shift
From: Greg Kroah-Hartman @ 2026-05-30 15:53 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, linux-input, Jiri Kosina
In-Reply-To: <20260530160300.485627683@linuxfoundation.org>
6.1-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 69c02ffde6ed4d535fa4e693a9e572729cad3d0d upstream.
s32ton() shifts by n-1 where n is the field's report_size, a value that
comes directly from a HID device. The HID parser bounds report_size
only to <= 256, so a broken HID device can supply a report descriptor
with a wide field that triggers shift exponents up to 256 on a 32-bit
type when an output report is built via hid_output_field() or
hid_set_field().
Commit ec61b41918587 ("HID: core: fix shift-out-of-bounds in
hid_report_raw_event") added the same n > 32 clamp to the function
snto32(), but s32ton() was never given the same fix as I guess syzbot
hadn't figured out how to fuzz a device the same way.
Fix this up by just clamping the max value of n, just like snto32()
does.
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-core.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1358,6 +1358,9 @@ static u32 s32ton(__s32 value, unsigned
if (!value || !n)
return 0;
+ if (n > 32)
+ n = 32;
+
a = value >> (n - 1);
if (a && a != -1)
^ permalink raw reply
* [PATCH 6.1 060/969] HID: alps: fix NULL pointer dereference in alps_raw_event()
From: Greg Kroah-Hartman @ 2026-05-30 15:53 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, Masaki Ota, linux-input, Jiri Kosina
In-Reply-To: <20260530160300.485627683@linuxfoundation.org>
6.1-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 1badfc4319224820d5d890f8eab6aa52e4e83339 upstream.
Commit ecfa6f34492c ("HID: Add HID_CLAIMED_INPUT guards in raw_event
callbacks missing them") attempted to fix up the HID drivers that had
missed the previous fix that was done in 2ff5baa9b527 ("HID: appleir:
Fix potential NULL dereference at raw event handle"), but the alps
driver was missed.
Fix this up by properly checking in the hid-alps driver that it had been
claimed correctly before attempting to process the raw event.
Fixes: 73196ebe134d ("HID: alps: add support for Alps T4 Touchpad device")
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: Masaki Ota <masaki.ota@jp.alps.com>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-alps.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -437,6 +437,9 @@ static int alps_raw_event(struct hid_dev
int ret = 0;
struct alps_dev *hdata = hid_get_drvdata(hdev);
+ if (!(hdev->claimed & HID_CLAIMED_INPUT) || !hdata->input)
+ return 0;
+
switch (hdev->product) {
case HID_PRODUCT_ID_T4_BTNLESS:
ret = t4_raw_event(hdata, data, size);
^ permalink raw reply
* Re: [PATCH] HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()
From: sashiko-bot @ 2026-05-30 16:16 UTC (permalink / raw)
To: Jinmo Yang; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260530155930.128183-1-jinmo44.yang@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] A malicious USB device can trigger a massive heap buffer overflow by sending a HID report larger than the wacom pen_fifo capacity (e.g., > PAGE_SIZE).
--
commit 2421694466917c461a3ca5695c18542757de2d44
Author: Jinmo Yang <jinmo44.yang@gmail.com>
HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()
wacom_wac_queue_flush() is called via the .raw_event callback
(wacom_raw_event → wacom_wac_pen_serial_enforce → wacom_wac_queue_flush).
For USB HID devices, this callback is invoked from hid_irq_in(), which
is a URB completion handler running in atomic context. Using GFP_KERNEL
in this path can sleep, leading to a "scheduling while atomic" bug.
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..a25400b3e658d 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -74,7 +74,7 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
> unsigned int count;
> int err;
>
> - buf = kzalloc(size, GFP_KERNEL);
> + buf = kzalloc(size, GFP_ATOMIC);
> if (!buf) {
> kfifo_skip(fifo);
[Severity: Critical]
This is a pre-existing issue, but does this usage of kfifo_skip() point to a
similar problem in wacom_wac_queue_insert() in drivers/hid/wacom_sys.c?
If a malicious device spoofs a Wacom device with WACOM_QUIRK_TOOLSERIAL and
sends a report larger than PAGE_SIZE, wouldn't wacom_wac_queue_insert() be
called with size > PAGE_SIZE?
Since pen_fifo capacity is bounded to at most PAGE_SIZE, kfifo_avail(fifo) can
never naturally be >= size, causing the code to loop and blindly call
kfifo_skip(fifo):
drivers/hid/wacom_sys.c:wacom_wac_queue_insert() {
...
while (kfifo_avail(fifo) < size) {
if (!warned)
hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
warned = true;
kfifo_skip(fifo);
}
kfifo_in(fifo, raw_data, size);
...
Doesn't kfifo_skip(fifo) advance fifo->out even when empty, underflowing
fifo->in - fifo->out to a massive unsigned value?
Could this trick kfifo_avail(fifo) into returning a huge number, breaking the
loop and allowing kfifo_in() to write up to 16KB out of bounds into the 4KB
fifo->data buffer via lib/kfifo.c:kfifo_copy_in()?
lib/kfifo.c:kfifo_copy_in() {
...
memcpy(fifo->data, src + l, len - l);
...
> continue;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260530155930.128183-1-jinmo44.yang@gmail.com?part=1
^ permalink raw reply
* [PATCH] HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()
From: Jinmo Yang @ 2026-05-30 15:59 UTC (permalink / raw)
To: Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, linux-input, linux-kernel, Jinmo Yang, stable
wacom_wac_queue_flush() is called via the .raw_event callback
(wacom_raw_event → wacom_wac_pen_serial_enforce → wacom_wac_queue_flush).
For USB HID devices, this callback is invoked from hid_irq_in(), which
is a URB completion handler running in atomic context. Using GFP_KERNEL
in this path can sleep, leading to a "scheduling while atomic" bug.
Use GFP_ATOMIC instead. The existing code already handles allocation
failure by skipping the fifo entry and continuing.
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Fixes: 5e013ad20689 ("HID: wacom: Remove static WACOM_PKGLEN_MAX limit")
Cc: stable@vger.kernel.org
Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
---
drivers/hid/wacom_sys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index a32320b35..2e237bdd2 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -74,7 +74,7 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
unsigned int count;
int err;
- buf = kzalloc(size, GFP_KERNEL);
+ buf = kzalloc(size, GFP_ATOMIC);
if (!buf) {
kfifo_skip(fifo);
continue;
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v1] Input: Use named initializers for arrays of i2c_device_data
From: Uwe Kleine-König (The Capable Hub) @ 2026-05-30 12:02 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Anshul Dalal, Michael Hennerich, Yassine Oudjana, Linus Walleij,
Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Support Opensource, Nick Dyer, Hans de Goede, Job Noorman,
Mika Penttilä, Maxime Coquelin, Alexandre Torgue, Kees Cook,
bui duc phuc, Thorsten Blum, Yauhen Kharuzhy, Sakari Ailus,
Marco Crivellari, Minseong Kim, Ingo Molnar, Thomas Gleixner,
Oleh Kuzhylnyi, Marek Vasut, Krzysztof Kozlowski,
Geert Uytterhoeven, Josua Mayer, Michael Tretter, Jeff LaBundy,
Javier Carrasco, David Heidelberg, Petr Hodina, Svyatoslav Ryhel,
Johannes Kirchmair, Andy Shevchenko, Xichao Zhao, linux-input,
linux-kernel, linux-arm-kernel, platform-driver-x86, linux-stm32
In-Reply-To: <ahp7WuHiPYa7Q_pX@google.com>
[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]
Hello Dmitry,
On Fri, May 29, 2026 at 10:56:48PM -0700, Dmitry Torokhov wrote:
> I like the ability to properly set up pointers for driver data, however
> I do not think we should use named initializers for name field. As long
> as we are not planning on moving its position I like the brevity of just
> saying
>
> { "ili210x", .driver_data_ptr = &ili210x_chip },
>
> Can we keep the old style for the name field?
I have no strong opinion here, my patch introduces the named initializer
for both members for consistency. Note that my patches for all the other
subsystems (and also other *_device_id) do the same and you're the first
maintainer with that expressed preference.
I even sent a few patches that adapt initializers without .driver_data
(e.g.
https://lore.kernel.org/all/177932943265.3832404.360063047381979287.git-patchwork-notify@kernel.org/)
that got positive feedback so far.
So if you're ok with having input differ in style from (up to now) all
other subsystems, I can rework the patch.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v1] Input: Use named initializers for arrays of i2c_device_data
From: Dmitry Torokhov @ 2026-05-30 5:56 UTC (permalink / raw)
To: Uwe Kleine-König (The Capable Hub)
Cc: Anshul Dalal, Michael Hennerich, Yassine Oudjana, Linus Walleij,
Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Support Opensource, Nick Dyer, Hans de Goede, Job Noorman,
Mika Penttilä, Maxime Coquelin, Alexandre Torgue, Kees Cook,
bui duc phuc, Thorsten Blum, Yauhen Kharuzhy, Sakari Ailus,
Marco Crivellari, Minseong Kim, Ingo Molnar, Thomas Gleixner,
Oleh Kuzhylnyi, Marek Vasut, Krzysztof Kozlowski,
Geert Uytterhoeven, Josua Mayer, Michael Tretter, Jeff LaBundy,
Javier Carrasco, David Heidelberg, Petr Hodina, Svyatoslav Ryhel,
Johannes Kirchmair, Andy Shevchenko, Xichao Zhao, linux-input,
linux-kernel, linux-arm-kernel, platform-driver-x86, linux-stm32
In-Reply-To: <20260515164848.497608-2-u.kleine-koenig@baylibre.com>
Hi Uwe,
On Fri, May 15, 2026 at 06:48:47PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> While being less compact, using named initializers allows to more easily
> see which members of the structs are assigned which value without having
> to lookup the declaration of the struct. And it's also more robust
> against changes to the struct definition.
>
> The mentioned robustness is relevant for a planned change to struct
> i2c_device_id that replaces .driver_data by an anonymous union.
>
> This patch doesn't modify the compiled arrays, only their representation
> in source form benefits. The former was confirmed with x86 and arm64
> builds.
>
> Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
> ---
> Hello,
>
> the mentioned change to i2c_device_id is the following:
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 23ff24080dfd..aebd3a5e90af 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -477,7 +477,11 @@ struct rpmsg_device_id {
>
> struct i2c_device_id {
> char name[I2C_NAME_SIZE];
> - kernel_ulong_t driver_data; /* Data private to the driver */
> + union {
> + /* Data private to the driver */
> + kernel_ulong_t driver_data;
> + const void *driver_data_ptr;
> + };
> };
>
> /* pci_epf */
>
> and this requires that .driver_data is assigned via a named initializer
> for static data. This requirement isn't a bad one because named
> initializers are also much better readable than list initializers.
>
> The union added to struct i2c_device_id enables further cleanups like:
>
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 66ada7ffbc80..94aa4dc002c5 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -969,7 +969,7 @@ static int ili210x_i2c_probe(struct i2c_client *client)
>
> chip = device_get_match_data(dev);
> if (!chip && id)
> - chip = (const struct ili2xxx_chip *)id->driver_data;
> + chip = id->driver_data_ptr;
> if (!chip)
> return dev_err_probe(&client->dev, -ENODEV, "unknown device model\n");
>
> @@ -1049,10 +1049,10 @@ static int ili210x_i2c_probe(struct i2c_client *client)
> }
>
> static const struct i2c_device_id ili210x_i2c_id[] = {
> - { .name = "ili210x", .driver_data = (long)&ili210x_chip },
> - { .name = "ili2117", .driver_data = (long)&ili211x_chip },
> - { .name = "ili2120", .driver_data = (long)&ili212x_chip },
> - { .name = "ili251x", .driver_data = (long)&ili251x_chip },
> + { .name = "ili210x", .driver_data_ptr = &ili210x_chip },
> + { .name = "ili2117", .driver_data_ptr = &ili211x_chip },
> + { .name = "ili2120", .driver_data_ptr = &ili212x_chip },
> + { .name = "ili251x", .driver_data_ptr = &ili251x_chip },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, ili210x_i2c_id);
>
> that are an improvement for readability (again!) and it keeps some
> properties of the pointers (here: being const) without having to pay
> attention for that.
>
> My additional motivation for this effort is CHERI[1]. This is a hardware
> extension that uses 128 bit pointers but unsigned long is still 64 bit.
> So with CHERI you cannot store pointers in unsigned long variables.
I like the ability to properly set up pointers for driver data, however
I do not think we should use named initializers for name field. As long
as we are not planning on moving its position I like the brevity of just
saying
{ "ili210x", .driver_data_ptr = &ili210x_chip },
Can we keep the old style for the name field?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: ims-pcu - only expose sysfs attributes on control interface
From: sashiko-bot @ 2026-05-30 5:53 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <ahp23lj4_vXIeUBl@google.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Low] The `ims_pcu_ofn_is_attr_visible()` function introduced in the patch returns `0` when `intf != pcu->ctrl_intf` for the `ofn` attribute group. Because `ofn` is a named attribute group, returning `0` suppresses the individual files but still creates an empty 'ofn' directory on the data interface.
Pre-existing issues:
- [High] The `buttons->input` pointer is assigned after `input_register_device()`, and there is no memory barrier before setting `pcu->setup_complete = true`. On weakly ordered architectures, the CPU can reorder these stores, leading to a NULL pointer dereference in the interrupt handler (`ims_pcu_handle_response`) if an event arrives and observes `setup_complete == true` before `buttons->input` is initialized.
- [High] The driver claims the secondary data interface (`pcu->data_intf`) in `ims_pcu_probe` but fails to release it in `ims_pcu_disconnect` when unbinding the control interface. The data interface remains bound with its `intfdata` pointing to the freed `pcu` structure. Subsequent sysfs accesses on the data interface will dereference this freed pointer, resulting in a use-after-free (UAF).
- [High] A malicious USB device can provide an incorrect master interface number in the CDC Union descriptor. `ims_pcu_parse_cdc_data()` uses this to set `pcu->ctrl_intf` and assigns `intfdata` to it. If this differs from the interface currently being probed, the probed interface will have `NULL` driver data. When the sysfs core subsequently calls `ims_pcu_is_attr_visible()`, `usb_get_intfdata()` returns `NULL`, causing a NULL pointer dereference when accessing `pcu->ctrl_intf`.
- [High] The sysfs attribute store function `ims_pcu_reset_device` calls `ims_pcu_execute_command()` without acquiring `pcu->cmd_mutex`. This allows concurrent sysfs operations (e.g., setting backlight brightness or updating firmware) to interleave their command preparations, causing data corruption in the shared `pcu->urb_out_buf` and race conditions on the `pcu->cmd_done` completion structure.
- [High] In `ims_pcu_buffers_free()`, the input URB DMA buffer (`pcu->urb_in_buf`) is incorrectly freed using `pcu->max_out_size` instead of the `pcu->max_in_size` used during allocation.
--
commit e7e143a0135bed02c30cf00e17a9a25adf1ab6c4
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Input: ims-pcu - only expose sysfs attributes on control interface
When the driver was converted to use the driver core to instantiate device
attributes, they started appearing on all interfaces bound to the driver.
This patch updates the is_visible callbacks to verify that the interface
being accessed is indeed the control interface.
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..7c76bc82ab703 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1249,6 +1249,9 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
> struct ims_pcu *pcu = usb_get_intfdata(intf);
> umode_t mode = attr->mode;
>
> + if (intf != pcu->ctrl_intf)
> + return 0;
[Severity: High]
This isn't a bug introduced by this patch, but could pcu be NULL here?
If a malicious USB device provides an incorrect master interface number in the
CDC Union descriptor, ims_pcu_parse_cdc_data() might set pcu->ctrl_intf to
a different interface than the one being probed. When the sysfs core
subsequently calls ims_pcu_is_attr_visible(), usb_get_intfdata() would
return NULL, leading to a NULL pointer dereference when accessing
pcu->ctrl_intf.
[ ... ]
> @@ -1488,6 +1491,9 @@ static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
> struct ims_pcu *pcu = usb_get_intfdata(intf);
> umode_t mode = attr->mode;
>
> + if (intf != pcu->ctrl_intf)
> + return 0;
[Severity: Low]
Since ofn is a named attribute group, does returning 0 here suppress the
individual files but still leave an empty ofn directory on the data interface?
According to sysfs guidelines, should this return SYSFS_GROUP_INVISIBLE
instead to fully suppress the group directory?
I also noticed a few pre-existing issues in the surrounding code:
drivers/input/misc/ims-pcu.c:ims_pcu_init_application_mode() {
...
if (info->has_gamepad) {
error = ims_pcu_setup_gamepad(pcu);
if (error)
goto err_destroy_buttons;
}
pcu->setup_complete = true;
return 0;
}
[Severity: High]
This is a pre-existing issue, but should there be a memory barrier before
setting pcu->setup_complete = true?
On weakly ordered architectures, could the CPU reorder these stores? If an
event arrives and ims_pcu_handle_response() observes setup_complete == true
before buttons->input is fully initialized, it might lead to a NULL pointer
dereference in the interrupt handler.
drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
...
/*
* See if we are dealing with control or data interface. The cleanup
* happens when we unbind primary (control) interface.
*/
if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
return;
ims_pcu_stop_io(pcu);
...
ims_pcu_buffers_free(pcu);
kfree(pcu);
}
[Severity: High]
This isn't a bug introduced by this patch, but does this fail to release the
claimed data interface?
If the driver claims the secondary data interface (pcu->data_intf) in
ims_pcu_probe(), failing to release it via usb_driver_release_interface()
when unbinding the control interface results in an asymmetric teardown.
The data interface remains bound with its intfdata pointing to the freed
pcu structure, and reading its sysfs attributes could trigger a use-after-free.
drivers/input/misc/ims-pcu.c:ims_pcu_reset_device() {
...
dev_info(pcu->dev, "Attempting to reset device\n");
error = ims_pcu_execute_command(pcu, PCU_RESET, &reset_byte, 1);
if (error) {
...
}
[Severity: High]
This is a pre-existing issue, but shouldn't ims_pcu_reset_device() acquire
pcu->cmd_mutex before calling ims_pcu_execute_command()?
Without acquiring the lock, concurrent sysfs operations (e.g., setting
backlight brightness or updating firmware) could interleave their command
preparations. This could cause data corruption in the shared
pcu->urb_out_buf and race conditions on the pcu->cmd_done completion
structure.
drivers/input/misc/ims-pcu.c:ims_pcu_buffers_free() {
...
usb_kill_urb(pcu->urb_in);
usb_free_urb(pcu->urb_in);
usb_free_coherent(pcu->udev, pcu->max_out_size,
pcu->urb_in_buf, pcu->read_dma);
...
}
[Severity: High]
This isn't a bug introduced by this patch, but is pcu->urb_in_buf being freed
with the wrong size?
It appears the input URB DMA buffer is incorrectly freed using
pcu->max_out_size instead of the pcu->max_in_size that was used during
allocation. Depending on the DMA allocator, this could lead to memory
corruption or leaks.
--
Sashiko AI review · https://sashiko.dev/#/patchset/ahp23lj4_vXIeUBl@google.com?part=1
^ permalink raw reply
* [PATCH] Input: atlas_btns - modernize the driver
From: Dmitry Torokhov @ 2026-05-30 5:52 UTC (permalink / raw)
To: linux-input; +Cc: Rafael J. Wysocki, linux-kernel
Rework the driver to use modern style:
- Remove global state by introducing a per-device structure
- Use devm for resource management (input device allocation)
- Use dev_err_probe() for error reporting in the probe path
- Clean up unused definitions and headers.
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/atlas_btns.c | 109 ++++++++++++++++----------------
1 file changed, 55 insertions(+), 54 deletions(-)
diff --git a/drivers/input/misc/atlas_btns.c b/drivers/input/misc/atlas_btns.c
index 835ad45a9d65..5805a1a3bb05 100644
--- a/drivers/input/misc/atlas_btns.c
+++ b/drivers/input/misc/atlas_btns.c
@@ -15,17 +15,18 @@
#include <linux/types.h>
#include <linux/acpi.h>
#include <linux/platform_device.h>
-#include <linux/uaccess.h>
#define ACPI_ATLAS_NAME "Atlas ACPI"
-#define ACPI_ATLAS_CLASS "Atlas"
-static unsigned short atlas_keymap[16];
-static struct input_dev *input_dev;
+struct atlas_btns {
+ struct input_dev *input_dev;
+ unsigned short keymap[16];
+};
/* button handling code */
static acpi_status acpi_atlas_button_setup(acpi_handle region_handle,
- u32 function, void *handler_context, void **return_context)
+ u32 function, void *handler_context,
+ void **return_context)
{
*return_context =
(function != ACPI_REGION_DEACTIVATE) ? handler_context : NULL;
@@ -34,33 +35,37 @@ static acpi_status acpi_atlas_button_setup(acpi_handle region_handle,
}
static acpi_status acpi_atlas_button_handler(u32 function,
- acpi_physical_address address,
- u32 bit_width, u64 *value,
- void *handler_context, void *region_context)
+ acpi_physical_address address,
+ u32 bit_width, u64 *value,
+ void *handler_context,
+ void *region_context)
{
- acpi_status status;
+ struct atlas_btns *atlas = region_context;
if (function == ACPI_WRITE) {
int code = address & 0x0f;
int key_down = !(address & 0x10);
- input_event(input_dev, EV_MSC, MSC_SCAN, code);
- input_report_key(input_dev, atlas_keymap[code], key_down);
- input_sync(input_dev);
+ input_event(atlas->input_dev, EV_MSC, MSC_SCAN, code);
+ input_report_key(atlas->input_dev, atlas->keymap[code],
+ key_down);
+ input_sync(atlas->input_dev);
- status = AE_OK;
- } else {
- pr_warn("shrugged on unexpected function: function=%x,address=%lx,value=%x\n",
- function, (unsigned long)address, (u32)*value);
- status = AE_BAD_PARAMETER;
+ return AE_OK;
}
- return status;
+ dev_warn(atlas->input_dev->dev.parent,
+ "unexpected function: function=%x,address=%lx,value=%x\n",
+ function, (unsigned long)address, (u32)*value);
+
+ return AE_BAD_PARAMETER;
}
static int atlas_acpi_button_probe(struct platform_device *pdev)
{
struct acpi_device *device;
+ struct atlas_btns *atlas;
+ struct input_dev *input_dev;
acpi_status status;
int i;
int err;
@@ -69,65 +74,62 @@ static int atlas_acpi_button_probe(struct platform_device *pdev)
if (!device)
return -ENODEV;
- input_dev = input_allocate_device();
- if (!input_dev) {
- pr_err("unable to allocate input device\n");
+ atlas = devm_kzalloc(&pdev->dev, sizeof(*atlas), GFP_KERNEL);
+ if (!atlas)
return -ENOMEM;
- }
+
+ input_dev = devm_input_allocate_device(&pdev->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ atlas->input_dev = input_dev;
input_dev->name = "Atlas ACPI button driver";
input_dev->phys = "ASIM0000/atlas/input0";
input_dev->id.bustype = BUS_HOST;
- input_dev->keycode = atlas_keymap;
- input_dev->keycodesize = sizeof(unsigned short);
- input_dev->keycodemax = ARRAY_SIZE(atlas_keymap);
+ input_dev->keycode = atlas->keymap;
+ input_dev->keycodesize = sizeof(atlas->keymap[0]);
+ input_dev->keycodemax = ARRAY_SIZE(atlas->keymap);
input_set_capability(input_dev, EV_MSC, MSC_SCAN);
- __set_bit(EV_KEY, input_dev->evbit);
- for (i = 0; i < ARRAY_SIZE(atlas_keymap); i++) {
+ for (i = 0; i < ARRAY_SIZE(atlas->keymap); i++) {
if (i < 9) {
- atlas_keymap[i] = KEY_F1 + i;
- __set_bit(KEY_F1 + i, input_dev->keybit);
- } else
- atlas_keymap[i] = KEY_RESERVED;
+ atlas->keymap[i] = KEY_F1 + i;
+ input_set_capability(input_dev, EV_KEY, KEY_F1 + i);
+ } else {
+ atlas->keymap[i] = KEY_RESERVED;
+ }
}
err = input_register_device(input_dev);
- if (err) {
- pr_err("couldn't register input device\n");
- input_free_device(input_dev);
- return err;
- }
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
+ "couldn't register input device\n");
/* hookup button handler */
status = acpi_install_address_space_handler(device->handle,
- 0x81, &acpi_atlas_button_handler,
- &acpi_atlas_button_setup, device);
- if (ACPI_FAILURE(status)) {
- pr_err("error installing addr spc handler\n");
- input_unregister_device(input_dev);
- err = -EINVAL;
- }
+ 0x81,
+ &acpi_atlas_button_handler,
+ &acpi_atlas_button_setup,
+ atlas);
+ if (ACPI_FAILURE(status))
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "error installing addr spc handler\n");
- return err;
+ return 0;
}
static void atlas_acpi_button_remove(struct platform_device *pdev)
{
struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
- acpi_status status;
- status = acpi_remove_address_space_handler(device->handle,
- 0x81, &acpi_atlas_button_handler);
- if (ACPI_FAILURE(status))
- pr_err("error removing addr spc handler\n");
-
- input_unregister_device(input_dev);
+ acpi_remove_address_space_handler(device->handle, 0x81,
+ &acpi_atlas_button_handler);
}
static const struct acpi_device_id atlas_device_ids[] = {
- {"ASIM0000", 0},
- {"", 0},
+ { "ASIM0000" },
+ { "" }
};
MODULE_DEVICE_TABLE(acpi, atlas_device_ids);
@@ -144,4 +146,3 @@ module_platform_driver(atlas_acpi_driver);
MODULE_AUTHOR("Jaya Kumar");
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Atlas button driver");
-
--
2.54.0.823.g6e5bcc1fc9-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] Input: ims-pcu - only expose sysfs attributes on control interface
From: Dmitry Torokhov @ 2026-05-30 5:40 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel
When the driver was converted to use the driver core to instantiate device
attributes (via .dev_groups in the usb_driver structure), the attributes
started appearing on all interfaces bound to the driver. Since the ims-pcu
driver manually claims the secondary data interface during probe, the
driver core automatically creates the sysfs attributes for that interface
as well.
However, the driver only supports these attributes on the primary control
interface. Data interfaces lack the necessary descriptors and internal
state to handle these requests, and accessing them can lead to unexpected
behavior or crashes.
Fix this by updating the is_visible() callbacks for both the main and OFN
attribute groups to verify that the interface being accessed is indeed the
control interface.
Fixes: 204d18a7a0c6 ("Input: ims-pcu - use driver core to instantiate device attributes")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 13367ec66936..ddfb88b7c8f5 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1283,6 +1283,9 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
struct ims_pcu *pcu = usb_get_intfdata(intf);
umode_t mode = attr->mode;
+ if (intf != pcu->ctrl_intf)
+ return 0;
+
if (pcu->bootloader_mode) {
if (attr != &dev_attr_update_firmware_status.attr &&
attr != &dev_attr_update_firmware.attr &&
@@ -1534,6 +1537,9 @@ static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
struct ims_pcu *pcu = usb_get_intfdata(intf);
umode_t mode = attr->mode;
+ if (intf != pcu->ctrl_intf)
+ return 0;
+
/*
* PCU-B devices, both GEN_1 and GEN_2 do not have OFN sensor.
*/
--
2.54.0.823.g6e5bcc1fc9-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH 4/4] dt-bindings: input: remove obsolete matrix-keymap.txt
From: sashiko-bot @ 2026-05-30 5:23 UTC (permalink / raw)
To: Akash Sukhavasi; +Cc: dmitry.torokhov, robh, linux-input, devicetree, conor+dt
In-Reply-To: <20260529052246.4934-5-akash.sukhavasi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit message inaccurately claims that 'Eight files still reference matrix-keymap.txt', when only 6 files actually do.
--
commit eb5de2cbf16470ef87ea36fcd4695758a1c30a9f
Author: Akash Sukhavasi <akash.sukhavasi@gmail.com>
dt-bindings: input: remove obsolete matrix-keymap.txt
This commit removes the obsolete matrix-keymap.txt stub and updates all
remaining references across the input and mfd binding documentation to
point directly to the matrix-keymap.yaml schema.
> Eight files still reference matrix-keymap.txt, forcing readers
> through an extra hop to reach the .yaml. The stub has not been
> touched since June 2020. Update all references across input and mfd
> binding documentation to point directly to matrix-keymap.yaml and
> remove the stub.
[Severity: Low]
This isn't a bug, but the commit message states that eight files still
reference the text file. Looking at the patch, there are eight total
references updated, but they are spread across only six files since
tc3589x.txt contains three of those references.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529052246.4934-1-akash.sukhavasi@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 01/10] [v2] [net-next] net: dsa: b53: hide legacy gpiolib usage on non-mips
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260520183815.2510387-2-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe errors from devm_gpio_request_one() are silently ignored, failing to propagate critical errors such as -EPROBE_DEFER.
--
commit 46ca85c6cb9da93cad093c8eb342afc0ea94be33
Author: Arnd Bergmann <arnd@arndb.de>
net: dsa: b53: hide legacy gpiolib usage on non-mips
This commit moves legacy gpiolib usage inside an #ifdef block and transitions
the common parts of the driver to use modern GPIO descriptors. This allows
building the driver even when gpio_set_value is left out of the kernel.
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 2c4131ed7e30b..d838511ae6744 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
[ ... ]
> @@ -3173,12 +3172,6 @@ static int b53_switch_init(struct b53_device *dev)
> return -ENOMEM;
>
> dev->reset_gpio = b53_switch_get_reset_gpio(dev);
> - if (dev->reset_gpio >= 0) {
> - ret = devm_gpio_request_one(dev->dev, dev->reset_gpio,
> - GPIOF_OUT_INIT_HIGH, "robo_reset");
> - if (ret)
> - return ret;
> - }
>
> return 0;
> }
[Severity: High]
Does this silently ignore probe errors like -EPROBE_DEFER?
Previously, if devm_gpio_request_one failed, the error was returned to the
caller. Now, b53_switch_get_reset_gpio handles the request and returns an
ERR_PTR on failure, but that error pointer is simply stored in dev->reset_gpio
while b53_switch_init unconditionally returns 0.
Later in the code, b53_switch_reset_gpio relies on IS_ERR(dev->reset_gpio)
to gracefully return, which would silently skip the reset sequence entirely
if the GPIO controller simply isn't ready yet (-EPROBE_DEFER) or if there
was a memory allocation failure (-ENOMEM).
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=1
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-3-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying the legacy GPIO validation to check `!button->irq` breaks legacy devices that legitimately provide both a GPIO and an IRQ.
- [Medium] Conditionally removing the `gpio` field from `struct gpio_keys_button` causes build failures in drivers that assign to this field when `CONFIG_GPIOLIB_LEGACY` is disabled.
--
commit cb399daff5ceabcf54d5ea611fd309f5c0600ab4
Author: Arnd Bergmann <arnd@arndb.de>
input: gpio-keys: make legacy gpiolib optional
This commit makes the legacy gpiolib interface optional for gpio-keys and
gpio-keys-polled by adding ifdef blocks. It also modifies the gpio-keys
driver to ignore the gpio number if an IRQ is passed to keep Rohm PMIC
drivers working with and without CONFIG_GPIOLIB_LEGACY.
Link: https://lore.kernel.org/all/b3c94552-c104-42e3-be15-7e8362e8039e@gmail.com/
Link: https://lore.kernel.org/all/afJXG4_rtaj3l2Dk@google.com/
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
[ ... ]
> @@ -528,7 +528,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> */
> bdata->gpiod = NULL;
> }
> - } else if (gpio_is_valid(button->gpio)) {
> +#ifdef CONFIG_GPIOLIB_LEGACY
> + } else if (!button->irq && gpio_is_valid(button->gpio)) {
[Severity: High]
Does this change break legacy devices that legitimately provide both a valid
GPIO and a valid IRQ?
If a legacy platform provides both a valid GPIO and an IRQ, this condition
now evaluates to false.
By skipping the GPIO request entirely, the driver falls back to treating the
device as a pure IRQ button. This forces the driver to lose hardware
debouncing and rely on a simulated release timer instead of checking the
physical GPIO line.
[ ... ]
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> --- a/include/linux/gpio_keys.h
> +++ b/include/linux/gpio_keys.h
[ ... ]
> @@ -25,7 +25,9 @@ struct device;
> */
> struct gpio_keys_button {
> unsigned int code;
> +#ifdef CONFIG_GPIOLIB_LEGACY
> int gpio;
> +#endif
> int active_low;
[Severity: Medium]
Will this cause build failures for drivers that assign to this field when
CONFIG_GPIOLIB_LEGACY is disabled?
Other modern drivers dynamically create the platform device and pass legacy
GPIO numbers by directly assigning to this field:
drivers/input/misc/soc_button_array.c:soc_button_device_create() {
...
gpio_keys[n_buttons].gpio = gpio;
...
}
When CONFIG_GPIOLIB_LEGACY is disabled, this results in a compilation failure
since struct gpio_keys_button no longer has the gpio member.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=2
^ permalink raw reply
* Re: [PATCH 10/10] gpiolib: turn off legacy interface by default
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-11-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Turning off `CONFIG_GPIOLIB_LEGACY` by default will break the build for multiple architectures and platforms that still rely on the legacy GPIO interface but do not select `CONFIG_GPIOLIB_LEGACY`.
--
commit a920fbf61a1d127c3a98b41cb32f7260acd5657b
Author: Arnd Bergmann <arnd@arndb.de>
gpiolib: turn off legacy interface by default
This commit removes the default enablement of GPIOLIB_LEGACY, requiring
platforms to explicitly select it if they still use the legacy interface.
It also adds a prompt to enable it manually for compile testing.
> All users of the legacy interface now select CONFIG_GPIOLIB_LEGACY,
> so it can be turned off by default and only get built on platforms
> that still have one unconverted driver.
[Severity: Low]
Is this statement accurate? There appear to be platforms like ARM PXA
(arch/arm/mach-pxa/spitz_pm.c), ARM Orion5x, and m68k Coldfire that still
contain direct calls to legacy GPIO functions like gpio_request() but do
not select GPIOLIB_LEGACY in their Kconfig files.
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 020e51e30317a..543de15a12f63 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -4,7 +4,14 @@
> #
>
> config GPIOLIB_LEGACY
> - def_bool y
[Severity: Low]
Will removing this default cause build regressions for the unconverted
platforms mentioned above?
It seems the legacy function declarations in include/linux/gpio.h will be
hidden, leading to implicit declaration errors when compiling those
architectures.
> + bool "Legacy GPIO interfaces" if COMPILE_TEST
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=10
^ permalink raw reply
* Re: [PATCH 08/10] [v6 net-next] p54spi: convert to devicetree
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-9-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] SPI transfers are using stack-allocated buffers for `tx_buf` and `rx_buf`, violating the SPI subsystem's DMA requirements.
--
commit 8ba2b2da7516b867a52706d993b9ce2d49c99105
Author: Arnd Bergmann <arnd@arndb.de>
p54spi: convert to devicetree
The Prism54 SPI driver hardcodes GPIO numbers and expects users to
pass them as module parameters, apparently a relic from its life as a
staging driver. This works because there is only one user, the Nokia
N8x0 tablet.
Convert this to the gpio descriptor interface and DT based probing
to improve this and simplify the code at the same time.
> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index 9d66dcae54e02..d18be25450285 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
[ ... ]
> @@ -30,19 +31,6 @@
> MODULE_FIRMWARE("3826.arm");
> MODULE_FIRMWARE("3826.eeprom");
>
> -/* gpios should be handled in board files and provided via platform data,
> - * but because it's currently impossible for p54spi to have a header file
> - * in include/linux, let's use module parameters for now
> - */
> -
> -static int p54spi_gpio_power = 97;
> -module_param(p54spi_gpio_power, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_power, "gpio number for power line");
> -
> -static int p54spi_gpio_irq = 87;
> -module_param(p54spi_gpio_irq, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_irq, "gpio number for irq line");
> -
> static void p54spi_spi_read(struct p54s_priv *priv, u8 address,
> void *buf, size_t len)
> {
[Severity: High]
This isn't a bug introduced by this patch, but does this code use
stack-allocated buffers for SPI transfers?
It appears that p54spi_spi_read() and other similar functions pass stack
variables (e.g., addr in p54spi_spi_read(), rx_head in p54spi_rx(), and
val in p54spi_read32()) directly into spi_sync() as tx_buf and rx_buf.
If the underlying SPI controller uses DMA, the SPI core maps these buffers.
Using stack memory for DMA is unsafe because, with options like
CONFIG_VMAP_STACK enabled, the stack may not be physically contiguous.
Even without it, stack DMA leads to cache-line sharing and incoherency,
which can result in severe memory corruption or kernel panics.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=8
^ permalink raw reply
* Re: [PATCH 05/10] [v2] mips: select legacy gpiolib interfaces where used
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260520183815.2510387-6-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Duplicate `select GPIOLIB` in `config MIPS_ALCHEMY`.
- [Low] Unnecessary `select GPIOLIB_LEGACY` for `config MIKROTIK_RB532`.
--
commit 0726793f1a39f1a0b2ac5d24243bcbe5e8999b93
Author: Arnd Bergmann <arnd@arndb.de>
mips: select legacy gpiolib interfaces where used
This commit updates several old mips machines that have not been converted
away from the old-style gpiolib interfaces to select CONFIG_GPIOLIB_LEGACY.
This ensures the legacy code continues to work where needed but can be
omitted otherwise.
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 4364f3dba688f..b91e62d69a5db 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -206,6 +206,8 @@ config MIPS_ALCHEMY
> select CSRC_R4K
> select IRQ_MIPS_CPU
> select DMA_NONCOHERENT # Au1000,1500,1100 aren't, rest is
> + select GPIOLIB
> + select GPIOLIB_LEGACY
[Severity: Low]
Is this select GPIOLIB a duplicate? It appears select GPIOLIB is already
present further down in the MIPS_ALCHEMY block.
> select MIPS_FIXUP_BIGPHYS_ADDR if PCI
> select SYS_HAS_CPU_MIPS32_R1
> select SYS_SUPPORTS_32BIT_KERNEL
[ ... ]
> @@ -999,6 +1003,7 @@ config MIKROTIK_RB532
> select SWAP_IO_SPACE
> select BOOT_RAW
> select GPIOLIB
> + select GPIOLIB_LEGACY
[Severity: Low]
Does this select GPIOLIB_LEGACY introduce an unnecessary dependency for
MIKROTIK_RB532? A look at the code indicates that MIKROTIK_RB532 and its
associated drivers (like pata_rb532_cf and rb532_button) use the modern
descriptor-based gpiod API and do not seem to call the legacy GPIO functions.
Could this dependency be omitted so we don't compile legacy interfaces
unnecessarily?
> select MIPS_L1_CACHE_SHIFT_4
> help
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=5
^ permalink raw reply
* Re: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
From: sashiko-bot @ 2026-05-29 23:12 UTC (permalink / raw)
To: d3z-the-dev; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260529222210.1532876-1-d3z.the.dev@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Unconditional hardware reset during system resume destroys pending wake events and breaks Wake-on-Touch.
Pre-existing issues:
- [High] Using wait_event_interruptible_timeout in a system resume callback can prematurely abort device resume.
- [High] Concurrent execution of `reset_tic()` from the PM resume callback and the IRQ thread without proper locking.
--
commit e9088cb0484bc3fbc7b5dd49968293dc14d55d36
Author: d3z-the-dev <d3z.the.dev@gmail.com>
HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
This commit modifies quickspi_resume() to perform a full reset of the touch
IC during system resume. It mirrors the hibernation restore path to fix an
issue where the touch IC loses power during s2idle suspend on certain platforms.
Link: https://github.com/linux-surface/linux-surface/issues/1799
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index f669235f18839..d59278a3e5c10 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
> thc_interrupt_config(qsdev->thc_hw);
>
> thc_interrupt_enable(qsdev->thc_hw, true);
[Severity: High]
This is a pre-existing issue, but does enabling interrupts right before calling
reset_tic() introduce a race condition?
If the touch controller generates an error interrupt during the reset sequence,
the OS schedules the IRQ thread, which calls try_recover() and executes
reset_tic() again. Since both the PM thread and the IRQ thread execute
reset_tic() concurrently without a mutex:
PM thread
quickspi_resume()
reset_tic()
IRQ thread
quickspi_irq_thread_handler()
try_recover()
reset_tic()
Can this corrupt the device state and cause both threads to wait on the same
reset_ack_wq?
>
> - ret = thc_dma_configure(qsdev->thc_hw);
> + /* The TIC may lose power across system suspend, reset it to recover */
> + ret = reset_tic(qsdev);
[Severity: High]
This isn't a bug introduced by this patch, but since reset_tic() is now
used in the primary system resume path, is it safe for it to use
wait_event_interruptible_timeout() internally?
System resume callbacks are often executed by a user-space process which
might have a pending signal from the task freeze phase. If
wait_event_interruptible_timeout() aborts immediately and returns
-ERESTARTSYS, won't reset_tic() fail with -ETIMEDOUT and completely abort
the device resume?
> if (ret)
> return ret;
>
> - ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
> + ret = thc_dma_configure(qsdev->thc_hw);
> if (ret)
> return ret;
>
> - if (!device_may_wakeup(qsdev->dev))
> - return quickspi_set_power(qsdev, HIDSPI_ON);
[Severity: High]
Does unconditionally calling reset_tic() here break Wake-on-Touch for
platforms that don't lose power?
Previously, this function had a check to avoid disturbing a wake-enabled
device:
- if (!device_may_wakeup(qsdev->dev))
- return quickspi_set_power(qsdev, HIDSPI_ON);
Will a hard reset wipe the internal buffers of the touch controller before
the OS can fetch the wake-triggering touch event, resulting in permanently
lost events?
> + thc_ltr_config(qsdev->thc_hw,
> + qsdev->active_ltr_val,
> + qsdev->low_power_ltr_val);
> +
> + thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
> +
> + qsdev->state = QUICKSPI_ENABLED;
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529222210.1532876-1-d3z.the.dev@gmail.com?part=1
^ permalink raw reply
* [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
From: d3z-the-dev @ 2026-05-29 22:22 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Abhishek Tamboli, Sakari Ailus,
d3z-the-dev
On the Surface Pro 10 (Meteor Lake) the touchscreen stops working after a
suspend/resume cycle and only recovers after a reboot. The driver logs
"GET_DEVICE_INFO: recv failed: -11" on resume.
The touch IC loses power during system suspend (s2idle) on this platform,
the same way it does across hibernation. quickspi_resume() only restores
the THC port, interrupts and DMA and sends a HIDSPI_ON command, assuming
the touch IC kept its power and state. When it has actually lost power the
HIDSPI_ON command is never acknowledged and the descriptor read fails,
leaving the touchscreen dead until the module is reloaded.
quickspi_restore() already handles this for hibernation by running
reset_tic() and reconfiguring the THC SPI/LTR settings. Make
quickspi_resume() do the same: quiesce interrupts, re-select the THC port,
reconfigure the SPI input/output addresses and read/write parameters, run
reset_tic() to re-enumerate the device and restore the LTR configuration.
Tested on a Surface Pro 10 across multiple s2idle suspend/resume cycles.
Link: https://github.com/linux-surface/linux-surface/issues/1799
Signed-off-by: d3z-the-dev <d3z.the.dev@gmail.com>
---
.../intel-quickspi/pci-quickspi.c | 38 +++++++++++++++++--
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index f669235f1883..d59278a3e5c1 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
if (!qsdev)
return -ENODEV;
+ ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
+ if (ret)
+ return ret;
+
ret = thc_port_select(qsdev->thc_hw, THC_PORT_TYPE_SPI);
if (ret)
return ret;
+ thc_spi_input_output_address_config(qsdev->thc_hw,
+ qsdev->input_report_hdr_addr,
+ qsdev->input_report_bdy_addr,
+ qsdev->output_report_addr);
+
+ ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
+ qsdev->spi_read_io_mode,
+ qsdev->spi_read_opcode,
+ qsdev->spi_packet_size);
+ if (ret)
+ return ret;
+
+ ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
+ qsdev->spi_write_io_mode,
+ qsdev->spi_write_opcode,
+ qsdev->spi_packet_size,
+ qsdev->performance_limit);
+ if (ret)
+ return ret;
+
thc_interrupt_config(qsdev->thc_hw);
thc_interrupt_enable(qsdev->thc_hw, true);
- ret = thc_dma_configure(qsdev->thc_hw);
+ /* The TIC may lose power across system suspend, reset it to recover */
+ ret = reset_tic(qsdev);
if (ret)
return ret;
- ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
+ ret = thc_dma_configure(qsdev->thc_hw);
if (ret)
return ret;
- if (!device_may_wakeup(qsdev->dev))
- return quickspi_set_power(qsdev, HIDSPI_ON);
+ thc_ltr_config(qsdev->thc_hw,
+ qsdev->active_ltr_val,
+ qsdev->low_power_ltr_val);
+
+ thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
+
+ qsdev->state = QUICKSPI_ENABLED;
return 0;
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2] HID: wacom: fix NULL pointer dereference in wacom_intuos_pad()
From: Dmitry Torokhov @ 2026-05-29 21:44 UTC (permalink / raw)
To: Jinmo Yang
Cc: Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <20260523150619.615565-1-jinmo44.yang@gmail.com>
On Sun, May 24, 2026 at 12:06:19AM +0900, Jinmo Yang wrote:
> wacom_intuos_pad() dereferences wacom->pad_input without a NULL check.
> When a Wacom tablet is created via UHID with parameters that route
> through wacom_bpt_irq() -> wacom_intuos_irq() -> wacom_intuos_pad(),
> but probe did not allocate pad_input, the call to
> wacom_report_numbered_buttons() passes a NULL input_dev, causing a
> general protection fault in input_get_drvdata().
>
> Add a NULL check for pad_input at the top of wacom_intuos_pad() to
> bail out early when the pad input device was not set up.
>
> The bug was found by syzkaller on linux-next 7.1.0-rc4 (next-20260522):
>
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000052
> KASAN: null-ptr-deref in range [0x0000000000000290-0x0000000000000297]
> RIP: 0010:input_get_drvdata include/linux/input.h:390 [inline]
> RIP: 0010:wacom_report_numbered_buttons+0x37/0x210 drivers/hid/wacom_wac.c:4210
> Call Trace:
> wacom_intuos_pad drivers/hid/wacom_wac.c:643 [inline]
> wacom_intuos_irq+0x29a/0x32b0 drivers/hid/wacom_wac.c:1042
> wacom_bpt_irq drivers/hid/wacom_wac.c:3275 [inline]
> wacom_wac_irq+0x12ed/0xaef0 drivers/hid/wacom_wac.c:3545
> wacom_raw_event+0x677/0xa90 drivers/hid/wacom_sys.c:184
> __hid_input_report.constprop.0+0x39a/0x4d0 drivers/hid/hid-core.c:2161
> uhid_dev_input2 drivers/hid/uhid.c:618 [inline]
> uhid_char_write+0xa8a/0xfa0 drivers/hid/uhid.c:776
>
> Also confirmed on a Pixel 9 Pro (Android 16, kernel 6.1.124) where
> it causes an immediate kernel panic and reboot via /dev/uhid without
> requiring root privileges:
>
> KP: Oops: Fatal exception: comm:wacom_27qhdt
> Reboot reason: 0xbaba - Kernel PANIC
>
> Reproducer (unprivileged):
> open("/dev/uhid", O_RDWR)
> write(fd, UHID_CREATE2{vendor=0x056a, product=0x0020})
> write(fd, UHID_INPUT2{report_id=0x0c, size=10})
>
> Fixes: c7f0522a1ad1 ("HID: wacom: Slim down wacom_intuos_pad processing")
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> drivers/hid/wacom_wac.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index da1f0ea85..251ddda3e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -515,7 +515,6 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
> struct wacom_features *features = &wacom->features;
> unsigned char *data = wacom->data;
> struct input_dev *input = wacom->pad_input;
> - int i;
> int buttons = 0, nbuttons = features->numbered_buttons;
> int keys = 0, nkeys = 0;
> int ring1 = 0, ring2 = 0;
> @@ -523,6 +522,10 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
> bool prox = false;
> bool wrench = false, keyboard = false, mute_touch = false, menu = false,
> info = false;
> + int i;
> +
> + if (!input)
> + return 0;
>
> /* pad packets. Works as a second tool and is always in prox */
> if (!(data[0] == WACOM_REPORT_INTUOSPAD || data[0] == WACOM_REPORT_INTUOS5PAD ||
I think there are many more places in the driver where it used
wacom->pad_input without verifying that it exists.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
From: Dmitry Torokhov @ 2026-05-29 21:34 UTC (permalink / raw)
To: Jinmo Yang; +Cc: linux-input, jikos, benjamin.tissoires, stable
In-Reply-To: <20260528175945.2987781-1-jinmo44.yang@gmail.com>
On Fri, May 29, 2026 at 02:59:45AM +0900, Jinmo Yang wrote:
> wacom_wac_queue_insert() calls kfifo_skip() in a loop when the kfifo
> doesn't have enough space for the incoming report. If the kfifo is
> empty, kfifo_skip() reads stale data left in the kmalloc'd buffer
> via __kfifo_peek_n() and interprets it as a record length, advancing
> fifo->out by that garbage value. This corrupts the internal kfifo
> state, causing kfifo_unused() to return a value much larger than the
> actual buffer size, which bypasses __kfifo_in_r()'s guard:
>
> if (len + recsize > kfifo_unused(fifo))
> return 0;
>
> kfifo_copy_in() then performs an out-of-bounds memcpy, writing up to
> 3842 bytes past the 256-byte buffer.
>
> Add a !kfifo_is_empty() condition to the while loop so kfifo_skip()
> is never called on an empty fifo, and check the return value of
> kfifo_in() to reject reports that are too large for the fifo.
>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Fixes: 5e013ad20689 ("HID: wacom: Remove static WACOM_PKGLEN_MAX limit")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> Changes in v2:
> - Instead of a size check at the top, add !kfifo_is_empty() to the
> while loop condition to prevent kfifo_skip() on an empty fifo
> (Suggested by Dmitry Torokhov)
> - Check kfifo_in() return value to reject oversized reports instead
> of a separate guard
>
> drivers/hid/wacom_sys.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a32320b35..489ca68f1 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -54,7 +54,7 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> {
> bool warned = false;
>
> - while (kfifo_avail(fifo) < size) {
> + while (kfifo_avail(fifo) < size && !kfifo_is_empty(fifo)) {
> if (!warned)
> hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
> warned = true;
> @@ -62,7 +62,9 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> kfifo_skip(fifo);
> }
>
> - kfifo_in(fifo, raw_data, size);
> + if (!kfifo_in(fifo, raw_data, size))
> + hid_warn_ratelimited(hdev, "%s: report is too large (%d)\n",
> + __func__, size);
> }
>
> static void wacom_wac_queue_flush(struct hid_device *hdev,
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ 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