From 1a5e6852f5359a8e0f27e60d1fe7ad8024ff62a5 Mon Sep 17 00:00:00 2001 From: Miguel Vadillo Date: Tue, 19 May 2026 10:02:41 -0700 Subject: [PATCH 1/5] icvs: fix uninitialized spinlock and work queue objects buffer_lock spinlock, fw_dl_task work item and the three wait queue heads (hostwake_event, update_complete_event, lvfs_fwdl_complete_event) were only initialized inside the ICVS_FULLCAP branch. On LIGHTCAP devices the sysfs handlers cvs_ctrl_data_pre_{show,store} and cvs_ctrl_data_fwupd_show are still reachable and use all of those objects, leading to undefined behaviour on the uninitialized memory. Move all initializations unconditionally to the top of cvs_common_probe so every device variant starts with valid objects. Remove the now- redundant duplicate block from inside the FULLCAP branch. Add an ICVS_FULLCAP capability guard in both sysfs handlers so LIGHTCAP devices receive -EOPNOTSUPP rather than silently attempting FW update operations that require hardware resources only present on FULLCAP. Signed-off-by: Miguel Vadillo --- drivers/misc/icvs/intel_cvs.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/drivers/misc/icvs/intel_cvs.c b/drivers/misc/icvs/intel_cvs.c index 061d8c9..75a2610 100644 --- a/drivers/misc/icvs/intel_cvs.c +++ b/drivers/misc/icvs/intel_cvs.c @@ -73,6 +73,19 @@ static int cvs_common_probe(struct device *dev, bool is_i2c) return -ENOMEM; icvs->dev = dev; icvs->has_i2c = is_i2c; + + /* + * Initialize synchronisation primitives unconditionally so sysfs + * handlers and the remove path are safe regardless of capability + * (LIGHTCAP devices never reach the FULLCAP branch below). + */ + spin_lock_init(&icvs->buffer_lock); + INIT_WORK(&icvs->fw_dl_task, cvs_fw_dl_thread); + init_waitqueue_head(&icvs->hostwake_event); + init_waitqueue_head(&icvs->update_complete_event); + init_waitqueue_head(&icvs->lvfs_fwdl_complete_event); + icvs->fw_dl_task_finished = false; + if (is_i2c) { struct i2c_client *i2c = to_i2c_client(dev); i2c_set_clientdata(i2c, icvs); @@ -142,12 +155,6 @@ static int cvs_common_probe(struct device *dev, bool is_i2c) goto exit; } - INIT_WORK(&icvs->fw_dl_task, cvs_fw_dl_thread); - init_waitqueue_head(&icvs->hostwake_event); - init_waitqueue_head(&icvs->update_complete_event); - init_waitqueue_head(&icvs->lvfs_fwdl_complete_event); - icvs->fw_dl_task_finished = false; - if (icvs->has_i2c) { ret = cvs_init(icvs); if (ret) { @@ -505,9 +512,11 @@ static ssize_t cvs_ctrl_data_pre_show(struct device *dev, } count = sizeof(struct cvs_to_plugin_interface); - memset(&cvs->info_fwupd, 0, sizeof(struct ctrl_data_fwupd)); - cvs->fw_dl_task_finished = false; - cvs->fw_dl_task_started = false; + + if (cvs->cap != ICVS_FULLCAP) { + dev_err(cvs->dev, "%s:FW update not supported on LIGHTCAP", __func__); + return -EOPNOTSUPP; + } if (cvs_get_fwver_vid_pid()) { dev_err(cvs->dev, "%s:Not able to read vid/pid", __func__); @@ -548,6 +557,11 @@ static ssize_t cvs_ctrl_data_pre_store(struct device *dev, return -EINVAL; } + if (cvs->cap != ICVS_FULLCAP) { + dev_err(cvs->dev, "%s:FW update not supported on LIGHTCAP", __func__); + return -EOPNOTSUPP; + } + if (count == 0 || count != sizeof(struct plugin_to_cvs_interface)) { dev_err(cvs->dev, "%s:Wrong count:%x", __func__, (unsigned int)count); return -EINVAL; From f42ff68614aec623b5746b515cb37f33a5d1c056 Mon Sep 17 00:00:00 2001 From: Miguel Vadillo Date: Tue, 19 May 2026 10:03:04 -0700 Subject: [PATCH 2/5] icvs: fix null ACPI handle dereference in probe ACPI_HANDLE() returns NULL when the device has no ACPI companion node (e.g. platform devices not enumerated via ACPI). The return value was passed directly to acpi_evaluate_integer() without a null check, causing a null-pointer dereference. Guard the find_oem_prod_id() call so it is only attempted when a valid ACPI handle is available. A missing OEM product ID is non-fatal; the driver continues with oem_prod_id left at its zero-initialised value. Signed-off-by: Miguel Vadillo --- drivers/misc/icvs/intel_cvs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/misc/icvs/intel_cvs.c b/drivers/misc/icvs/intel_cvs.c index 75a2610..cb29dfc 100644 --- a/drivers/misc/icvs/intel_cvs.c +++ b/drivers/misc/icvs/intel_cvs.c @@ -162,7 +162,13 @@ static int cvs_common_probe(struct device *dev, bool is_i2c) goto exit; } - find_oem_prod_id(ACPI_HANDLE(icvs->dev), "OPID", &icvs->oem_prod_id); + /* + * ACPI_HANDLE() returns NULL on non-ACPI platforms; + * passing NULL to acpi_evaluate_integer() oopses. + */ + if (ACPI_HANDLE(icvs->dev)) + find_oem_prod_id(ACPI_HANDLE(icvs->dev), "OPID", + &icvs->oem_prod_id); ret = cvs_find_magic_num_support(icvs); if (ret) From 860ce0c8b1bbd5d6b3237a6865961ec4172ec2c2 Mon Sep 17 00:00:00 2001 From: Miguel Vadillo Date: Tue, 19 May 2026 10:04:15 -0700 Subject: [PATCH 3/5] icvs: fix memory leaks in I2C helpers and firmware buffer management Three related memory leak issues: 1. cvs_read_i2c() and cvs_write_i2c() used devm_kzalloc() for per-call temporary receive/transmit buffers. devm_ allocations are only released at device removal, so every I2C transaction (including those triggered by repeated sysfs reads) accumulates allocations in the device-managed resource list without bound. Switch to kzalloc()/kfree() with explicit release on all paths. Move argument validation before allocation in cvs_read_i2c() to avoid allocating and immediately leaking when inputs are invalid. 2. cvs_ctrl_data_pre_store() allocates fw_buffer with vmalloc() but does not free the previous buffer when called a second time (e.g. LVFS retry). Add an explicit vfree() + NULL reset before each new allocation. 3. On kernel_read() failure in cvs_ctrl_data_pre_store() the function returned -EIO while leaving fw_buffer allocated with partially written data. Free and NULL the buffer so a subsequent write starts from a clean state. Signed-off-by: Miguel Vadillo --- drivers/misc/icvs/intel_cvs.c | 45 ++++++++++++++++++++-------- drivers/misc/icvs/intel_cvs_update.c | 6 ++-- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/drivers/misc/icvs/intel_cvs.c b/drivers/misc/icvs/intel_cvs.c index cb29dfc..c15d133 100644 --- a/drivers/misc/icvs/intel_cvs.c +++ b/drivers/misc/icvs/intel_cvs.c @@ -281,22 +281,20 @@ static void cvs_platform_remove(struct platform_device *pdev) int cvs_read_i2c(u16 cmd, char *data, int size) { struct intel_cvs *ctx = cvs; - if (!ctx || !ctx->has_i2c) - return -EOPNOTSUPP; - struct i2c_client *i2c = container_of(cvs->dev, struct i2c_client, dev); + struct i2c_client *i2c; + u16 cvs_cmd; int cnt; - char *in_data; - in_data = devm_kzalloc(ctx->dev, (CVMAGICNUMSIZE + size), GFP_KERNEL); - if (!in_data) { - dev_err(cvs->dev, "%s:Buffer allocation failed", __func__); - return -ENOMEM; - } - u16 cvs_cmd = (((cmd) >> 8) & 0x00ff) | (((cmd) << 8) & 0xff00); + if (!ctx || !ctx->has_i2c) + return -EOPNOTSUPP; + /* Validate before any allocation */ if (size < 0 || !data) return -EINVAL; + i2c = container_of(cvs->dev, struct i2c_client, dev); + cvs_cmd = (((cmd) >> 8) & 0x00ff) | (((cmd) << 8) & 0xff00); + cnt = i2c_master_send(i2c, (const char *)&cvs_cmd, sizeof(u16)); if (cnt != sizeof(u16)) { dev_err(&i2c->dev, "%s:cmd:%x count:%d (!=2)\n", __func__, cmd, @@ -305,17 +303,29 @@ int cvs_read_i2c(u16 cmd, char *data, int size) } if (ctx->magic_num_support) { + /* + * Use kzalloc for the per-call receive buffer; devm_kzalloc + * retains every allocation until device removal, so calling + * this function repeatedly leaks device-managed memory + * without bound. + */ + char *in_data = kzalloc(CVMAGICNUMSIZE + size, GFP_KERNEL); u32 magic_num_received; + if (!in_data) { + dev_err(cvs->dev, "%s:Buffer allocation failed", __func__); + return -ENOMEM; + } cnt = i2c_master_recv(i2c, in_data, size + CVMAGICNUMSIZE); magic_num_received = ((u32 *)in_data)[0]; - if (magic_num_received == CVMAGICNUM) { + if (magic_num_received == CVMAGICNUM) { dev_dbg(&i2c->dev, "%s:Valid magic number", __func__); memcpy(data, in_data + CVMAGICNUMSIZE, size); } else { dev_dbg(&i2c->dev, "%s:Invalid magic number", __func__); - return -EIO; + cnt = -EIO; } + kfree(in_data); } else { cnt = i2c_master_recv(i2c, (char *)data, size); } @@ -598,9 +608,15 @@ static ssize_t cvs_ctrl_data_pre_store(struct device *dev, cvs->fw_buffer_size = fw_bin_size; cvs->max_flashtime_ms = cvs->plugin_to_cvs.max_flash_time; cvs->fw_update_retries = cvs->plugin_to_cvs.max_fwupd_retry_count; + + /* Free any buffer left over from a previous (possibly failed) update */ + if (cvs->fw_buffer) { + vfree(cvs->fw_buffer); + cvs->fw_buffer = NULL; + } cvs->fw_buffer = vmalloc(cvs->fw_buffer_size); - if (IS_ERR_OR_NULL(cvs->fw_buffer)) { + if (!cvs->fw_buffer) { dev_err(cvs->dev, "%s:No memory for fw_buffer", __func__); fput(f); return -ENOMEM; @@ -614,6 +630,9 @@ static ssize_t cvs_ctrl_data_pre_store(struct device *dev, if (bytes_read != cvs->fw_buffer_size) { dev_err(cvs->dev, "%s:kernel_read failed with bytes_read:%lx", __func__, bytes_read); + /* Free the buffer to avoid leaving stale data on retry */ + vfree(cvs->fw_buffer); + cvs->fw_buffer = NULL; return -EIO; } else { dev_info(cvs->dev, "%s:Full fw_buffer received. Start fw_download", diff --git a/drivers/misc/icvs/intel_cvs_update.c b/drivers/misc/icvs/intel_cvs_update.c index 5f069aa..e254489 100644 --- a/drivers/misc/icvs/intel_cvs_update.c +++ b/drivers/misc/icvs/intel_cvs_update.c @@ -43,8 +43,9 @@ int cvs_write_i2c(u16 cmd, u8 *data, u32 len) u8 *out_buff; union cv_host_identifiers host_identifiers; - out_buff = devm_kzalloc(ctx->dev, - cv_host_identifier_size + sizeof(cmd), GFP_KERNEL); + /* Use kzalloc; devm_kzalloc leaks on every call until removal */ + out_buff = kzalloc(cv_host_identifier_size + sizeof(cmd), + GFP_KERNEL); if (!out_buff) { dev_err(cvs->dev, "%s:Buffer alloc failed", __func__); return -ENOMEM; @@ -61,6 +62,7 @@ int cvs_write_i2c(u16 cmd, u8 *data, u32 len) count = i2c_master_send(client, (const char *)out_buff, sizeof(cmd) + cv_host_identifier_size); + kfree(out_buff); if (count != cv_host_identifier_size + sizeof(cmd)) return -EIO; From 28d353662e812d026cf22faf635e6f9db1492b2a Mon Sep 17 00:00:00 2001 From: Miguel Vadillo Date: Tue, 19 May 2026 10:06:19 -0700 Subject: [PATCH 4/5] icvs: fix integer type safety in firmware download path Two related issues: 1. fw_buffer_size is declared as u32. In both cvs_dev_fw_dl_data() and cvs_fw_dl_thread() the expression fw_buffer_size - FW_BIN_HDR_SIZE is evaluated as unsigned arithmetic. When the firmware image is smaller than FW_BIN_HDR_SIZE (256 bytes) the result wraps to a large positive value, causing a while loop that reads far beyond the vmalloc buffer. Add an explicit size check before the subtraction in both call sites. Additionally, cvs_dev_fw_dl_data() advances fw_size by a fixed I2C_PKT_SIZE on every iteration. The last packet may be shorter; the unsigned subtraction wraps and fw_buff_ptr overshoots the buffer. Replace the subtraction with min_t(u32, fw_size, I2C_PKT_SIZE). 2. cvs_ctrl_data_pre_store() stored the return value of generic_file_llseek() (loff_t / s64) in an int, silently truncating files larger than 2 GiB and producing a negative seek-back delta that would seek forward instead of rewinding. Replace the int local with loff_t, switch to vfs_llseek(), and treat a non-positive result as an error rather than proceeding with a bogus size. Signed-off-by: Miguel Vadillo --- drivers/misc/icvs/intel_cvs.c | 23 ++++++++++++++++++----- drivers/misc/icvs/intel_cvs_update.c | 20 +++++++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/drivers/misc/icvs/intel_cvs.c b/drivers/misc/icvs/intel_cvs.c index c15d133..7f9e124 100644 --- a/drivers/misc/icvs/intel_cvs.c +++ b/drivers/misc/icvs/intel_cvs.c @@ -565,7 +565,7 @@ static ssize_t cvs_ctrl_data_pre_store(struct device *dev, { unsigned long flags; struct file *f; - int fw_bin_size; + loff_t fw_bin_size; ssize_t bytes_read = 0; if (!buf) { @@ -600,12 +600,25 @@ static ssize_t cvs_ctrl_data_pre_store(struct device *dev, return -EBADF; } - fw_bin_size = generic_file_llseek(f, 0, SEEK_END); - dev_dbg(cvs->dev, "%s:Calculated fw_bin size:%x bytes", + /* + * generic_file_llseek() returns loff_t; assigning to int silently + * truncates files larger than 2 GiB and produces a wrong (positive) + * seek-back offset. Use vfs_llseek() + loff_t throughout, and treat + * a non-positive size as an error rather than seeking backwards. + */ + fw_bin_size = vfs_llseek(f, 0, SEEK_END); + if (fw_bin_size <= 0) { + dev_err(cvs->dev, "%s:Failed to determine fw file size: %lld", + __func__, fw_bin_size); + fput(f); + return fw_bin_size < 0 ? (ssize_t)fw_bin_size : -EINVAL; + } + vfs_llseek(f, 0, SEEK_SET); + + dev_dbg(cvs->dev, "%s:Calculated fw_bin size:%llx bytes", __func__, fw_bin_size); - generic_file_llseek(f, -fw_bin_size, SEEK_CUR); - cvs->fw_buffer_size = fw_bin_size; + cvs->fw_buffer_size = (u32)fw_bin_size; cvs->max_flashtime_ms = cvs->plugin_to_cvs.max_flash_time; cvs->fw_update_retries = cvs->plugin_to_cvs.max_fwupd_retry_count; diff --git a/drivers/misc/icvs/intel_cvs_update.c b/drivers/misc/icvs/intel_cvs_update.c index e254489..cb77537 100644 --- a/drivers/misc/icvs/intel_cvs_update.c +++ b/drivers/misc/icvs/intel_cvs_update.c @@ -258,6 +258,18 @@ int cvs_dev_fw_dl_data(void) dev_info(cvs->dev, "%s:Enter", __func__); fw_buff_ptr = (u8 *)ctx->fw_buffer + FW_BIN_HDR_SIZE; + + /* + * fw_buffer_size is u32; subtracting FW_BIN_HDR_SIZE without + * checking wraps to a huge value and causes an out-of-bounds read + * over the entire vmalloc'd buffer. + */ + if (ctx->fw_buffer_size <= FW_BIN_HDR_SIZE) { + dev_err(cvs->dev, "%s:fw image too small (%u bytes)", __func__, + ctx->fw_buffer_size); + kfree(out_buf); + return -EINVAL; + } fw_size = ctx->fw_buffer_size - FW_BIN_HDR_SIZE; while ((fw_size > 0) && (ctx->icvs_state != CV_STOPPING)) { @@ -333,7 +345,8 @@ int cvs_dev_fw_dl_data(void) goto err_exit; } ctx->info_fwupd.num_packets_sent++; - fw_size -= I2C_PKT_SIZE; + /* min_t prevents u32 underflow on the last (short) chunk */ + fw_size -= min_t(u32, fw_size, I2C_PKT_SIZE); fw_buff_ptr += I2C_PKT_SIZE; ctx->cv_fw_state = fw_state; } @@ -466,6 +479,11 @@ void cvs_fw_dl_thread(struct work_struct *arg) return; } + if (ctx->fw_buffer_size <= FW_BIN_HDR_SIZE) { + dev_err(cvs->dev, "%s:fw image too small (%u bytes)", __func__, + ctx->fw_buffer_size); + goto xit; + } fw_size = ctx->fw_buffer_size - FW_BIN_HDR_SIZE; ctx->info_fwupd.total_packets = fw_size / I2C_PKT_SIZE; ctx->info_fwupd.total_packets += (fw_size % I2C_PKT_SIZE) ? 1 : 0; From f5906c1a2dac3202376b56b219263d8ef7d93223 Mon Sep 17 00:00:00 2001 From: Miguel Vadillo Date: Tue, 19 May 2026 10:07:34 -0700 Subject: [PATCH 5/5] icvs: fix driver teardown and sysfs download state management Three related lifecycle and state-consistency issues: 1. The global 'cvs' pointer was assigned at the beginning of cvs_common_probe() before hardware initialisation succeeded. On any failure path the struct is freed via devm_kfree() but cvs is left pointing at freed memory. Set cvs = NULL in the failure branch of the exit label. In cvs_common_remove() the pointer was also never cleared after devm_kfree(), so any code running after remove (e.g. a concurrent sysfs access) would dereference freed memory. Set cvs = NULL at the end of the remove path. 2. cancel_work_sync() was never called in cvs_common_remove(). The existing signal + wait_event_interruptible() sequence is only reached when fw_dl_task_started is true; if the work item had been scheduled but had not yet set that flag the work would run against the already-freed intel_cvs struct. Add cancel_work_sync() after the signal/wait block to cover that race. Also remove the premature 'return' on cvs_reset_cv_device() failure that left fw_buffer unfreed and cvs dangling; log the error and continue cleanup instead. 3. cvs_ctrl_data_pre_show() reset info_fwupd, fw_dl_task_finished and fw_dl_task_started on every read. Any concurrent sysfs read during an in-flight download zeroed the progress counters and caused the LVFS plugin to see a spurious completion or restart condition. Move the state reset to cvs_ctrl_data_pre_store() where it belongs: at the beginning of a new update request, not on every read. Signed-off-by: Miguel Vadillo --- drivers/misc/icvs/intel_cvs.c | 40 +++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/drivers/misc/icvs/intel_cvs.c b/drivers/misc/icvs/intel_cvs.c index 7f9e124..99f76fa 100644 --- a/drivers/misc/icvs/intel_cvs.c +++ b/drivers/misc/icvs/intel_cvs.c @@ -203,8 +203,15 @@ static int cvs_common_probe(struct device *dev, bool is_i2c) acpi_dev_clear_dependencies(ACPI_COMPANION(icvs->dev)); exit: - if (ret) + if (ret) { + /* + * cvs was assigned early in probe before we knew whether + * initialisation would succeed. Clear it now so no other + * code path dereferences the about-to-be-freed struct. + */ + cvs = NULL; devm_kfree(icvs->dev, icvs); + } return ret; } @@ -252,19 +259,30 @@ static void cvs_common_remove(struct device *dev, bool is_i2c) mdelay(WAIT_HOST_RELEASE_MS); } - /* reset vision chip */ - if (cvs_reset_cv_device()) { + /* + * Guarantee the work item is not queued or running before + * we free the struct. The signal+wait above handles the + * running case; cancel_work_sync() catches the race where + * fw_dl_task_started was never set but the work was already + * enqueued. + */ + cancel_work_sync(&icvs->fw_dl_task); + + /* reset vision chip; log but do not abort cleanup on failure */ + if (cvs_reset_cv_device()) dev_err(cvs->dev, "%s:CV reset fail after flash", __func__); - return; - } + cvs->icvs_state = CV_INIT_STATE; cvs_exit(icvs); } - if (cvs->fw_buffer) + if (cvs->fw_buffer) { vfree(cvs->fw_buffer); + cvs->fw_buffer = NULL; + } devm_kfree(dev, icvs); + cvs = NULL; /* prevent use-after-free via the global pointer */ } } @@ -583,6 +601,16 @@ static ssize_t cvs_ctrl_data_pre_store(struct device *dev, return -EINVAL; } + /* + * Reset download tracking state here (at the start of a new update + * request) rather than in the show handler. Resetting in show means + * any concurrent read (even an unprivileged one) silently zeroes + * in-flight progress counters and triggers a spurious re-download. + */ + memset(&cvs->info_fwupd, 0, sizeof(struct ctrl_data_fwupd)); + cvs->fw_dl_task_finished = false; + cvs->fw_dl_task_started = false; + spin_lock_irqsave(&cvs->buffer_lock, flags); memcpy(&cvs->plugin_to_cvs, buf, count); spin_unlock_irqrestore(&cvs->buffer_lock, flags);