Skip to content

icvs: fix security and stability bugs in CVS driver #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vad1110 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

icvs: fix security and stability bugs in CVS driver #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a5e685
icvs: fix uninitialized spinlock and work queue objects
vad1110 May 19, 2026
f42ff68
icvs: fix null ACPI handle dereference in probe
vad1110 May 19, 2026
860ce0c
icvs: fix memory leaks in I2C helpers and firmware buffer management
vad1110 May 19, 2026
28d3536
icvs: fix integer type safety in firmware download path
vad1110 May 19, 2026
f5906c1
icvs: fix driver teardown and sysfs download state management
vad1110 May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 114 additions & 34 deletions drivers/misc/icvs/intel_cvs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -142,20 +155,20 @@ 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) {
dev_err(icvs->dev, "Failed to initialize\n");
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)
Expand Down Expand Up @@ -190,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;
}
Expand Down Expand Up @@ -239,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 */
}
}

Expand All @@ -268,22 +299,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,
Expand All @@ -292,17 +321,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);
}
Expand Down Expand Up @@ -505,9 +546,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__);
Expand Down Expand Up @@ -540,19 +583,34 @@ 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) {
dev_err(cvs->dev, "%s:buff is null", __func__);
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;
}

/*
* 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);
Expand All @@ -570,17 +628,36 @@ 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;

/* 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;
Expand All @@ -594,6 +671,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",
Expand Down
26 changes: 23 additions & 3 deletions drivers/misc/icvs/intel_cvs_update.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -256,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)) {
Expand Down Expand Up @@ -331,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;
}
Expand Down Expand Up @@ -464,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;
Expand Down