[PATCH] Input: raydium_i2c_ts - propagate device info query errors from raydium_i2c_initialize()

[PATCH] Input: raydium_i2c_ts - propagate device info query errors from raydium_i2c_initialize()

[Muhammad Bilal]

raydium_i2c_initialize() calls raydium_i2c_query_ts_info() (or
raydium_i2c_query_ts_bootloader_info() in bootloader mode) but never
checks or returns its result. raydium_i2c_probe() treats a zero
return from raydium_i2c_initialize() as full success, then
unconditionally allocates ts->report_data with size ts->pkg_size and
registers the IRQ handler.

If raydium_i2c_query_ts_info() fails on every one of its own retries
(e.g. because the device is still completing power-up when probe
runs), ts->pkg_size and ts->report_size are left at their
devm_kzalloc()'d default of 0, and ts->report_data is never
meaningfully allocated: devm_kmalloc(dev, 0, GFP_KERNEL) returns
ZERO_SIZE_PTR, a non-NULL sentinel (include/linux/slab.h) that must
never be dereferenced, which passes the existing
'if (!ts->report_data)' check. RAYDIUM_TS_MAIN is also 0, the same
value ts->boot_mode has from devm_kzalloc() before
raydium_i2c_check_fw_status() ever confirms a mode, so a
spurious/garbage first read that raydium_i2c_check_fw_status()
does not recognize as either ack byte leaves boot_mode looking like
an already-confirmed RAYDIUM_TS_MAIN without it ever being set.

With both conditions met, probe() completes 'successfully' with a
live IRQ, ts->boot_mode == RAYDIUM_TS_MAIN, and ts->report_data
pointing at a zero-size allocation. The next touch fires
raydium_i2c_irq(), which passes the boot_mode check and dereferences
ts->report_data at offset ts->report_size, both derived from that
never-actually-confirmed state, producing a NULL/invalid pointer
dereference in interrupt context:

  BUG: kernel NULL pointer dereference
  RIP: raydium_i2c_irq+0x5b/0x1d0 [raydium_i2c_ts]

On kernels with CONFIG_PANIC_ON_OOPS=y (e.g. linux-hardened) this is
a full system panic on the first touch; on a default kernel it only
kills the IRQ thread, but the touchscreen stops working and a
subsequent reboot can hang cleaning up the recursive fault.

Have raydium_i2c_initialize() return the result of the info query in
both the bootloader and main-firmware branches, so a failure there
aborts raydium_i2c_probe() before report_data or the IRQ are ever
set up, instead of leaving them in this half-initialized state.

Reported-by: Herlangga Maulani <w1zardsec@proton.me>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=221777
Fixes: 48a2b783483b ("Input: add Raydium I2C touchscreen driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/input/touchscreen/raydium_i2c_ts.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
index 0256055abcef..a0b23772f4c0 100644
--- a/drivers/input/touchscreen/raydium_i2c_ts.c
+++ b/drivers/input/touchscreen/raydium_i2c_ts.c
@@ -426,9 +426,9 @@ static int raydium_i2c_initialize(struct raydium_data *ts)
 		ts->boot_mode = RAYDIUM_TS_BLDR;
 
 	if (ts->boot_mode == RAYDIUM_TS_BLDR)
-		raydium_i2c_query_ts_bootloader_info(ts);
+		error = raydium_i2c_query_ts_bootloader_info(ts);
 	else
-		raydium_i2c_query_ts_info(ts);
+		error = raydium_i2c_query_ts_info(ts);
 
 	return error;
 }
-- 
2.55.0

Re: [PATCH] Input: raydium_i2c_ts - propagate device info query errors from raydium_i2c_initialize()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Propagating the error from `raydium_i2c_query_ts_bootloader_info()` makes non-fatal HWID read failures fatal, permanently breaking firmware recovery.

Pre-existing issues:
- [Critical] Firmware update from bootloader mode dereferences ZERO_SIZE_PTR due to un-updated buffer size.
- [High] The driver blindly trusts hardware-provided `pkg_size` and `tp_info_size` without validation, leading to integer underflow, out-of-bounds reads, and division-by-zero panics.
- [High] The target patch's fix is bypassed because `ts->boot_mode` defaults to `RAYDIUM_TS_MAIN` (0), breaking the initialization retry loop and allowing ZERO_SIZE_PTR dereference if the I2C bus returns all zeros.
- [High] The `calibrate_store` sysfs function performs a multi-step hardware command sequence without disabling interrupts, allowing the touchscreen IRQ handler to interleave its own I2C transactions.
- [High] The driver extensively passes stack-allocated memory to I2C transfer functions, violating DMA mapping rules and risking kernel panics.
--

--- Patch [1]: [PATCH] Input: raydium_i2c_ts - propagate device info query errors from raydium_i2c_initialize() ---
commit cc4ec8b45e33c4b58e61efd967f923405f1be4a5
Author: Muhammad Bilal <meatuni001@gmail.com>

Input: raydium_i2c_ts - propagate device info query errors from raydium_i2c_initialize()

This patch modifies raydium_i2c_initialize() to return the result of info
queries in both bootloader and main modes. It aims to prevent probe from
succeeding and allocating a zero-sized buffer if the info query fails on all
retries. By aborting probe early, it prevents a subsequent NULL pointer
dereference in the IRQ handler.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221777

> diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
> index 0256055abcef3..a0b23772f4c0b 100644
> --- a/drivers/input/touchscreen/raydium_i2c_ts.c
> +++ b/drivers/input/touchscreen/raydium_i2c_ts.c
> @@ -426,9 +426,9 @@ static int raydium_i2c_initialize(struct raydium_data *ts)
>  		ts->boot_mode = RAYDIUM_TS_BLDR;
>  
>  	if (ts->boot_mode == RAYDIUM_TS_BLDR)
> -		raydium_i2c_query_ts_bootloader_info(ts);
> +		error = raydium_i2c_query_ts_bootloader_info(ts);

[Severity: High]
Does this change unintentionally break firmware recovery for devices with HWID
read issues?

Looking at raydium_i2c_query_ts_bootloader_info(), if the HWID read fails, it
falls back to setting a dummy hw_ver but does not clear the error code:

raydium_i2c_query_ts_bootloader_info() {
        ...
        error = raydium_i2c_read(client, RM_CMD_BOOT_CHK, rbuf, sizeof(rbuf));
        if (error) {
                dev_err(&client->dev, "Read HWID command failed: %d (%4ph)\n",
                        error, rbuf + 1);
                hw_ver = 0xffffffffUL;
        } else {
                hw_ver = get_unaligned_be32(rbuf + 1);
        }
        ...
        return error;
}

By propagating this error, probe() will now fail and the device will unbind,
preventing the sysfs node from being created and blocking recovery. Should
the error code be cleared when falling back to the dummy hw_ver?

>  	else
> -		raydium_i2c_query_ts_info(ts);
> +		error = raydium_i2c_query_ts_info(ts);
>  
>  	return error;
>  }

[Severity: High]
This is a pre-existing issue, but does the current driver state bypass the
intended fix of this patch?

Since RAYDIUM_TS_MAIN is 0, which is the default value of ts->boot_mode from
allocation, the retry loop in raydium_i2c_initialize() breaks on the very
first iteration:

raydium_i2c_initialize() {
        for (retry_cnt = 0; retry_cnt < RM_MAX_RETRIES; retry_cnt++) {
                ...
                if (ts->boot_mode == RAYDIUM_TS_BLDR ||
                    ts->boot_mode == RAYDIUM_TS_MAIN) {
                        break;
                }
        }
        ...
}

If the I2C bus is hung and returns all zeros, raydium_i2c_query_ts_info()
will also read 0s, set ts->pkg_size to 0, and return 0 (success). This
completely bypasses the new error propagation check, allowing probe() to
allocate a zero-sized pointer which can still crash the kernel in the IRQ
handler.

[Severity: Critical]
This isn't a bug introduced by this patch, but can a firmware update trigger
a panic due to a zero-sized pointer dereference?

If the device initially probes in bootloader mode, ts->pkg_size remains 0
and probe() allocates a zero-sized pointer for ts->report_data.

During a firmware update, when raydium_i2c_query_ts_info() is called to
switch to MAIN mode, it encounters this check:

raydium_i2c_query_ts_info() {
        ...
        if (ts->report_data && ts->pkg_size != data_info.pkg_size) {
                dev_warn(&client->dev,
                         "report size changes, was: %d, new: %d\n",
                         ts->pkg_size, data_info.pkg_size);
        } else {
                ts->pkg_size = data_info.pkg_size;
                ts->report_size = ts->pkg_size - RM_PACKET_CRC_SIZE;
        }
        ...
}

Since the pointer is non-NULL (0x10), the condition evaluates to true. The
warning is logged, but ts->pkg_size is left as 0 and the buffer is never
reallocated. When the next touch fires raydium_i2c_irq(), it reads 0 bytes
and dereferences the zero-sized pointer.

[Severity: High]
This is a pre-existing issue, but does the driver validate the hardware
parameters read from the device?

In raydium_i2c_query_ts_info(), data_info.pkg_size and data_info.tp_info_size
are read directly from I2C.

If a faulty device returns a pkg_size less than 2, the subtraction
ts->pkg_size - RM_PACKET_CRC_SIZE underflows. If tp_info_size is 0,
ts->contact_size becomes 0.

Later in raydium_mt_event():

raydium_mt_event() {
        ...
        for (i = 0; i < ts->report_size / ts->contact_size; i++) {
                ...
        }
}

Could a 0 contact_size result in a division-by-zero panic here?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for I2C
commands to be interleaved during calibration?

raydium_i2c_calibrate_store() initiates a multi-step sequence without
disabling interrupts:

raydium_i2c_calibrate_store() {
        ...
        scoped_guard(mutex_intr, &ts->sysfs_mutex) {
                error = raydium_i2c_write_object(client, cal_cmd, ...);
        ...
}

Since the IRQ handler does not acquire ts->sysfs_mutex, if a touch occurs
during this sequence, raydium_i2c_irq() can fire and send RM_CMD_BANK_SWITCH,
injecting unexpected commands into the hardware's calibration state machine.

[Severity: High]
This is a pre-existing issue, but does this driver pass stack-allocated memory
to the I2C core?

In raydium_i2c_read():

raydium_i2c_read() {
        struct raydium_bank_switch_header header = {
                .cmd = RM_CMD_BANK_SWITCH,
                .be_addr = cpu_to_be32(addr),
        };
        struct i2c_msg xfer[] = {
                {
                        .addr = client->addr,
                        .len = sizeof(header),
                        .buf = (u8 *)&header,
                },
        ...
}

If the underlying I2C master driver uses DMA, mapping these vmalloc virtual
addresses (when CONFIG_VMAP_STACK=y) will result in a kernel panic. Should
these transfer buffers be heap-allocated instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728135127.48971-1-meatuni001@gmail.com?part=1