* [PATCH 4/6] Input: mms114 - replace udelay with usleep_range
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
The driver currently uses udelay(MMS114_I2C_DELAY) (50us) to ensure a
mandatory delay between I2C transfers in __mms114_read_reg() and
mms114_write_reg().
Both functions invoke underlying I2C core operations (i2c_transfer,
i2c_master_send) which acquire mutexes and sleep. Furthermore, the
interrupt handler mms114_interrupt() is registered as a threaded IRQ
handler. Since the entire execution path is fully sleepable,
busy-waiting with udelay() for 50us unnecessarily wastes CPU cycles.
Replace udelay() with usleep_range() to allow the CPU to enter low-power
states or execute other tasks during the delay.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index c2e006ac1196..c59aec8f2feb 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -116,7 +116,7 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
"%s: i2c transfer failed (%d)\n", __func__, error);
return error < 0 ? error : -EIO;
}
- udelay(MMS114_I2C_DELAY);
+ usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
return 0;
}
@@ -148,7 +148,7 @@ static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
"%s: i2c send failed (%d)\n", __func__, error);
return error < 0 ? error : -EIO;
}
- udelay(MMS114_I2C_DELAY);
+ usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
if (reg == MMS114_MODE_CONTROL)
data->cache_mode_control = val;
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 5/6] Input: mms114 - replace BUG() and fix alignment
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
Avoid crashing the kernel with BUG() when userspace attempts to read
the write-only mode control register. Instead, trigger a warning with
WARN_ON() and return -EINVAL to allow the kernel to recover.
Additionally, fix parameter alignment to match the open parenthesis
in several functions to conform to the kernel coding style.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index c59aec8f2feb..bf01eee0560a 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -95,8 +95,8 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
u8 buf = reg;
int error;
- if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
- BUG();
+ if (WARN_ON(reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL))
+ return -EINVAL;
/* Write register */
xfer[0].addr = client->addr;
@@ -310,8 +310,7 @@ static int mms114_get_version(struct mms114_data *data)
if (error)
return error;
- group = i2c_smbus_read_byte_data(data->client,
- MMS152_COMPAT_GROUP);
+ group = i2c_smbus_read_byte_data(data->client, MMS152_COMPAT_GROUP);
if (group < 0)
return group;
@@ -371,14 +370,14 @@ static int mms114_setup_regs(struct mms114_data *data)
if (data->contact_threshold) {
error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD,
- data->contact_threshold);
+ data->contact_threshold);
if (error < 0)
return error;
}
if (data->moving_threshold) {
error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD,
- data->moving_threshold);
+ data->moving_threshold);
if (error < 0)
return error;
}
@@ -464,9 +463,9 @@ static int mms114_parse_legacy_bindings(struct mms114_data *data)
}
device_property_read_u32(dev, "contact-threshold",
- &data->contact_threshold);
+ &data->contact_threshold);
device_property_read_u32(dev, "moving-threshold",
- &data->moving_threshold);
+ &data->moving_threshold);
if (device_property_read_bool(dev, "x-invert"))
props->invert_x = true;
@@ -519,7 +518,7 @@ static int mms114_probe(struct i2c_client *client)
return data->num_keycodes;
} else if (data->num_keycodes > MMS114_MAX_TOUCHKEYS) {
dev_warn(&client->dev,
- "Found %d linux,keycodes but max is %d, ignoring the rest\n",
+ "Found %d linux,keycodes but max is %d, ignoring the rest\n",
data->num_keycodes, MMS114_MAX_TOUCHKEYS);
data->num_keycodes = MMS114_MAX_TOUCHKEYS;
}
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 6/6] Input: mms114 - refactor chip variant handling using descriptors
From: Dmitry Torokhov @ 2026-06-16 5:09 UTC (permalink / raw)
To: linux-input; +Cc: Linus Walleij, Bryam Vargas, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
Instead of using an enum and conditional switch/if statements throughout
the driver to handle differences between chip variants (MMS114, MMS134S,
MMS136, MMS152, MMS345L), introduce a variant-specific descriptor
structure that encapsulates variant-specific properties (name, event
size, presence of configuration registers) and callbacks (such as
get_version). Define descriptors for each supported chip and associate
them with the matching entries in the OF and I2C device ID tables.
This eliminates the need for variant checks in the driver logic, making
it easier to support new chip variants in the future.
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/mms114.c | 204 ++++++++++++++++-------------
1 file changed, 112 insertions(+), 92 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index bf01eee0560a..006dded17eb8 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -52,21 +52,13 @@
#define MMS114_TYPE_TOUCHSCREEN 1
#define MMS114_TYPE_TOUCHKEY 2
-enum mms_type {
- TYPE_MMS114 = 114,
- TYPE_MMS134S = 134,
- TYPE_MMS136 = 136,
- TYPE_MMS152 = 152,
- TYPE_MMS345L = 345,
-};
-
struct mms114_data {
+ const struct mms_chip *chip;
struct i2c_client *client;
struct input_dev *input_dev;
struct regulator *core_reg;
struct regulator *io_reg;
struct touchscreen_properties props;
- enum mms_type type;
unsigned int contact_threshold;
unsigned int moving_threshold;
@@ -77,6 +69,13 @@ struct mms114_data {
u8 cache_mode_control;
};
+struct mms_chip {
+ const char *name;
+ int event_size;
+ bool has_config_regs;
+ int (*get_version)(struct mms114_data *data);
+};
+
struct mms114_touch {
u8 id:4, reserved_bit4:1, type:2, pressed:1;
u8 x_hi:4, y_hi:4;
@@ -156,6 +155,91 @@ static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
return 0;
}
+static int mms114_get_version(struct mms114_data *data)
+{
+ struct device *dev = &data->client->dev;
+ u8 buf[6];
+ int error;
+
+ error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
+ if (error)
+ return error;
+
+ dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 0x%x\n",
+ buf[0], buf[1], buf[3]);
+ return 0;
+}
+
+static int mms152_get_version(struct mms114_data *data)
+{
+ struct device *dev = &data->client->dev;
+ u8 buf[3];
+ int group;
+ int error;
+
+ error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+ if (error)
+ return error;
+
+ group = i2c_smbus_read_byte_data(data->client, MMS152_COMPAT_GROUP);
+ if (group < 0)
+ return group;
+
+ dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x, Compat group: %c\n",
+ buf[0], buf[1], buf[2], group);
+ return 0;
+}
+
+static int mms345l_get_version(struct mms114_data *data)
+{
+ struct device *dev = &data->client->dev;
+ u8 buf[3];
+ int error;
+
+ error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+ if (error)
+ return error;
+
+ dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x\n",
+ buf[0], buf[1], buf[2]);
+ return 0;
+}
+
+static const struct mms_chip mms114_descriptor = {
+ .name = "MMS114",
+ .event_size = MMS114_EVENT_SIZE,
+ .has_config_regs = true,
+ .get_version = mms114_get_version,
+};
+
+static const struct mms_chip mms134s_descriptor = {
+ .name = "MMS134S",
+ .event_size = MMS136_EVENT_SIZE,
+ .has_config_regs = true,
+ .get_version = mms114_get_version,
+};
+
+static const struct mms_chip mms136_descriptor = {
+ .name = "MMS136",
+ .event_size = MMS136_EVENT_SIZE,
+ .has_config_regs = true,
+ .get_version = mms114_get_version,
+};
+
+static const struct mms_chip mms152_descriptor = {
+ .name = "MMS152",
+ .event_size = MMS114_EVENT_SIZE,
+ .has_config_regs = false,
+ .get_version = mms152_get_version,
+};
+
+static const struct mms_chip mms345l_descriptor = {
+ .name = "MMS345L",
+ .event_size = MMS114_EVENT_SIZE,
+ .has_config_regs = false,
+ .get_version = mms345l_get_version,
+};
+
static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
{
struct i2c_client *client = data->client;
@@ -217,8 +301,8 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
struct i2c_client *client = data->client;
struct mms114_touch touch[MMS114_MAX_TOUCH];
struct mms114_touch *t;
+ int event_size = data->chip->event_size;
int packet_size;
- int event_size;
int touch_size;
int index;
int error;
@@ -233,12 +317,6 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
goto out;
}
- /* MMS136 has slightly different event size */
- if (data->type == TYPE_MMS134S || data->type == TYPE_MMS136)
- event_size = MMS136_EVENT_SIZE;
- else
- event_size = MMS114_EVENT_SIZE;
-
touch_size = packet_size / event_size;
error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
@@ -288,64 +366,17 @@ static int mms114_set_active(struct mms114_data *data, bool active)
return mms114_write_reg(data, MMS114_MODE_CONTROL, val);
}
-static int mms114_get_version(struct mms114_data *data)
-{
- struct device *dev = &data->client->dev;
- u8 buf[6];
- int group;
- int error;
-
- switch (data->type) {
- case TYPE_MMS345L:
- error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
- if (error)
- return error;
-
- dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x\n",
- buf[0], buf[1], buf[2]);
- break;
-
- case TYPE_MMS152:
- error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
- if (error)
- return error;
-
- group = i2c_smbus_read_byte_data(data->client, MMS152_COMPAT_GROUP);
- if (group < 0)
- return group;
-
- dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x, Compat group: %c\n",
- buf[0], buf[1], buf[2], group);
- break;
-
- case TYPE_MMS114:
- case TYPE_MMS134S:
- case TYPE_MMS136:
- error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
- if (error)
- return error;
-
- dev_info(dev, "TSP Rev: 0x%x, HW Rev: 0x%x, Firmware Ver: 0x%x\n",
- buf[0], buf[1], buf[3]);
- break;
- }
-
- return 0;
-}
-
static int mms114_setup_regs(struct mms114_data *data)
{
const struct touchscreen_properties *props = &data->props;
int val;
int error;
- error = mms114_get_version(data);
- if (error < 0)
+ error = data->chip->get_version(data);
+ if (error)
return error;
- /* MMS114, MMS134S and MMS136 have configuration and power on registers */
- if (data->type != TYPE_MMS114 && data->type != TYPE_MMS134S &&
- data->type != TYPE_MMS136)
+ if (!data->chip->has_config_regs)
return 0;
error = mms114_set_active(data, true);
@@ -481,7 +512,6 @@ static int mms114_probe(struct i2c_client *client)
{
struct mms114_data *data;
struct input_dev *input_dev;
- const void *match_data;
int error;
int i;
@@ -501,12 +531,10 @@ static int mms114_probe(struct i2c_client *client)
data->client = client;
data->input_dev = input_dev;
- match_data = device_get_match_data(&client->dev);
- if (!match_data)
+ data->chip = i2c_get_match_data(client);
+ if (!data->chip)
return -EINVAL;
- data->type = (enum mms_type)match_data;
-
data->num_keycodes = device_property_count_u32(&client->dev,
"linux,keycodes");
if (data->num_keycodes == -EINVAL) {
@@ -563,8 +591,7 @@ static int mms114_probe(struct i2c_client *client)
0, data->props.max_y, 0, 0);
}
- if (data->type == TYPE_MMS114 || data->type == TYPE_MMS134S ||
- data->type == TYPE_MMS136) {
+ if (data->chip->has_config_regs) {
/*
* The firmware handles movement and pressure fuzz, so
* don't duplicate that in software.
@@ -579,8 +606,8 @@ static int mms114_probe(struct i2c_client *client)
}
input_dev->name = devm_kasprintf(&client->dev, GFP_KERNEL,
- "MELFAS MMS%d Touchscreen",
- data->type);
+ "MELFAS %s Touchscreen",
+ data->chip->name);
if (!input_dev->name)
return -ENOMEM;
@@ -676,29 +703,22 @@ static int mms114_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(mms114_pm_ops, mms114_suspend, mms114_resume);
static const struct i2c_device_id mms114_id[] = {
- { .name = "mms114" },
+ { .name = "mms114", .driver_data = (kernel_ulong_t)&mms114_descriptor },
+ { .name = "mms134s", .driver_data = (kernel_ulong_t)&mms134s_descriptor },
+ { .name = "mms136", .driver_data = (kernel_ulong_t)&mms136_descriptor },
+ { .name = "mms152", .driver_data = (kernel_ulong_t)&mms152_descriptor },
+ { .name = "mms345l", .driver_data = (kernel_ulong_t)&mms345l_descriptor },
{ }
};
MODULE_DEVICE_TABLE(i2c, mms114_id);
#ifdef CONFIG_OF
static const struct of_device_id mms114_dt_match[] = {
- {
- .compatible = "melfas,mms114",
- .data = (void *)TYPE_MMS114,
- }, {
- .compatible = "melfas,mms134s",
- .data = (void *)TYPE_MMS134S,
- }, {
- .compatible = "melfas,mms136",
- .data = (void *)TYPE_MMS136,
- }, {
- .compatible = "melfas,mms152",
- .data = (void *)TYPE_MMS152,
- }, {
- .compatible = "melfas,mms345l",
- .data = (void *)TYPE_MMS345L,
- },
+ { .compatible = "melfas,mms114", .data = &mms114_descriptor },
+ { .compatible = "melfas,mms134s", .data = &mms134s_descriptor },
+ { .compatible = "melfas,mms136", .data = &mms136_descriptor },
+ { .compatible = "melfas,mms152", .data = &mms152_descriptor },
+ { .compatible = "melfas,mms345l", .data = &mms345l_descriptor },
{ }
};
MODULE_DEVICE_TABLE(of, mms114_dt_match);
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 1/4] Input: sur40 - fix input device registration ordering
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In sur40_probe(), input_register_device() was previously called early before
the V4L2 video device and vb2_queue components were fully initialized. If
userspace opened the input device immediately upon registration, sur40_open()
would trigger and start the sur40_poll() worker thread. This worker thread
invokes sur40_process_video() and accesses the uninitialized vb2_queue
structure, leading to a data race and potential system crash.
Furthermore, if V4L2 or video registration failed after input_register_device()
succeeded, the error path fell through to calling input_free_device() on a
successfully registered device instead of input_unregister_device(), corrupting
input core state.
Move input_register_device() to the very end of sur40_probe(). This ensures
the V4L2 and video queue structures are fully initialized before polling can
start, and naturally resolves the error path bug since input_free_device()
is now only called when input registration has not yet occurred.
To maintain strict LIFO (Last-In, First-Out) teardown ordering, also move
input_unregister_device() to the very beginning of sur40_disconnect(). This
guarantees that the input polling worker thread is stopped before V4L2
video components or control handlers are unregistered.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index fe63d53d56db..8639ec3ad703 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -725,21 +725,13 @@ static int sur40_probe(struct usb_interface *interface,
goto err_free_input;
}
- /* register the polled input device */
- error = input_register_device(input);
- if (error) {
- dev_err(&interface->dev,
- "Unable to register polled input device.");
- goto err_free_buffer;
- }
-
/* register the video master device */
snprintf(sur40->v4l2.name, sizeof(sur40->v4l2.name), "%s", DRIVER_LONG);
error = v4l2_device_register(sur40->dev, &sur40->v4l2);
if (error) {
dev_err(&interface->dev,
"Unable to register video master device.");
- goto err_unreg_v4l2;
+ goto err_free_buffer;
}
/* initialize the lock and subdevice */
@@ -798,6 +790,14 @@ static int sur40_probe(struct usb_interface *interface,
goto err_unreg_video;
}
+ /* register the polled input device */
+ error = input_register_device(input);
+ if (error) {
+ dev_err(&interface->dev,
+ "Unable to register polled input device.");
+ goto err_unreg_video;
+ }
+
/* we can register the device now, as it is ready */
usb_set_intfdata(interface, sur40);
dev_dbg(&interface->dev, "%s is now attached\n", DRIVER_DESC);
@@ -823,11 +823,12 @@ static void sur40_disconnect(struct usb_interface *interface)
{
struct sur40_state *sur40 = usb_get_intfdata(interface);
+ input_unregister_device(sur40->input);
+
v4l2_ctrl_handler_free(&sur40->hdl);
video_unregister_device(&sur40->vdev);
v4l2_device_unregister(&sur40->v4l2);
- input_unregister_device(sur40->input);
kfree(sur40->bulk_in_buffer);
kfree(sur40);
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 2/4] Input: sur40 - fix V4L error path cleanup
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
In sur40_probe(), if video_register_device() fails, the error path jumps to
err_unreg_video. This incorrectly attempts to unregister a video device
that was never successfully registered, and fails to free the V4L2 control
handler (v4l2_ctrl_handler_free) that was initialized immediately prior.
Fix this by introducing an err_free_ctrl label to properly free the V4L2
control handler and bypass video_unregister_device() when video device
registration fails.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 8639ec3ad703..e9089b0c3e2f 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -787,7 +787,7 @@ static int sur40_probe(struct usb_interface *interface,
if (error) {
dev_err(&interface->dev,
"Unable to register video subdevice.");
- goto err_unreg_video;
+ goto err_free_ctrl;
}
/* register the polled input device */
@@ -806,6 +806,8 @@ static int sur40_probe(struct usb_interface *interface,
err_unreg_video:
video_unregister_device(&sur40->vdev);
+err_free_ctrl:
+ v4l2_ctrl_handler_free(&sur40->hdl);
err_unreg_v4l2:
v4l2_device_unregister(&sur40->v4l2);
err_free_buffer:
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 3/4] Input: sur40 - factor out and move input device initialization
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
The input device allocation, setup, and registration in sur40_probe() is
quite verbose. Factor it out into a helper function sur40_init_input() to
improve readability.
Additionally, call this helper at the very end of sur40_probe() instead of
allocating the input device early. This ensures all video components are fully
initialized before the input device is registered (which starts polling),
and simplifies the early probe error paths since we don't have to carry and
free the input device if probe fails during early V4L2 setup.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 91 +++++++++++++++++--------------
1 file changed, 50 insertions(+), 41 deletions(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index e9089b0c3e2f..1ad68131e3a6 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -647,6 +647,53 @@ static int sur40_input_setup_events(struct input_dev *input_dev)
return 0;
}
+static int sur40_init_input(struct sur40_state *sur40)
+{
+ struct input_dev *input;
+ int error;
+
+ input = input_allocate_device();
+ if (!input)
+ return -ENOMEM;
+
+ /* Set up regular input device structure */
+ input->name = DRIVER_LONG;
+ usb_to_input_id(sur40->usbdev, &input->id);
+ usb_make_path(sur40->usbdev, sur40->phys, sizeof(sur40->phys));
+ strlcat(sur40->phys, "/input0", sizeof(sur40->phys));
+ input->phys = sur40->phys;
+ input->dev.parent = sur40->dev;
+
+ input->open = sur40_open;
+ input->close = sur40_close;
+
+ error = sur40_input_setup_events(input);
+ if (error)
+ goto err_free_input;
+
+ input_set_drvdata(input, sur40);
+ error = input_setup_polling(input, sur40_poll);
+ if (error) {
+ dev_err(sur40->dev, "failed to set up polling\n");
+ goto err_free_input;
+ }
+
+ input_set_poll_interval(input, POLL_INTERVAL);
+
+ error = input_register_device(input);
+ if (error) {
+ dev_err(sur40->dev, "Unable to register polled input device.\n");
+ goto err_free_input;
+ }
+
+ sur40->input = input;
+ return 0;
+
+err_free_input:
+ input_free_device(input);
+ return error;
+}
+
/* Check candidate USB interface. */
static int sur40_probe(struct usb_interface *interface,
const struct usb_device_id *id)
@@ -655,7 +702,6 @@ static int sur40_probe(struct usb_interface *interface,
struct sur40_state *sur40;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor *endpoint;
- struct input_dev *input;
int error;
/* Check if we really have the right interface. */
@@ -676,44 +722,13 @@ static int sur40_probe(struct usb_interface *interface,
if (!sur40)
return -ENOMEM;
- input = input_allocate_device();
- if (!input) {
- error = -ENOMEM;
- goto err_free_dev;
- }
-
/* initialize locks/lists */
INIT_LIST_HEAD(&sur40->buf_list);
spin_lock_init(&sur40->qlock);
mutex_init(&sur40->lock);
- /* Set up regular input device structure */
- input->name = DRIVER_LONG;
- usb_to_input_id(usbdev, &input->id);
- usb_make_path(usbdev, sur40->phys, sizeof(sur40->phys));
- strlcat(sur40->phys, "/input0", sizeof(sur40->phys));
- input->phys = sur40->phys;
- input->dev.parent = &interface->dev;
-
- input->open = sur40_open;
- input->close = sur40_close;
-
- error = sur40_input_setup_events(input);
- if (error)
- goto err_free_input;
-
- input_set_drvdata(input, sur40);
- error = input_setup_polling(input, sur40_poll);
- if (error) {
- dev_err(&interface->dev, "failed to set up polling");
- goto err_free_input;
- }
-
- input_set_poll_interval(input, POLL_INTERVAL);
-
sur40->usbdev = usbdev;
sur40->dev = &interface->dev;
- sur40->input = input;
/* use the bulk-in endpoint tested above */
sur40->bulk_in_size = usb_endpoint_maxp(endpoint);
@@ -722,7 +737,7 @@ static int sur40_probe(struct usb_interface *interface,
if (!sur40->bulk_in_buffer) {
dev_err(&interface->dev, "Unable to allocate input buffer.");
error = -ENOMEM;
- goto err_free_input;
+ goto err_free_dev;
}
/* register the video master device */
@@ -790,13 +805,9 @@ static int sur40_probe(struct usb_interface *interface,
goto err_free_ctrl;
}
- /* register the polled input device */
- error = input_register_device(input);
- if (error) {
- dev_err(&interface->dev,
- "Unable to register polled input device.");
+ error = sur40_init_input(sur40);
+ if (error)
goto err_unreg_video;
- }
/* we can register the device now, as it is ready */
usb_set_intfdata(interface, sur40);
@@ -812,8 +823,6 @@ static int sur40_probe(struct usb_interface *interface,
v4l2_device_unregister(&sur40->v4l2);
err_free_buffer:
kfree(sur40->bulk_in_buffer);
-err_free_input:
- input_free_device(input);
err_free_dev:
kfree(sur40);
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH 4/4] Input: sur40 - fix V4L2 video device lifetime
From: Dmitry Torokhov @ 2026-06-16 5:12 UTC (permalink / raw)
To: Hans Verkuil, linux-input; +Cc: linux-kernel, sashiko-bot, stable
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
sur40_disconnect() synchronously frees the sur40_state structure (kfree(sur40))
while userspace might still hold an open file descriptor to the V4L2 video
device node. When userspace later accesses or closes the lingering file
descriptor, the V4L2 core invokes file operations (such as vb2_fop_release)
that dereference the already freed sur40 memory, resulting in a use-after-free
vulnerability.
Fix this by implementing a V4L2 release callback (sur40_video_release) in
sur40_video_device to clean up V4L2 components and free the sur40 structure
only when the last video file descriptor is closed.
Additionally, update the sur40_probe() error path to call video_unregister_device()
and return inline if input initialization fails after video device registration
succeeded, allowing the V4L2 release callback to manage cleanup.
Also, call v4l2_device_disconnect() in sur40_disconnect() to safely dissociate
the V4L2 device from the parent USB device during unplug.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/sur40.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 1ad68131e3a6..2f0efee23d1e 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -806,8 +806,10 @@ static int sur40_probe(struct usb_interface *interface,
}
error = sur40_init_input(sur40);
- if (error)
- goto err_unreg_video;
+ if (error) {
+ video_unregister_device(&sur40->vdev);
+ return error;
+ }
/* we can register the device now, as it is ready */
usb_set_intfdata(interface, sur40);
@@ -815,8 +817,6 @@ static int sur40_probe(struct usb_interface *interface,
return 0;
-err_unreg_video:
- video_unregister_device(&sur40->vdev);
err_free_ctrl:
v4l2_ctrl_handler_free(&sur40->hdl);
err_unreg_v4l2:
@@ -835,13 +835,8 @@ static void sur40_disconnect(struct usb_interface *interface)
struct sur40_state *sur40 = usb_get_intfdata(interface);
input_unregister_device(sur40->input);
-
- v4l2_ctrl_handler_free(&sur40->hdl);
video_unregister_device(&sur40->vdev);
- v4l2_device_unregister(&sur40->v4l2);
-
- kfree(sur40->bulk_in_buffer);
- kfree(sur40);
+ v4l2_device_disconnect(&sur40->v4l2);
usb_set_intfdata(interface, NULL);
dev_dbg(&interface->dev, "%s is now disconnected\n", DRIVER_DESC);
@@ -1176,11 +1171,21 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = {
.vidioc_streamoff = vb2_ioctl_streamoff,
};
+static void sur40_video_release(struct video_device *vdev)
+{
+ struct sur40_state *sur40 = video_get_drvdata(vdev);
+
+ v4l2_device_unregister(&sur40->v4l2);
+ v4l2_ctrl_handler_free(&sur40->hdl);
+ kfree(sur40->bulk_in_buffer);
+ kfree(sur40);
+}
+
static const struct video_device sur40_video_device = {
.name = DRIVER_LONG,
.fops = &sur40_video_fops,
.ioctl_ops = &sur40_video_ioctl_ops,
- .release = video_device_release_empty,
+ .release = sur40_video_release,
.device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_TOUCH |
V4L2_CAP_READWRITE | V4L2_CAP_STREAMING,
};
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* Re: [PATCH 4/6] Input: mms114 - replace udelay with usleep_range
From: sashiko-bot @ 2026-06-16 5:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616050912.1531241-4-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Race condition in `mms114_suspend` releases touches before disabling the hardware IRQ, potentially leaving touches stuck active.
- [High] Failure in `mms114_resume()` leads to unbalanced `disable_irq()` and `regulator_disable()`, causing kernel warnings and state corruption.
- [High] Copy-paste error in `mms114_setup_regs` writes the X resolution to the Y resolution register, breaking touchscreen coordinates mapping.
- [High] Hardware data bytes are read directly into a struct containing C bitfields, leading to endian-dependent memory layout corruption.
--
commit 4b0151507f5071f8f96d0af3ccffe9ae30a17c92
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: mms114 - replace udelay with usleep_range
The driver currently uses udelay(MMS114_I2C_DELAY) (50us) to ensure a
mandatory delay between I2C transfers in __mms114_read_reg() and
mms114_write_reg().
Both functions invoke underlying I2C core operations (i2c_transfer,
i2c_master_send) which acquire mutexes and sleep. Furthermore, the
interrupt handler mms114_interrupt() is registered as a threaded IRQ
handler. Since the entire execution path is fully sleepable,
busy-waiting with udelay() for 50us unnecessarily wastes CPU cycles.
Replace udelay() with usleep_range() to allow the CPU to enter low-power
states or execute other tasks during the delay.
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index c2e006ac11960..c59aec8f2febe 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -116,7 +116,7 @@ static int __mms114_read_reg(struct mms114_data *data, u8 reg,
> "%s: i2c transfer failed (%d)\n", __func__, error);
> return error < 0 ? error : -EIO;
> }
> - udelay(MMS114_I2C_DELAY);
> + usleep_range(MMS114_I2C_DELAY, MMS114_I2C_DELAY + 50);
>
> return 0;
> }
[ ... ]
[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in the
suspend path.
drivers/input/touchscreen/mms114.c:mms114_suspend() {
...
/* Release all touch */
for (id = 0; id < MMS114_MAX_TOUCH; id++) {
input_mt_slot(input_dev, id);
input_mt_report_slot_inactive(input_dev);
}
input_mt_report_pointer_emulation(input_dev, true);
input_sync(input_dev);
guard(mutex)(&input_dev->mutex);
if (input_device_enabled(input_dev))
mms114_stop(data);
...
}
Because mms114_stop() disables the hardware IRQ after input_sync() has reported
the release events, is it possible for a hardware interrupt to fire between
the synthetic release and the actual IRQ disable?
Since the interrupt handler runs as a concurrent kernel thread and does not
share a lock with this early part of the suspend path, the touch could be
reported as active again right before the system sleeps. Would this leave the
touch stuck active indefinitely upon resume?
[Severity: High]
This isn't a bug introduced by this patch, but the resume error handling
might lead to unbalanced resource disabling.
drivers/input/touchscreen/mms114.c:mms114_resume() {
...
if (input_device_enabled(input_dev)) {
error = mms114_start(data);
if (error)
return error;
}
...
}
If mms114_start() fails (e.g. due to I2C communication failure), it disables
the regulators and returns an error without enabling the IRQ. However, the
input device remains logically open.
If userspace subsequently closes the device, or another suspend cycle occurs,
mms114_stop() will be called unconditionally:
drivers/input/touchscreen/mms114.c:mms114_stop() {
...
disable_irq(client->irq);
error = regulator_disable(data->io_reg);
...
}
Will this unconditionally call disable_irq() and regulator_disable() on
resources that are already disabled, triggering warnings and corrupting
regulator reference counts?
[Severity: High]
This is a pre-existing issue, but I spotted a potential copy-paste error
during resolution configuration.
drivers/input/touchscreen/mms114.c:mms114_setup_regs() {
...
val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
if (error < 0)
return error;
val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
...
}
Should the second register write use props->max_y instead of props->max_x?
Writing the X resolution to MMS114_Y_RESOLUTION might truncate the hardware's
internal Y resolution, breaking touch tracking and coordinate mapping on the
Y axis for screens where the dimensions differ.
[Severity: High]
This isn't a bug introduced by this patch, but there might be an endianness
problem with the touch data structure.
drivers/input/touchscreen/mms114.c:
...
struct mms114_touch {
u8 id:4, reserved_bit4:1, type:2, pressed:1;
u8 x_hi:4, y_hi:4;
u8 x_lo;
u8 y_lo;
u8 width;
u8 strength;
u8 reserved[2];
} __packed;
...
This struct is used to map raw byte stream data from the I2C hardware. Since
the memory layout of C bitfields is compiler and endian-dependent, will these
bitfields be reversed on big-endian architectures?
This could result in incorrect parsing of touch coordinates, IDs, and event
types.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 6/6] Input: mms114 - refactor chip variant handling using descriptors
From: sashiko-bot @ 2026-06-16 5:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616050912.1531241-6-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The input device name exposed to userspace has changed for the MMS134S and MMS345L variants, which risks breaking userspace configurations.
--
commit 6433e094b88447042b6bbde3852dd64f8e938510
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: mms114 - refactor chip variant handling using descriptors
This commit replaces enum-based chip variant checks with a descriptor
structure. It encapsulates variant-specific properties and callbacks into
per-chip descriptors mapped directly via device ID tables.
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index bf01eee0560a1..006dded17eb80 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
[ ... ]
> @@ -579,8 +606,8 @@ static int mms114_probe(struct i2c_client *client)
> }
>
> - input_dev->name = devm_kasprintf(&client->dev, GFP_KERNEL,
> - "MELFAS MMS%d Touchscreen",
> - data->type);
> + input_dev->name = devm_kasprintf(&client->dev, GFP_KERNEL,
> + "MELFAS %s Touchscreen",
> + data->chip->name);
[Severity: Medium]
Could this string format change break userspace configurations that rely on
the exact input device name?
Previously, using the integer type, TYPE_MMS134S (134) resulted in the name
"MELFAS MMS134 Touchscreen", and TYPE_MMS345L (345) resulted in
"MELFAS MMS345 Touchscreen".
By switching to the descriptor name string, the names now include the trailing
characters ("MELFAS MMS134S Touchscreen" and "MELFAS MMS345L Touchscreen").
Might this disrupt existing hwdb rules or udev scripts matching on the old
UAPI-visible names?
> if (!input_dev->name)
> return -ENOMEM;
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH 3/6] Input: mms114 - use appropriate register argument types
From: sashiko-bot @ 2026-06-16 5:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616050912.1531241-3-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Stack-allocated memory is passed to I2C transfer functions, which is unsafe as I2C controllers may use DMA.
- [High] The driver doesn't handle a touch ID of 0 for touchscreen events, which causes a slot underflow and corrupts the previously active slot's state.
- [High] Incorrect initialization of Y-axis resolution register.
--
commit e6fe847438e30cfef36a69ac4ec8110ef02638d0
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: mms114 - use appropriate register argument types
This commit updates the register arguments in the mms114 touch controller
driver to use 8-bit unsigned integers, matching the hardware specifications.
It also changes the value buffer pointer to void* to prevent explicit casting
when reading from the device.
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index db23b51f4630d..c2e006ac11960 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -87,12 +87,12 @@ struct mms114_touch {
> u8 reserved[2];
> } __packed;
>
> -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
> - unsigned int len, u8 *val)
> +static int __mms114_read_reg(struct mms114_data *data, u8 reg,
> + unsigned int len, void *val)
> {
> struct i2c_client *client = data->client;
> struct i2c_msg xfer[2];
> - u8 buf = reg & 0xff;
> + u8 buf = reg;
[Severity: High]
This is a pre-existing issue, but I noticed we are using a stack-allocated
variable here.
Could this cause issues with DMA? The buf variable is allocated on the stack
and its address is assigned to xfer[0].buf for the outgoing I2C transfer.
Does this violate the DMA API prohibition on using stack-allocated memory
for I2C controllers that use DMA?
[ ... ]
> -static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
> - unsigned int val)
> +static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
> {
> struct i2c_client *client = data->client;
> u8 buf[2];
> int error;
>
> - buf[0] = reg & 0xff;
> - buf[1] = val & 0xff;
> + buf[0] = reg;
> + buf[1] = val;
>
> error = i2c_master_send(client, buf, 2);
[Severity: High]
This isn't a bug introduced by this patch, but this function also uses a stack
array.
Is it safe to pass the local stack array buf[2] directly to
i2c_master_send()? Does this risk stack corruption if the I2C controller
uses DMA?
[ ... ]
> @@ -242,9 +241,8 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
>
> touch_size = packet_size / event_size;
>
> - error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
> - (u8 *)touch);
> - if (error < 0)
> + error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
> + if (error)
> goto out;
[Severity: High]
This isn't a bug introduced by this patch, but the touch array is allocated
on the stack in this interrupt handler.
Since the touch array is passed as the read buffer for incoming I2C
touch events, could this lead to stack corruption if the I2C controller
performs DMA?
While reviewing this file, I also noticed a couple of other pre-existing
issues outside of this diff:
[Severity: High]
This is a pre-existing issue, but I noticed there isn't a check for touch ID
0 in mms114_process_mt():
id = touch->id - 1;
...
input_mt_slot(input_dev, id);
If the hardware sends a touch event with touch->id == 0, id would underflow
to -1. When passed to input_mt_slot(), would this cause the input core to
ignore the slot update and overwrite the state of the previously active slot?
[Severity: High]
This is a pre-existing issue, but it looks like we might be writing the
X-resolution into the Y-resolution register in mms114_setup_regs():
val = props->max_x & 0xff;
error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
Does val here inadvertently use props->max_x instead of props->max_y
when writing to MMS114_Y_RESOLUTION?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136
From: sashiko-bot @ 2026-06-16 5:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing validation for `touch->id == 0` causes multi-touch slot state corruption.
- [High] The devres allocation order in probe registers the input device's unregistration action before the regulators and IRQ are allocated, leading to a Use-After-Free.
- [Medium] Hardware I2C packet layout is mapped using compiler-dependent C bitfields.
--
commit d12fd2dedb0344983eaa73c88bc06f0ac0c59d09
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: mms114 - fix touch indexing for MMS134S and MMS136
This commit fixes an issue where the driver iterates through touch events
using standard array indexing, assuming an 8-byte size. It explicitly
calculates the byte offset for each event based on the device's specific
event size (6 bytes for MMS134S and MMS136).
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 4d748a13408d1..53ad35d61d476 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -217,7 +217,9 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
> struct mms114_data *data = dev_id;
> struct i2c_client *client = data->client;
> struct mms114_touch touch[MMS114_MAX_TOUCH];
> + struct mms114_touch *t;
[ ... ]
> for (index = 0; index < touch_size; index++) {
> - switch (touch[index].type) {
> + t = (struct mms114_touch *)((u8 *)touch + index * event_size);
> +
[Severity: Medium]
This is a pre-existing issue, but I noticed the hardware I2C packet layout
is mapped using C bitfields:
struct mms114_touch {
u8 id:4, reserved_bit4:1, type:2, pressed:1;
u8 x_hi:4, y_hi:4;
...
Because the layout of bitfields within a byte is compiler and endianness
dependent, will this read garbage for the id, type, and pressed fields
if the driver is compiled for a Big Endian system?
> + switch (t->type) {
> case MMS114_TYPE_TOUCHSCREEN:
> - mms114_process_mt(data, touch + index);
> + mms114_process_mt(data, t);
[Severity: High]
This isn't a bug introduced by this patch, but does the multi-touch slot
processing handle an empty or invalid touch ID safely?
Looking at mms114_process_mt(), the slot index is calculated without a
check for zero:
mms114_process_mt() {
...
id = touch->id - 1;
...
input_mt_slot(input_dev, id);
If the device reports a touch event with touch->id == 0, id will underflow
to -1. Since the input core ignores negative slot values and leaves the
active slot unchanged, would subsequent coordinate reporting functions act
on this unchanged slot and silently corrupt the multi-touch tracking?
> break;
>
> case MMS114_TYPE_TOUCHKEY:
[ ... ]
[Severity: High]
This is a pre-existing issue, but I have a question about the device cleanup
sequence during unbind.
In mms114_probe(), devm_input_allocate_device() is called before the
regulators and IRQ are allocated:
mms114_probe() {
...
input_dev = devm_input_allocate_device(&client->dev);
...
data->core_reg = devm_regulator_get(&client->dev, "avdd");
}
Because devres executes cleanup actions in reverse order (LIFO), during
unbind the IRQ and regulators are freed before input_unregister_device()
is called.
If the input device is held open by userspace during unbind, does
input_unregister_device() trigger mms114_input_close() and then
mms114_stop()?
mms114_stop() {
...
disable_irq(client->irq);
error = regulator_disable(data->io_reg);
...
}
Could this attempt to disable a freed IRQ and access freed regulators,
causing a use-after-free?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 3/4] Input: sur40 - factor out and move input device initialization
From: sashiko-bot @ 2026-06-16 5:23 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616051235.1549517-3-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Deadlock in `sur40_stop_streaming` due to waiting for buffers that will never be processed if input polling is not active.
--
commit 7253dffc6b93516e6c96ea7370f123676e435b94
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: sur40 - factor out and move input device initialization
The input device allocation, setup, and registration in sur40_probe() is
quite verbose. Factor it out into a helper function sur40_init_input() to
improve readability.
Additionally, call this helper at the very end of sur40_probe() instead of
allocating the input device early. This ensures all video components are fully
initialized before the input device is registered (which starts polling),
and simplifies the early probe error paths since we don't have to carry and
free the input device if probe fails during early V4L2 setup.
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index e9089b0c3e2f4..1ad68131e3a64 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -647,6 +647,53 @@ static int sur40_input_setup_events(struct input_dev *input_dev)
> return 0;
> }
>
> +static int sur40_init_input(struct sur40_state *sur40)
> +{
> + struct input_dev *input;
> + int error;
> +
[ ... ]
> + error = sur40_input_setup_events(input);
> + if (error)
> + goto err_free_input;
> +
> + input_set_drvdata(input, sur40);
> + error = input_setup_polling(input, sur40_poll);
[Severity: High]
This isn't a bug introduced by this patch, but setting up polling here
highlights an existing deadlock in the driver.
If a user opens the video device, queues buffers, starts streaming, and then
stops streaming while the input device remains closed, the sur40_poll()
callback will never run.
This causes a deadlock when stopping the video stream:
drivers/input/touchscreen/sur40.c:sur40_stop_streaming() {
...
vb2_wait_for_all_buffers(vq);
sur40->sequence = -1;
/* Release all active buffers */
return_all_buffers(sur40, VB2_BUF_STATE_ERROR);
}
The vb2_wait_for_all_buffers() function blocks indefinitely waiting for buffers
to be returned, but those buffers are only processed inside sur40_poll(). Since
the input device is closed, the polling thread isn't active, causing the
process to hang.
Should the driver avoid waiting for buffers if polling isn't active, or
should the polling lifecycle be decoupled from the input device open state?
> + if (error) {
> + dev_err(sur40->dev, "failed to set up polling\n");
> + goto err_free_input;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616051235.1549517-1-dmitry.torokhov@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH 4/4] Input: sur40 - fix V4L2 video device lifetime
From: sashiko-bot @ 2026-06-16 5:26 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616051235.1549517-4-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Critical] Calling `v4l2_device_disconnect()` after `video_unregister_device()` causes a deterministic use-after-free on device unplug.
- [Critical] Double-free and use-after-free of the `sur40` structure and V4L2 components if `video_register_device()` fails during probe.
Pre-existing issues:
- [High] `sur40_stop_streaming()` hangs forever waiting for buffers to be returned by a stopped polling thread.
- [High] Lingering input device file descriptor allows Use-After-Free access to sur40->phys via EVIOCGPHYS.
--
commit c1c922112f8294f71a1841f179b4f4b61138851b
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: sur40 - fix V4L2 video device lifetime
This commit introduces a V4L2 release callback to clean up components and
free the sur40 structure only when the last video file descriptor is closed.
It updates the error path and disconnect sequence to handle this new
lifecycle.
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 1ad68131e3a64..2f0efee23d1ec 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -806,8 +806,10 @@ static int sur40_probe(struct usb_interface *interface,
> }
>
> error = sur40_init_input(sur40);
[Severity: High]
This is a pre-existing issue, but could there be a use-after-free when
userspace queries EVIOCGPHYS?
Inside sur40_init_input(), the input device's phys path is pointed directly
to the sur40->phys array:
sur40_init_input()
input->phys = sur40->phys;
If the video device is closed, the new release callback frees the sur40
structure. If a userspace process still holds the input device open, a
subsequent EVIOCGPHYS ioctl will read from the freed sur40 memory.
> - if (error)
> - goto err_unreg_video;
> + if (error) {
> + video_unregister_device(&sur40->vdev);
> + return error;
> + }
>
> /* we can register the device now, as it is ready */
> usb_set_intfdata(interface, sur40);
> @@ -815,8 +817,6 @@ static int sur40_probe(struct usb_interface *interface,
>
> return 0;
>
> -err_unreg_video:
> - video_unregister_device(&sur40->vdev);
> err_free_ctrl:
> v4l2_ctrl_handler_free(&sur40->hdl);
> err_unreg_v4l2:
[Severity: Critical]
Does this error path cause a double-free if video_register_device() fails
earlier in sur40_probe()?
If video_register_device() fails internally during device_register(), the
V4L2 core handles cleanup by dropping the reference count, which
synchronously invokes the new sur40_video_release() callback. That
callback frees the control handler, buffer, and the sur40 structure.
Jumping to err_free_ctrl on failure will then call v4l2_ctrl_handler_free()
and kfree() again on the already-freed pointers.
> @@ -835,13 +835,8 @@ static void sur40_disconnect(struct usb_interface *interface)
> struct sur40_state *sur40 = usb_get_intfdata(interface);
>
> input_unregister_device(sur40->input);
[Severity: High]
This is a pre-existing issue, but does this sequence cause a deadlock when
sur40_stop_streaming() is eventually called?
Calling input_unregister_device() synchronously stops the input polling
thread (sur40_poll). Later, when the video device is closed and
sur40_stop_streaming() is triggered, it invokes vb2_wait_for_all_buffers().
This waits indefinitely for buffers that can only be returned by the
now-dead polling thread.
> -
> - v4l2_ctrl_handler_free(&sur40->hdl);
> video_unregister_device(&sur40->vdev);
> - v4l2_device_unregister(&sur40->v4l2);
> -
> - kfree(sur40->bulk_in_buffer);
> - kfree(sur40);
> + v4l2_device_disconnect(&sur40->v4l2);
[Severity: Critical]
Will this dereference the newly freed sur40 memory?
If there are no open file descriptors, video_unregister_device() drops the
final reference to the video device. This synchronously triggers the
new sur40_video_release() callback, which executes kfree(sur40).
Immediately after, v4l2_device_disconnect(&sur40->v4l2) is called, which
would access the freed memory. Should the order of these two calls be
swapped?
>
> usb_set_intfdata(interface, NULL);
> dev_dbg(&interface->dev, "%s is now disconnected\n", DRIVER_DESC);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616051235.1549517-1-dmitry.torokhov@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 1/4] Input: sur40 - fix input device registration ordering
From: sashiko-bot @ 2026-06-16 5:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616051235.1549517-1-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] A malicious USB device can cause the input polling thread to hang indefinitely in an infinite loop.
- [High] Device unplug while streaming causes a deadlock in `sur40_stop_streaming()`.
--
commit e70bc2bfbadc0d9c5dd284e6e5e9d8bace3e06fd
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: sur40 - fix input device registration ordering
This patch changes the device registration ordering in sur40_probe() to
ensure V4L2 components are initialized before the input device is registered,
preventing a race condition. It also updates sur40_disconnect() to
unregister the input device first, stopping the polling worker thread
before cleaning up V4L2 structures.
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index fe63d53d56db0..8639ec3ad7038 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -823,11 +823,12 @@ static void sur40_disconnect(struct usb_interface *interface)
> {
> struct sur40_state *sur40 = usb_get_intfdata(interface);
>
> + input_unregister_device(sur40->input);
> +
> v4l2_ctrl_handler_free(&sur40->hdl);
> video_unregister_device(&sur40->vdev);
[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing deadlock
when the device is unplugged while streaming.
If the device is physically disconnected, sur40_disconnect() calls
input_unregister_device() which synchronously stops the polling worker thread.
When the application then closes the video file descriptor,
sur40_stop_streaming() is invoked:
drivers/input/touchscreen/sur40.c:sur40_stop_streaming() {
...
vb2_wait_for_all_buffers(vq);
sur40->sequence = -1;
/* Release all active buffers */
return_all_buffers(sur40, VB2_BUF_STATE_ERROR);
...
}
Because vb2_wait_for_all_buffers() blocks waiting for the now-dead polling
worker to return queued buffers, and the manual buffer release
return_all_buffers() is executed after the wait, will this code deadlock the
system call?
> v4l2_device_unregister(&sur40->v4l2);
>
> - input_unregister_device(sur40->input);
> kfree(sur40->bulk_in_buffer);
[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing Denial of
Service vulnerability where a malicious USB device can cause the input
polling thread to hang indefinitely.
In sur40_poll(), the driver reads bulk messages and calculates packet_blobs:
drivers/input/touchscreen/sur40.c:sur40_poll() {
...
packet_blobs = result / sizeof(struct sur40_blob);
// ...
for (i = 0; i < packet_blobs; i++) {
need_blobs--;
...
}
If a malicious USB device sends exactly 16 bytes (the size of struct
sur40_header) with count > 0, result will be 0, leading to packet_blobs = 0.
The inner loop is skipped, leaving need_blobs unchanged.
Because need_blobs never decrements, could the outer do { ... } while
(need_blobs > 0) loop spin indefinitely, hanging the worker thread and
blocking cancel_delayed_work_sync() during module removal and device
cleanup?
> kfree(sur40);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616051235.1549517-1-dmitry.torokhov@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 5/6] Input: mms114 - replace BUG() and fix alignment
From: sashiko-bot @ 2026-06-16 5:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616050912.1531241-5-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Race condition in `mms114_suspend` leading to stuck touch state.
--
commit a5a14734d553a190d315dfdcd845e9a868df4005
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: mms114 - replace BUG() and fix alignment
This commit avoids crashing the kernel by replacing a BUG() call with WARN_ON()
when userspace reads a write-only register. It also corrects parameter
alignment in several function calls to match kernel coding style.
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index c59aec8f2febe..bf01eee0560a1 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential race
condition in mms114_suspend() that could leave the touchscreen stuck in a
pressed state:
drivers/input/touchscreen/mms114.c:mms114_suspend() {
...
for (id = 0; id < MMS114_MAX_TOUCH; id++) {
input_mt_slot(input_dev, id);
input_mt_report_slot_inactive(input_dev);
}
input_mt_report_pointer_emulation(input_dev, true);
input_sync(input_dev);
guard(mutex)(&input_dev->mutex);
if (input_device_enabled(input_dev))
mms114_stop(data);
...
}
Can mms114_interrupt() execute concurrently after input_sync() but before
mms114_stop() disables the IRQ?
If a user touches the screen during this window, wouldn't the interrupt
handler report a new active touch state right after these synthetic release
events?
Because mms114_stop() then disables the IRQ, would this freeze the touch in a
pressed state throughout suspend?
When resuming, releasing the screen wouldn't generate a release interrupt,
which might leave the touchscreen stuck until reboot or until that exact
slot is touched again.
Could mms114_stop() be called before synthesizing the release events to
prevent this race?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616050912.1531241-1-dmitry.torokhov@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH 2/4] Input: sur40 - fix V4L error path cleanup
From: sashiko-bot @ 2026-06-16 5:28 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260616051235.1549517-2-dmitry.torokhov@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Calling `vb2_wait_for_all_buffers` in `sur40_stop_streaming` causes a system deadlock.
--
commit 1d8742f82651a7f0160c799526e70838d3c9fa29
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Input: sur40 - fix V4L error path cleanup
In sur40_probe(), if video_register_device() fails, the error path jumps to
err_unreg_video. This incorrectly attempts to unregister a video device
that was never successfully registered, and fails to free the V4L2 control
handler (v4l2_ctrl_handler_free) that was initialized immediately prior.
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 8639ec3ad7038..e9089b0c3e2f4 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
[Severity: High]
This is a pre-existing issue, but can the current implementation of
sur40_stop_streaming() cause a system deadlock?
Looking at drivers/input/touchscreen/sur40.c:sur40_stop_streaming():
vb2_wait_for_all_buffers(vq);
sur40->sequence = -1;
/* Release all active buffers */
return_all_buffers(sur40, VB2_BUF_STATE_ERROR);
The driver unconditionally calls vb2_wait_for_all_buffers() before forcibly
returning the active buffers. This blocks infinitely until all queued buffers
are returned via vb2_buffer_done().
The driver expects the poller thread (sur40_poll) to process and return these
buffers. Is it possible for the poller thread to be stopped or not running
when this is called?
For instance, if userspace opens only /dev/videoX without ever opening
/dev/input/eventX, the input-bound poller never starts.
Additionally, if the device is abruptly unplugged while streaming is active,
sur40_disconnect() calls input_unregister_device(), which synchronously stops
the poller. When the video file descriptor is subsequently closed,
sur40_stop_streaming() is triggered.
In both of these scenarios, won't vb2_wait_for_all_buffers() wait forever for
buffers to be returned by a thread that is not running? Should the driver
synchronously cancel transfers and immediately return all queued buffers with
VB2_BUF_STATE_ERROR instead of waiting?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616051235.1549517-1-dmitry.torokhov@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-06-16 6:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <ajDPtOyr8GJYaVYQ@google.com>
вт, 16 черв. 2026 р. о 07:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
>
> Hi Svyatoslav,
>
> On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Add support for multimedia top button row of ASUS Transformer's Mobile
> > Dock keyboard. Driver is made that function keys (F1-F12) are used by
> > default which suits average Linux use better and with pressing
> > ScreenLock + AltGr function keys layout is switched to multimedia keys.
> > Since this only modifies codes sent by asus-ec-keys it doesn't affect
> > normal keyboards at all.
>
> I think using input handler to intercept ScreenLock + AltGr is quite
> awkward. I think this also passes the original key events (unless you
> make it a filter not a regular handler).
>
> I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
> to activate the special mode on this one. So given the fact that you
> already mange the data stream when you split it into "serio" ports,
> maybe just intercept this key combo right there and create the input
> device and signal input events right there?
>
Though it seems awkward at a first glance, media keys are integrated
with a standard keyboard in a detachable dock. It is highly unlikely
that media keys will be used with a different keyboard then the one
that is integrated with dock. Additionally, the ScreenLock key has a
code specific to this driver and is not in general use, so even if any
standard keyboard has AltGr but none has ScreenLock specific to this
driver except the dock itself. Handler is also set as observer so it
should not interfere with work of other input devices.
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH v5 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: Svyatoslav Ryhel @ 2026-06-16 6:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, devicetree, linux-kernel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij
In-Reply-To: <ajDEsU8oZWT7KB9d@google.com>
вт, 16 черв. 2026 р. о 07:16 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
>
> Hi Svyatoslav,
>
> On Mon, Jun 15, 2026 at 09:19:27AM +0300, Svyatoslav Ryhel wrote:
> > чт, 28 трав. 2026 р. о 08:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > >
> > > вт, 12 трав. 2026 р. о 13:24 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > > >
> > > > From: Linus Walleij <linusw@kernel.org>
> > > >
> > > > The ISA1200 is a haptic feedback unit from Imagis Technology using two
> > > > motors for haptic feedback in mobile phones. Used in many mobile devices
> > > > c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
> > > > GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
> > > >
> > > > The exact datasheet for the ISA1200 is not available; all data was modeled
> > > > based on available downstream kernel sources for various devices and
> > > > fragments of information scattered across the internet.
> > > >
> > > > Tested-by: Linus Walleij <linusw@kernel.org> # GT-I9070 Janice
> > > > Signed-off-by: Linus Walleij <linusw@kernel.org>
> > > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > ---
> > > > drivers/input/misc/Kconfig | 12 +
> > > > drivers/input/misc/Makefile | 1 +
> > > > drivers/input/misc/isa1200.c | 524 +++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 537 insertions(+)
> > > > create mode 100644 drivers/input/misc/isa1200.c
> > > >
> > >
> > > Hello Dmitry! Do I need to make any further adjustments to this driver?
> >
> > Hello Dmitry! Do I need to make any further adjustments to this
> > driver? This driver is hanging in LKML for some time already without
> > responds from input maintainer. It is still relevant and I would like
> > it to move forward.
>
> There were valid sashiko comments on the patch regarding resetting
> "level" to 0 and also potential racing conditions, as well as suggestion
> to check number of gpios specified in the device tree.
>
> Please see if the following works for you:
>
> diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
> index ff82252a08e1..c61adc4b605c 100644
> --- a/drivers/input/misc/isa1200.c
> +++ b/drivers/input/misc/isa1200.c
> @@ -131,6 +131,7 @@ struct isa1200 {
> struct work_struct play_work;
> struct isa1200_config config;
>
> + bool suspended;
> bool active;
> int level;
> };
> @@ -247,17 +248,21 @@ static void isa1200_stop(struct isa1200 *isa)
> isa->supplies);
>
> isa->active = false;
> - isa->level = 0;
> }
>
> static void isa1200_play_work(struct work_struct *work)
> {
> struct isa1200 *isa = container_of(work, struct isa1200, play_work);
> -
> - if (isa->level)
> - isa1200_start(isa);
> - else
> - isa1200_stop(isa);
> + struct input_dev *input = isa->input;
> +
> + scoped_guard(mutex_try, &input->mutex) {
> + if (!isa->suspended) {
> + if (isa->level)
> + isa1200_start(isa);
> + else
> + isa1200_stop(isa);
> + }
> + }
> }
>
> static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
> @@ -280,7 +285,8 @@ static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
>
> if (isa->level != level) {
> isa->level = level;
> - schedule_work(&isa->play_work);
> + if (!READ_ONCE(isa->suspended))
> + schedule_work(&isa->play_work);
> }
>
> return 0;
> @@ -292,6 +298,7 @@ static void isa1200_vibrator_close(struct input_dev *input)
>
> cancel_work_sync(&isa->play_work);
> isa1200_stop(isa);
> + isa->level = 0;
> }
>
> static int isa1200_of_probe(struct i2c_client *client)
> @@ -331,6 +338,9 @@ static int isa1200_of_probe(struct i2c_client *client)
> return dev_err_probe(dev, PTR_ERR(isa->enable_gpios),
> "failed to get enable gpios\n");
>
> + if (isa->enable_gpios && isa->enable_gpios->ndescs > ISA1200_EN_PINS_MAX)
> + return dev_err_probe(dev, -EINVAL, "too many enable gpios\n");
> +
> ldo_node = device_get_named_child_node(dev, "ldo");
> if (!ldo_node)
> return dev_err_probe(dev, -ENODEV,
> @@ -479,9 +489,9 @@ static int isa1200_suspend(struct device *dev)
> guard(mutex)(&isa->input->mutex);
>
> if (input_device_enabled(isa->input)) {
> + WRITE_ONCE(isa->suspended, true);
> cancel_work_sync(&isa->play_work);
> - if (isa->level)
> - isa1200_stop(isa);
> + isa1200_stop(isa);
> }
>
> return 0;
> @@ -493,9 +503,11 @@ static int isa1200_resume(struct device *dev)
>
> guard(mutex)(&isa->input->mutex);
>
> - if (input_device_enabled(isa->input))
> + if (input_device_enabled(isa->input)) {
> + WRITE_ONCE(isa->suspended, false);
> if (isa->level)
> - isa1200_start(isa);
> + schedule_work(&isa->play_work);
> + }
>
> return 0;
> }
>
> --
> Dmitry
I have tested your code on my P895 and it works perfectly fine. Should
I resend with these changes or you can integrate them while picking
patchset?
Thank you for your suggestions and efforts!
Best regards,
Svyatoslav R.
^ permalink raw reply
* Re: [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136
From: Bryam Vargas @ 2026-06-16 7:05 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <20260616050912.1531241-1-dmitry.torokhov@gmail.com>
Hi Dmitry,
The indexing fix looks correct -- deriving the byte offset from event_size
instead of leaning on sizeof(struct mms114_touch) is the right call, and the
cast is fine since the struct is __packed (no alignment issue at the 6-byte
stride, and the consumers only touch bytes 0..5).
Two things that might be worth folding into the series:
1) Fixes: tag. The 6-byte event path for MMS136 -- and therefore this
stride-vs-index mismatch -- predates ab108678195f. It was introduced in
53fefdd1d3a3 ("Input: mms114 - support MMS136")
which added MMS136_EVENT_SIZE and the "packet_size / MMS136_EVENT_SIZE"
branch while the loop already indexed with the 8-byte struct stride;
ab108678195f ("support MMS134S") only extended that branch to MMS134S.
So MMS136 hardware has mis-parsed multi-touch since v5.13. Tagging
Fixes: 53fefdd1d3a3 ("Input: mms114 - support MMS136")
(in addition to, or instead of, ab108678195f) lets stable pick it up for
the MMS136 range as well.
2) Unbounded packet_size. Since 1/6 already rewrites this handler: packet_size
comes straight from the device's PACKET_SIZE register (a single byte, so
1..255 after the "<= 0" guard) and is then used unclamped both as the read
length
__mms114_read_reg(data, MMS114_INFORMATION, packet_size, (u8 *)touch);
into the 80-byte touch[MMS114_MAX_TOUCH] stack buffer, and as the divisor
for touch_size -- which is never bounded by MMS114_MAX_TOUCH. A device that
reports packet_size > 80 overflows the stack buffer on the read, and even
with the indexing fix the loop still walks past it (touch_size > 10). A
one-liner before the division closes both:
if (packet_size <= 0)
goto out;
+ packet_size = min_t(int, packet_size, sizeof(touch));
This one is pre-existing -- the unbounded read goes back to the original
driver -- so it is really a separate patch in the series rather than part
of the indexing fix, but it seemed worth raising while the code is in
flight.
Thanks,
Bryam
^ permalink raw reply
* Re: [PATCH 5/6] Input: mms114 - replace BUG() and fix alignment
From: Bryam Vargas @ 2026-06-16 7:21 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <20260616050912.1531241-5-dmitry.torokhov@gmail.com>
Hi Dmitry,
Replacing BUG() with WARN_ON() + return is the right call, and the rest of
the patch is fine.
One nit on the changelog though: this guard isn't reachable from userspace.
reg and len are always driver-internal constants -- there is no sysfs/debugfs/
ioctl that lets userspace pick a register, every __mms114_read_reg() call site
passes a fixed reg >= MMS114_INFORMATION (0x10), and the single MMS114_MODE_CONTROL
read is short-circuited by the cache in mms114_read_reg() before it ever reaches
__mms114_read_reg(). So the condition only fires if a *future in-kernel caller*
requests a read that spans the write-only MODE_CONTROL register -- a driver-
internal invariant, not a userspace-triggered one.
Worth tightening the wording, since that distinction is exactly what justifies
WARN_ON() here: a genuinely userspace-reachable WARN_ON() would itself be a
problem (log spam / panic_on_warn). Maybe something like:
"Avoid taking the machine down with BUG() if a caller ever requests a read
spanning the write-only MODE_CONTROL register; warn and return -EINVAL so
the driver can recover."
The code is good either way.
Thanks,
Bryam
^ permalink raw reply
* Re: [PATCH 6/6] Input: mms114 - refactor chip variant handling using descriptors
From: Bryam Vargas @ 2026-06-16 7:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <20260616050912.1531241-6-dmitry.torokhov@gmail.com>
Hi Dmitry,
The descriptor refactor looks good -- it preserves the per-variant behaviour
(event size, config-reg gating, get_version, fuzz handling) for all five chips.
One user-visible side effect worth a line in the changelog though: switching
input_dev->name from "MELFAS MMS%d" (the enum value) to "MELFAS %s" (chip->name)
changes the reported name for the two variants whose part number carries a
non-numeric suffix:
MMS134S: "MELFAS MMS134 Touchscreen" -> "MELFAS MMS134S Touchscreen"
MMS345L: "MELFAS MMS345 Touchscreen" -> "MELFAS MMS345L Touchscreen"
(MMS114/136/152 are byte-identical.) The old "%d" on the enum silently dropped
the S/L. That string is userspace-visible via EVIOCGNAME, /sys/class/input/*/name
and /proc/bus/input/devices, so any udev/hwdb/libinput rule keyed on the old name
would stop matching. The in-tree boards affected are ste-ux500-samsung-kyle
(melfas,mms134s) and msm8916-samsung-a5u-eur / msm8916-samsung-e5 (melfas,mms345l).
The new names are more correct, so this is just worth a heads-up in the commit
message rather than a code change -- in case an out-of-tree quirk keys on the old
truncated strings.
Thanks,
Bryam
^ permalink raw reply
* [PATCH v4 1/3] HID: wacom: Fix Use-After-Free in wacom_intuos_pad
From: Lee Jones @ 2026-06-16 9:26 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, Dmitry Torokhov, linux-input, linux-kernel
wacom_intuos_pad() accesses wacom->shared->touch_input locklessly
inside the interrupt handler context. If the Touch sibling device
is disconnected, wacom_remove_shared_data() clears 'touch_input'
outside any lock, creating a Time-of-Check to Time-of-Use (TOCTOU)
race condition where a preempted reader in interrupt context
dereferences the freed pointer, leading to a Use-After-Free.
Resolve this by introducing RCU protection for the touch_input
pointer:
- Annotate 'touch_input' in wacom_shared struct with __rcu
- Wrap all lockless readers in wacom_wac.c with rcu_read_lock() and
rcu_dereference() using a unified wacom_report_touch_mute()
helper
- Update writers in wacom_sys.c using rcu_assign_pointer()
- Call synchronize_rcu() in wacom_remove_shared_data() to ensure
all active RCU readers have finished before the input device is
freed
Also wrap wacom_set_shared_values() and touch/pen assignments in
wacom_add_shared_data() inside the wacom_udev_list_lock to serialize
concurrent probe assignments, and verify that 'shared->touch == hdev'
before setting touch_input to prevent concurrent sibling probe state
desynchronization.
Finally, advertise the SW_MUTE_DEVICE capability on Touch input
devices prior to registration in wacom_setup_touch_input_capabilities()
to prevent invalid post-registration capability modifications.
Fixes: 961794a00eab ("Input: wacom - add reporting of SW_MUTE_DEVICE events")
Signed-off-by: Lee Jones <lee@kernel.org>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
v3 -> v4: Dmitry's review [redundant check and guard()]
drivers/hid/wacom_sys.c | 41 ++++++++++++++-----------
drivers/hid/wacom_wac.c | 66 +++++++++++++++++++++--------------------
drivers/hid/wacom_wac.h | 2 +-
3 files changed, 58 insertions(+), 51 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2220168bf116..86895f13dbae 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -877,10 +877,16 @@ static void wacom_remove_shared_data(void *res)
data = container_of(wacom_wac->shared, struct wacom_hdev_data,
shared);
- if (wacom_wac->shared->touch == wacom->hdev)
- wacom_wac->shared->touch = NULL;
- else if (wacom_wac->shared->pen == wacom->hdev)
- wacom_wac->shared->pen = NULL;
+ scoped_guard(mutex, &wacom_udev_list_lock) {
+ if (wacom_wac->shared->touch == wacom->hdev) {
+ wacom_wac->shared->touch = NULL;
+ rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
+ } else if (wacom_wac->shared->pen == wacom->hdev) {
+ wacom_wac->shared->pen = NULL;
+ }
+ }
+
+ synchronize_rcu();
kref_put(&data->kref, wacom_release_shared_data);
wacom_wac->shared = NULL;
@@ -909,6 +915,11 @@ static int wacom_add_shared_data(struct hid_device *hdev)
list_add_tail(&data->list, &wacom_udev_list);
}
+ if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
+ data->shared.touch = hdev;
+ else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
+ data->shared.pen = hdev;
+
mutex_unlock(&wacom_udev_list_lock);
wacom_wac->shared = &data->shared;
@@ -917,11 +928,6 @@ static int wacom_add_shared_data(struct hid_device *hdev)
if (retval)
return retval;
- if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
- wacom_wac->shared->touch = hdev;
- else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
- wacom_wac->shared->pen = hdev;
-
return retval;
}
@@ -2345,9 +2351,15 @@ static void wacom_release_resources(struct wacom *wacom)
static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
{
+ struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+
+ guard(mutex)(&wacom_udev_list_lock);
+
if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
- wacom_wac->shared->type = wacom_wac->features.type;
- wacom_wac->shared->touch_input = wacom_wac->touch_input;
+ if (wacom_wac->shared->touch == wacom->hdev) {
+ wacom_wac->shared->type = wacom_wac->features.type;
+ rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
+ }
}
if (wacom_wac->has_mute_touch_switch) {
@@ -2360,13 +2372,6 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
if (wacom_wac->is_soft_touch_switch)
wacom_wac->shared->is_touch_on = true;
}
-
- if (wacom_wac->shared->has_mute_touch_switch &&
- wacom_wac->shared->touch_input) {
- set_bit(EV_SW, wacom_wac->shared->touch_input->evbit);
- input_set_capability(wacom_wac->shared->touch_input, EV_SW,
- SW_MUTE_DEVICE);
- }
}
static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index da1f0ea85625..23eaa81cd827 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -510,6 +510,18 @@ static void wacom_intuos_schedule_prox_event(struct wacom_wac *wacom_wac)
}
}
+static void wacom_report_touch_mute(struct wacom_wac *wacom_wac, bool mute)
+{
+ struct input_dev *touch_input;
+
+ guard(rcu)();
+ touch_input = rcu_dereference(wacom_wac->shared->touch_input);
+ if (touch_input) {
+ input_report_switch(touch_input, SW_MUTE_DEVICE, mute);
+ input_sync(touch_input);
+ }
+}
+
static int wacom_intuos_pad(struct wacom_wac *wacom)
{
struct wacom_features *features = &wacom->features;
@@ -650,12 +662,8 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
input_report_key(input, KEY_CONTROLPANEL, menu);
input_report_key(input, KEY_INFO, info);
- if (wacom->shared && wacom->shared->touch_input) {
- input_report_switch(wacom->shared->touch_input,
- SW_MUTE_DEVICE,
- !wacom->shared->is_touch_on);
- input_sync(wacom->shared->touch_input);
- }
+ if (wacom->shared)
+ wacom_report_touch_mute(wacom, !wacom->shared->is_touch_on);
input_report_abs(input, ABS_RX, strip1);
input_report_abs(input, ABS_RY, strip2);
@@ -2153,7 +2161,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
*/
if ((equivalent_usage == WACOM_HID_WD_MUTE_DEVICE) ||
(equivalent_usage == WACOM_HID_WD_TOUCHONOFF)) {
- if (wacom_wac->shared->touch_input) {
+ if (wacom_wac->shared) {
bool *is_touch_on = &wacom_wac->shared->is_touch_on;
if (equivalent_usage == WACOM_HID_WD_MUTE_DEVICE && value)
@@ -2161,9 +2169,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
else if (equivalent_usage == WACOM_HID_WD_TOUCHONOFF)
*is_touch_on = value;
- input_report_switch(wacom_wac->shared->touch_input,
- SW_MUTE_DEVICE, !(*is_touch_on));
- input_sync(wacom_wac->shared->touch_input);
+ wacom_report_touch_mute(wacom_wac, !(*is_touch_on));
}
return;
}
@@ -3381,11 +3387,8 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
if ((wacom->shared->type == INTUOSHT ||
wacom->shared->type == INTUOSHT2) &&
- wacom->shared->touch_input &&
wacom->shared->touch_max) {
- input_report_switch(wacom->shared->touch_input,
- SW_MUTE_DEVICE, data[5] & 0x40);
- input_sync(wacom->shared->touch_input);
+ wacom_report_touch_mute(wacom, data[5] & 0x40);
}
pid = get_unaligned_be16(&data[6]);
@@ -3420,11 +3423,8 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
if ((features->type == INTUOSHT ||
features->type == INTUOSHT2) &&
- wacom_wac->shared->touch_input &&
features->touch_max) {
- input_report_switch(wacom_wac->shared->touch_input,
- SW_MUTE_DEVICE, data[8] & 0x40);
- input_sync(wacom_wac->shared->touch_input);
+ wacom_report_touch_mute(wacom_wac, data[8] & 0x40);
}
if (data[9] & 0x02) { /* wireless module is attached */
@@ -3951,11 +3951,22 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
struct wacom_wac *wacom_wac)
{
+ struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+ struct hid_device *hdev = wacom->hdev;
struct wacom_features *features = &wacom_wac->features;
if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
return -ENODEV;
+ if (features->type != TABLETPC &&
+ features->type != TABLETPC2FG &&
+ features->type != MTSCREEN &&
+ features->type != MTTPC &&
+ features->type != MTTPC_B) {
+ input_dev->evbit[0] |= BIT_MASK(EV_SW);
+ __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
+ }
+
if (features->device_type & WACOM_DEVICETYPE_DIRECT)
__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
else
@@ -3992,22 +4003,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
switch (features->type) {
case INTUOSP2_BT:
case INTUOSP2S_BT:
- input_dev->evbit[0] |= BIT_MASK(EV_SW);
- __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
-
- if (wacom_wac->shared->touch->product == 0x361) {
+ if (hdev->product == 0x361) {
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, 12440, 4, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
0, 8640, 4, 0);
- }
- else if (wacom_wac->shared->touch->product == 0x360) {
+ } else if (hdev->product == 0x360) {
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, 8960, 4, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
0, 5920, 4, 0);
- }
- else if (wacom_wac->shared->touch->product == 0x393) {
+ } else if (hdev->product == 0x393) {
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, 6400, 4, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
@@ -4037,10 +4043,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
fallthrough;
case WACOM_27QHDT:
- if (wacom_wac->shared->touch->product == 0x32C ||
- wacom_wac->shared->touch->product == 0xF6) {
- input_dev->evbit[0] |= BIT_MASK(EV_SW);
- __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
+ if (hdev->product == 0x32C ||
+ hdev->product == 0xF6) {
wacom_wac->has_mute_touch_switch = true;
wacom_wac->is_soft_touch_switch = true;
}
@@ -4059,8 +4063,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
case INTUOSHT:
case INTUOSHT2:
- input_dev->evbit[0] |= BIT_MASK(EV_SW);
- __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
fallthrough;
case BAMBOO_PT:
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 126bec6e5c0c..a8bbba4a6f37 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -285,7 +285,7 @@ struct wacom_shared {
/* for wireless device to access USB interfaces */
unsigned touch_max;
int type;
- struct input_dev *touch_input;
+ struct input_dev __rcu *touch_input;
struct hid_device *pen;
struct hid_device *touch;
bool has_mute_touch_switch;
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH v4 2/3] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
From: Lee Jones @ 2026-06-16 9:26 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20260616092658.1714548-1-lee@kernel.org>
wacom_bamboo_pad_pen_event() accesses wacom->shared->pen locklessly
relative to wacom_remove_shared_data() which nullifies it. This
can lead to a Use-After-Free if the sibling device is removed while
events are being processed.
Resolve this by introducing RCU protection for pen and touch
pointers:
- Annotate 'pen' and 'touch' in wacom_shared struct with __rcu.
- Wrap lockless readers in wacom_bamboo_pad_pen_event() with
rcu_read_lock() and rcu_dereference().
- Update writers in wacom_sys.c using rcu_assign_pointer().
- Use rcu_dereference_protected for comparisons under
wacom_udev_list_lock.
- Also use rcu_access_pointer in wacom_mode_change_work() to avoid
warnings (while lockless access there remains a pre-existing issue).
Fixes: 8c97a765467c ("HID: wacom: add full support of the Wacom Bamboo PAD")
Signed-off-by: Lee Jones <lee@kernel.org>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
v3 -> v4: Dmitry's review [guard()]
drivers/hid/wacom_sys.c | 36 +++++++++++++++++++++++++-----------
drivers/hid/wacom_wac.c | 13 ++++++-------
drivers/hid/wacom_wac.h | 4 ++--
3 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 86895f13dbae..3990d8d0b40c 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -878,11 +878,18 @@ static void wacom_remove_shared_data(void *res)
shared);
scoped_guard(mutex, &wacom_udev_list_lock) {
- if (wacom_wac->shared->touch == wacom->hdev) {
- wacom_wac->shared->touch = NULL;
+ struct hid_device *touch =
+ rcu_dereference_protected(wacom_wac->shared->touch,
+ lockdep_is_held(&wacom_udev_list_lock));
+ struct hid_device *pen =
+ rcu_dereference_protected(wacom_wac->shared->pen,
+ lockdep_is_held(&wacom_udev_list_lock));
+
+ if (touch == wacom->hdev) {
+ rcu_assign_pointer(wacom_wac->shared->touch, NULL);
rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
- } else if (wacom_wac->shared->pen == wacom->hdev) {
- wacom_wac->shared->pen = NULL;
+ } else if (pen == wacom->hdev) {
+ rcu_assign_pointer(wacom_wac->shared->pen, NULL);
}
}
@@ -916,9 +923,9 @@ static int wacom_add_shared_data(struct hid_device *hdev)
}
if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
- data->shared.touch = hdev;
+ rcu_assign_pointer(data->shared.touch, hdev);
else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
- data->shared.pen = hdev;
+ rcu_assign_pointer(data->shared.pen, hdev);
mutex_unlock(&wacom_udev_list_lock);
@@ -2356,7 +2363,11 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
guard(mutex)(&wacom_udev_list_lock);
if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
- if (wacom_wac->shared->touch == wacom->hdev) {
+ struct hid_device *touch =
+ rcu_dereference_protected(wacom_wac->shared->touch,
+ lockdep_is_held(&wacom_udev_list_lock));
+
+ if (touch == wacom->hdev) {
wacom_wac->shared->type = wacom_wac->features.type;
rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
}
@@ -2795,16 +2806,19 @@ static void wacom_mode_change_work(struct work_struct *work)
bool is_direct = wacom->wacom_wac.is_direct_mode;
int error = 0;
- if (shared->pen) {
- wacom1 = hid_get_drvdata(shared->pen);
+ struct hid_device *pen = rcu_access_pointer(shared->pen);
+ struct hid_device *touch = rcu_access_pointer(shared->touch);
+
+ if (pen) {
+ wacom1 = hid_get_drvdata(pen);
wacom_release_resources(wacom1);
hid_hw_stop(wacom1->hdev);
wacom1->wacom_wac.has_mode_change = true;
wacom1->wacom_wac.is_direct_mode = is_direct;
}
- if (shared->touch) {
- wacom2 = hid_get_drvdata(shared->touch);
+ if (touch) {
+ wacom2 = hid_get_drvdata(touch);
wacom_release_resources(wacom2);
hid_hw_stop(wacom2->hdev);
wacom2->wacom_wac.has_mode_change = true;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 23eaa81cd827..92d2f2f3f552 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3292,6 +3292,7 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, size_t len)
static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
unsigned char *data)
{
+ struct hid_device *pen;
unsigned char prefix;
/*
@@ -3304,13 +3305,11 @@ static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
prefix = data[0];
data[0] = WACOM_REPORT_BPAD_PEN;
- /*
- * actually reroute the event.
- * No need to check if wacom->shared->pen is valid, hid_input_report()
- * will check for us.
- */
- hid_input_report(wacom->shared->pen, HID_INPUT_REPORT, data,
- WACOM_PKGLEN_PENABLED, 1);
+ guard(rcu)();
+ pen = rcu_dereference(wacom->shared->pen);
+ if (pen)
+ hid_input_report(pen, HID_INPUT_REPORT, data,
+ WACOM_PKGLEN_PENABLED, 1);
data[0] = prefix;
}
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index a8bbba4a6f37..170d6adbe02a 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -286,8 +286,8 @@ struct wacom_shared {
unsigned touch_max;
int type;
struct input_dev __rcu *touch_input;
- struct hid_device *pen;
- struct hid_device *touch;
+ struct hid_device __rcu *pen;
+ struct hid_device __rcu *touch;
bool has_mute_touch_switch;
bool is_touch_on;
};
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* [PATCH v4 3/3] HID: wacom: Redesign shared sibling data lifecycle
From: Lee Jones @ 2026-06-16 9:26 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20260616092658.1714548-1-lee@kernel.org>
The Wacom driver coordinates state between sibling interfaces of
the same physical device using a shared structure 'wacom_shared'
inside 'wacom_hdev_data'. The driver kept a volatile representative
pointer 'data->dev' pointing to a sibling 'hid_device' for physical
path comparisons during sibling matching.
This pointer management is fragile. When the representative device
is disconnected, wacom_remove_shared_data() failed to clear/update
'data->dev', leading to a Use-After-Free vulnerability when
subsequent sibling probes dereference the dangling 'data->dev'
pointer.
Resolve this issue by redesigning the sibling data lifecycle:
- Eliminate the volatile 'data->dev' representative pointer
completely
- Redesign 'wacom_hdev_data' to store stable static copies of the
required attributes upon first allocation: 'phys' path string,
'vendor', 'product' IDs and the sibling's 'device_type'
- Use these static attributes for stable sibling matching in
wacom_are_sibling() and wacom_get_hdev_data()
This ensures sibling matching remains safe and stable even if
individual siblings are dynamically added or removed.
To secure the lifecycle against concurrent probe/disconnect races:
- Switch kref_put() to kref_put_mutex() in
wacom_remove_shared_data() to serialize refcount drops with list
traversal and lookup
- Modify wacom_release_shared_data() to assume the list lock is
already held
Also, do not accumulate the 'device_type' capability flag during
subsequent sibling probes. Keeping only the first probed sibling's
device_type exactly preserves the original sibling matching behavior
without introducing side effects.
Fixes: 4492efffffeb ("Input: wacom - share pen info with touch of the same ID")
Signed-off-by: Lee Jones <lee@kernel.org>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
v3 -> v4: No change
drivers/hid/wacom_sys.c | 70 +++++++++++++++++++++++++++--------------
1 file changed, 46 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 3990d8d0b40c..9fb9dde7dd73 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -759,27 +759,47 @@ static void wacom_retrieve_hid_descriptor(struct hid_device *hdev,
struct wacom_hdev_data {
struct list_head list;
struct kref kref;
- struct hid_device *dev;
+ char phys[64];
+ __u32 vendor;
+ __u32 product;
+ __u32 device_type;
struct wacom_shared shared;
};
+static bool wacom_compare_device_paths(struct hid_device *hdev_a,
+ const char *phys_b, char separator)
+{
+ const char *p1 = strrchr(hdev_a->phys, separator);
+ const char *p2 = strrchr(phys_b, separator);
+ int n1, n2;
+
+ if (!p1 || !p2)
+ return false;
+
+ n1 = p1 - hdev_a->phys;
+ n2 = p2 - phys_b;
+
+ if (n1 != n2 || n1 <= 0 || n2 <= 0)
+ return false;
+
+ return !strncmp(hdev_a->phys, phys_b, n1);
+}
+
static LIST_HEAD(wacom_udev_list);
static DEFINE_MUTEX(wacom_udev_list_lock);
static bool wacom_are_sibling(struct hid_device *hdev,
- struct hid_device *sibling)
+ struct wacom_hdev_data *data)
{
struct wacom *wacom = hid_get_drvdata(hdev);
struct wacom_features *features = &wacom->wacom_wac.features;
- struct wacom *sibling_wacom = hid_get_drvdata(sibling);
- struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
__u32 oVid = features->oVid ? features->oVid : hdev->vendor;
__u32 oPid = features->oPid ? features->oPid : hdev->product;
/* The defined oVid/oPid must match that of the sibling */
- if (features->oVid != HID_ANY_ID && sibling->vendor != oVid)
+ if (features->oVid != HID_ANY_ID && data->vendor != oVid)
return false;
- if (features->oPid != HID_ANY_ID && sibling->product != oPid)
+ if (features->oPid != HID_ANY_ID && data->product != oPid)
return false;
/*
@@ -787,11 +807,11 @@ static bool wacom_are_sibling(struct hid_device *hdev,
* device path, while those with different VID/PID must share
* the same physical parent device path.
*/
- if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
- if (!hid_compare_device_paths(hdev, sibling, '/'))
+ if (hdev->vendor == data->vendor && hdev->product == data->product) {
+ if (!wacom_compare_device_paths(hdev, data->phys, '/'))
return false;
} else {
- if (!hid_compare_device_paths(hdev, sibling, '.'))
+ if (!wacom_compare_device_paths(hdev, data->phys, '.'))
return false;
}
@@ -804,7 +824,7 @@ static bool wacom_are_sibling(struct hid_device *hdev,
* devices.
*/
if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
- !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+ !(data->device_type & WACOM_DEVICETYPE_DIRECT))
return false;
/*
@@ -812,17 +832,17 @@ static bool wacom_are_sibling(struct hid_device *hdev,
* devices.
*/
if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
- (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+ (data->device_type & WACOM_DEVICETYPE_DIRECT))
return false;
/* Pen devices may only be siblings of touch devices */
if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
- !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+ !(data->device_type & WACOM_DEVICETYPE_TOUCH))
return false;
/* Touch devices may only be siblings of pen devices */
if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
- !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
+ !(data->device_type & WACOM_DEVICETYPE_PEN))
return false;
/*
@@ -838,7 +858,7 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
/* Try to find an already-probed interface from the same device */
list_for_each_entry(data, &wacom_udev_list, list) {
- if (hid_compare_device_paths(hdev, data->dev, '/')) {
+ if (wacom_compare_device_paths(hdev, data->phys, '/')) {
kref_get(&data->kref);
return data;
}
@@ -846,7 +866,7 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
/* Fallback to finding devices that appear to be "siblings" */
list_for_each_entry(data, &wacom_udev_list, list) {
- if (wacom_are_sibling(hdev, data->dev)) {
+ if (wacom_are_sibling(hdev, data)) {
kref_get(&data->kref);
return data;
}
@@ -860,18 +880,15 @@ static void wacom_release_shared_data(struct kref *kref)
struct wacom_hdev_data *data =
container_of(kref, struct wacom_hdev_data, kref);
- mutex_lock(&wacom_udev_list_lock);
list_del(&data->list);
- mutex_unlock(&wacom_udev_list_lock);
-
kfree(data);
}
static void wacom_remove_shared_data(void *res)
{
- struct wacom *wacom = res;
+ struct wacom *res_wacom = res;
struct wacom_hdev_data *data;
- struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+ struct wacom_wac *wacom_wac = &res_wacom->wacom_wac;
if (wacom_wac->shared) {
data = container_of(wacom_wac->shared, struct wacom_hdev_data,
@@ -885,17 +902,19 @@ static void wacom_remove_shared_data(void *res)
rcu_dereference_protected(wacom_wac->shared->pen,
lockdep_is_held(&wacom_udev_list_lock));
- if (touch == wacom->hdev) {
+ if (touch == res_wacom->hdev) {
rcu_assign_pointer(wacom_wac->shared->touch, NULL);
rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
- } else if (pen == wacom->hdev) {
+ } else if (pen == res_wacom->hdev) {
rcu_assign_pointer(wacom_wac->shared->pen, NULL);
}
}
synchronize_rcu();
- kref_put(&data->kref, wacom_release_shared_data);
+ if (kref_put_mutex(&data->kref, wacom_release_shared_data, &wacom_udev_list_lock))
+ mutex_unlock(&wacom_udev_list_lock);
+
wacom_wac->shared = NULL;
}
}
@@ -918,7 +937,10 @@ static int wacom_add_shared_data(struct hid_device *hdev)
}
kref_init(&data->kref);
- data->dev = hdev;
+ strscpy(data->phys, hdev->phys, sizeof(data->phys));
+ data->vendor = hdev->vendor;
+ data->product = hdev->product;
+ data->device_type = wacom_wac->features.device_type;
list_add_tail(&data->list, &wacom_udev_list);
}
--
2.54.0.1136.gdb2ca164c4-goog
^ permalink raw reply related
* Re: [PATCH v4 2/3] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
From: sashiko-bot @ 2026-06-16 9:43 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260616092658.1714548-2-lee@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless access to `shared->pen` and `shared->touch` in `wacom_mode_change_work()` can lead to Use-After-Free.
--
commit e951961530e94aa8b795598b1d198c32fdbac428
Author: Lee Jones <lee@kernel.org>
HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
Resolves a use-after-free when events are processed while a sibling device
is removed by introducing RCU protection for the shared pen and touch
pointers.
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 86895f13dbaea..3990d8d0b40c0 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2795,16 +2806,19 @@ static void wacom_mode_change_work(struct work_struct *work)
> bool is_direct = wacom->wacom_wac.is_direct_mode;
> int error = 0;
>
> - if (shared->pen) {
> - wacom1 = hid_get_drvdata(shared->pen);
> + struct hid_device *pen = rcu_access_pointer(shared->pen);
> + struct hid_device *touch = rcu_access_pointer(shared->touch);
> +
> + if (pen) {
> + wacom1 = hid_get_drvdata(pen);
> wacom_release_resources(wacom1);
> hid_hw_stop(wacom1->hdev);
> wacom1->wacom_wac.has_mode_change = true;
> wacom1->wacom_wac.is_direct_mode = is_direct;
> }
[Severity: High]
This is a pre-existing issue, but can this lockless access to the sibling
device pointers lead to a use-after-free if the sibling is concurrently
disconnected?
In wacom_mode_change_work(), rcu_access_pointer() reads the pointer outside
of an RCU read-side critical section. Since the pointer is then dereferenced
to fetch wacom1 via hid_get_drvdata(), what happens if wacom_remove_shared_data()
runs concurrently on the sibling?
The removal path would clear the shared pointer and call synchronize_rcu().
Because wacom_mode_change_work() is not holding the RCU read lock,
synchronize_rcu() will not wait for the work item to complete. This could
allow wacom1's driver data to be freed while wacom_mode_change_work() is
still using it to call wacom_release_resources(wacom1) and hid_hw_stop().
Since wacom_release_resources() and hid_hw_stop() can sleep, simply wrapping
this block in rcu_read_lock() wouldn't be sufficient. Is there a plan to
use reference counting or SRCU here to safely access the sibling device?
> - if (shared->touch) {
> - wacom2 = hid_get_drvdata(shared->touch);
> + if (touch) {
> + wacom2 = hid_get_drvdata(touch);
> wacom_release_resources(wacom2);
> hid_hw_stop(wacom2->hdev);
> wacom2->wacom_wac.has_mode_change = true;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616092658.1714548-1-lee@kernel.org?part=2
^ 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