Re: [PATCH v5 4/6] HID: himax: Load the firmware from a file
From: sashiko-bot@kernel.org
Date: 2026-10-03 17:56:12
Also in:
linux-devicetree
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider: - [High] hid: himax: out-of-bounds read and heap overflow during firmware parsing - [High] hid: himax: data race and hardware corruption during concurrent firmware updates - [High] hid: himax: use-after-free of HID device due to uncoordinated asynchronous initialization - [High] hid: himax: double-free and use-after-free of HID device on concurrent resume and initialization - [High] hid: himax: integer underflow in HID report length calculation causes huge out-of-bounds read - [High] hid: himax: null pointer dereference in threaded IRQ during device removal - [High] hid: himax: unaligned memory access via direct pointer casting - [Medium] hid: himax: memory leak of zf_info array on error path - [Medium] hid: himax: out-of-bounds read and info leak via %s formatting - [Medium] hid: himax: interrupt registered with default level trigger instead of firmware configuration - [Low] hid: himax: unreachable cancel_delayed_work_sync and misplaced error label in chip init -- commit a448774427152dbb6ec0aa1a91961e9842a21cc0 Author: Allen_Lin [off-list ref] Subject: HID: himax: Load the firmware from a file The HX83102J is often shipped without a flash. The firmware then has to be loaded from a file into the SRAM of the IC when the driver probes and on every resume, as the IC loses it when it is powered down.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-himax.c b/drivers/hid/hid-himax.c --- a/drivers/hid/hid-himax.c +++ b/drivers/hid/hid-himax.c
[ ... ]
+static int himax_mcu_read_FW_ver(struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ ret = himax_mcu_register_read(ts, HIMAX_DSRAM_ADDR_CUS_INFO, data, HIMAX_TP_INFO_STR_LEN); + if (ret < 0) { + dev_err(ts->dev, "%s: read customer info fail\n", __func__); + return ret; + } + memcpy(ts->ic_data.vendor_cus_info, data, HIMAX_TP_INFO_STR_LEN); + dev_info(ts->dev, "%s: Cusomer ID : %s\n", __func__, ts->ic_data.vendor_cus_info);
[Severity: Medium] Could this print out-of-bounds memory into the kernel log? The firmware data copied into the fixed-size vendor_cus_info array may not be null-terminated. Should a precision specifier like %.*s be used here to bound the string formatting to HIMAX_TP_INFO_STR_LEN? [ ... ]
+static bool himax_bin_desc_data_get(struct himax_ts_data *ts, u32 addr, u8 *descript_buf)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ } else { /* 3. get data */ + map_data.buf = &descript_buf[i]; + map_code = le32_to_cpup(map_data.word);
[Severity: High] Does this pointer cast cause an unaligned access regression? Using a union to cast an unaligned byte pointer from the firmware image to a 32-bit integer pointer might cause a fault on architectures requiring strict alignment. Would it be safer to use get_unaligned_le32() here? [ ... ]
+static bool himax_mcu_bin_desc_get(unsigned char *fw, struct himax_ts_data *ts, u32 max_sz)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ /* Check bin is with description table or not */ + if (!(memcmp(fw, header, header_sz) == 0 && fw[header_id_loc] == header_id)) { + dev_err(ts->dev, "%s: No description table\n", __func__); + return false; + }
[Severity: High] Is there a risk of an out-of-bounds read here if the firmware file is shorter than expected? The code accesses fw[14] (header_id_loc) and calls memcmp without verifying if fw->size is at least large enough to contain the header. [ ... ]
+static int himax_mcu_tp_info_check(struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ ret = himax_mcu_register_read(ts, HIMAX_DSRAM_ADDR_X_Y_RES, data, 4); + if (ret < 0) { + dev_err(ts->dev, "%s: read x/y resolution fail\n", __func__); + return ret; + } + y_res = be16_to_cpup((u16 *)&data[0]); + x_res = be16_to_cpup((u16 *)&data[2]);
[Severity: High] Can this cause an alignment fault on architectures with strict alignment requirements? Casting a byte array directly to a 16-bit pointer and dereferencing it performs an unaligned access. Should this use get_unaligned_be16() instead? [ ... ]
+static int himax_zf_part_info(const struct firmware *fw, struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ if (i_min < 0 || i_max < 0) { + dev_err(ts->dev, "%s: DSRAM address invalid!\n", __func__); + return -EINVAL; + }
[Severity: Medium] Does this error path leak the dynamically allocated info array? It appears the function returns directly without calling kfree(info).
quoted hunk ↗ jump to hunk
+ + /* 3. prepare data to update */ + sram_min = info[i_min].cfg_addr; + + cfg_sz = (dsram_max - dsram_base) + info[i_max].write_size;
[Severity: High] Could this configuration size calculation bypass the bounds check and lead to a heap buffer overflow later? By assuming the highest address block (i_max) also holds the end boundary, this might undercalculate the true required buffer size if a lower-addressed partition has a very large payload.
quoted hunk ↗ jump to hunk
+ /* Wrtie size must be multiple of 4 */ + if (cfg_sz % 4 != 0) + cfg_sz = cfg_sz + 4 - (cfg_sz % 4);
[ ... ]
quoted hunk ↗ jump to hunk
+ memset(ts->zf_update_cfg_buffer, 0x00, + ts->chip_max_dsram_size * sizeof(u8)); + + /* Collect all partition in FW for DSRAM in a cfg buffer */ + for (i = 1; i < pnum; i++) + memcpy(&ts->zf_update_cfg_buffer[info[i].cfg_addr - dsram_base], + &fw->data[info[i].fw_addr], info[i].write_size);
[Severity: High] Does this memcpy risk overflowing the allocated zf_update_cfg_buffer or reading out-of-bounds from the firmware file? If the firmware provides malformed partition offsets or lengths, this could lead to memory corruption. [ ... ]
+static int himax_zf_reload_from_file(char *file_name, struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ if (!mutex_trylock(&ts->zf_update_lock)) { + dev_warn(ts->dev, "%s: Other thread is updating now!\n", __func__); + return 0; + }
[Severity: High] Does returning 0 here falsely report success to the resume path if another thread is already updating the firmware? Furthermore, since himax_initial_work() initiates the initial firmware update without holding the zf_update_lock, could both threads race and concurrently update the firmware, leading to hardware corruption regressions? [ ... ]
+static int himax_hid_probe(struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ hid = ts->hid; + if (hid) { + hid_destroy_device(hid); + hid = NULL; + }
[Severity: High] Is it safe to destroy and recreate the HID device here without locking against concurrent callers? Both himax_initial_work() and himax_chip_resume() call himax_hid_probe(). If a system suspend and resume occur while the asynchronous initial update is running, they might invoke hid_destroy_device() simultaneously on the same device, leading to a double-free or use-after-free regression. [ ... ]
quoted hunk ↗ jump to hunk
+static void himax_hid_remove(struct himax_ts_data *ts) +{ + if (ts && ts->hid) + hid_destroy_device(ts->hid); + else + return; + + ts->hid = NULL; +}
[Severity: High] Does this removal logic leave the ts->hid_probed flag set to true while ts->hid is set to NULL? The caller himax_spi_drv_remove() uses disable_irq_nosync(), which does not wait for active threaded handlers to finish. If a concurrent himax_ts_operation() executes, it will see ts->hid_probed as true and attempt to call hid_input_report() with a NULL ts->hid pointer, which could crash the system. [ ... ]
+static int himax_ts_operation(struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ if (ts->hid_probed) { + offset = ts->hid_desc.max_input_length; + if (ts->ic_data.stylus_function) { + ret += himax_hid_report(ts, + ts->xfer_buf + offset + HIMAX_HID_REPORT_HDR_SZ, + ts->hid_desc.max_input_length - + HIMAX_HID_REPORT_HDR_SZ);
[Severity: High] Can this subtraction underflow if the firmware provides a max_input_length of 0 or 1? Since HIMAX_HID_REPORT_HDR_SZ is 2, an underflow would result in a huge unsigned length being passed to the HID report, potentially causing a massive out-of-bounds memory read. [ ... ]
+static void himax_initial_work(struct work_struct *work)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ if (himax_update_fw(ts)) { + dev_err(ts->dev, "%s: Update FW fail\n", __func__); + goto err_update_fw_failed; + }
[Severity: High] Should this firmware update be protected by the ts->zf_update_lock? The resume path uses a trylock on zf_update_lock, but the initial asynchronous worker calls this without holding the lock. This could allow concurrent hardware updates if a suspend and resume happens immediately after boot. [ ... ]
+static int himax_chip_init(struct himax_ts_data *ts)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ if (himax_ts_register_interrupt(ts)) { + dev_err(ts->dev, "%s: register interrupt failed\n", __func__); + return -EIO; + }
[Severity: Medium] Will the interrupt be registered with the wrong trigger type here? himax_ts_register_interrupt() is called during driver probe and uses ts->ic_data.interrupt_is_edge. Since the ts structure is zero-allocated, it defaults to level triggering. However, the true firmware configuration is read asynchronously later in himax_initial_work() via himax_mcu_tp_info_check(), which is too late to affect the interrupt registration. [ ... ]
quoted hunk ↗ jump to hunk
+ ts->initialized = true; + + return 0; + cancel_delayed_work_sync(&ts->initial_work); +err_update_cfg_buf_alloc_failed: + + return ret; +}
[Severity: Low] Is the code following the return statement unreachable? The unconditional return makes the cancellation of the delayed work dead code. Additionally, the err_update_cfg_buf_alloc_failed label is placed after the return, meaning the failure path will also exit with 0 instead of propagating the allocation error. [ ... ]
static void himax_spi_drv_remove(struct spi_device *spi)
{[ ... ]
quoted hunk ↗ jump to hunk
+ if (ts->probe_finish) { + if (ts->ic_boot_done) { + himax_int_enable(ts, false); + + if (ts->hid_probed) + himax_hid_remove(ts); + } + himax_chip_deinit(ts); + himax_platform_deinit(ts); + } }
[Severity: High] Could this sequence lead to a use-after-free regression of the HID device? The check for ic_boot_done happens before himax_chip_deinit() cancels the delayed initialization work. If the delayed work is currently executing, ic_boot_done is false, so himax_hid_remove() is skipped. The work could then finish and register the HID device right after the SPI driver has been unbound and the ts structure freed, leaving an active HID device with a dangling pointer. -- Sashiko AI review · https://sashiko.dev/#/patchset/20261003142741.48634-1-michal@nozomi.space?part=4