icvs: fix security and stability bugs in CVS driver#40
Open
vad1110 wants to merge 5 commits into
Open
Conversation
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 <miguel.vadillo@intel.com>
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 <miguel.vadillo@intel.com>
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 <miguel.vadillo@intel.com>
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 <miguel.vadillo@intel.com>
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 <miguel.vadillo@intel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This series fixes 10 bugs found during a security and stability review of the Intel CVS i2c/platform driver. All fixes are self-contained and the module builds cleanly.
One known design issue (firmware loaded via user-supplied fd instead of
request_firmware()) is intentionally deferred as it requires a co-change with the LVFS userspace plugin.Commits
icvs: fix uninitialized spinlock and work queue objectsbuffer_lock,fw_dl_taskwork item, and three wait queue heads were only initialized inside the FULLCAP branch; LIGHTCAP sysfs paths hit uninitialized objects. Adds FULLCAP capability guard in sysfs handlers.icvs: fix null ACPI handle dereference in probeACPI_HANDLE(dev)returns NULL on non-ACPI platforms; was passed unchecked toacpi_evaluate_integer().icvs: fix memory leaks in I2C helpers and firmware buffer managementcvs_read_i2c/cvs_write_i2cuseddevm_kzallocfor per-call buffers — leaked until device removal. Validation moved before allocation. (b)fw_buffernot freed on repeatedpre_storewrites. (c)fw_buffernot freed onkernel_readfailure.icvs: fix integer type safety in firmware download pathfw_buffer_size - FW_BIN_HDR_SIZEwraps to ~4 GB when image < 256 bytes, causing out-of-bounds vmalloc reads. (b) Last-packetfw_size -= I2C_PKT_SIZEunderflows. (c)generic_file_llseekreturn (loff_t) stored inint, truncating files > 2 GiB and producing a wrong seek-back offset.icvs: fix driver teardown and sysfs download state managementcvspointer left dangling on probe failure and after removal. (b)cancel_work_sync()never called in remove — work item could run against freed struct. (c) Earlyreturnon reset failure in remove leftfw_bufferunfreed andcvsdangling. (d)pre_showreset in-flight download state on every sysfs read.Testing
-Wall -Wextra -WerrorNot addressed in this series
plugin_to_cvs_interface.fw_bin_fd): the correct fix isrequest_firmware()+ firmware name via sysfs, but this requires a coordinated change with the LVFS userspace plugin. Tracked separately.