Re: [PATCH] Input: cs40l50-vibra - validate custom data from user space

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Updating an existing OWT effect leaks DSP memory and desynchronizes indices.
- [High] Out-of-bounds slab read in cs40l50_upload_owt due to passing byte count instead of element count to regmap_bulk_write.
- [High] The driver allocates devm resources against the parent device rather than its own child device, leaking resources across unbind cycles.
- [High] Local DoS via unbounded synchronous sleeping in ordered workqueue during playback.
- [Medium] Unhandled odd custom_len values lead to truncated OWT data lengths.
- [Medium] Memory structures are permanently leaked if hardware cannot be resumed during worker execution.
--

--- Patch [1]: [PATCH] Input: cs40l50-vibra - validate custom data from user space ---
commit a72a6893fcb7d7f39ce7369bea556bb585afa8b7
Author: HyeongJun An <sammiee5311@gmail.com>

Input: cs40l50-vibra - validate custom data from user space

This commit adds validation for custom data lengths to prevent zero or one-byte
allocations. It also updates the bank value to a 32-bit unsigned integer to
avoid bypassing upper bounds checks with negative values.

> diff --git a/drivers/input/misc/cs40l50-vibra.c b/drivers/input/misc/cs40l50-vibra.c
> index 996d6c38cca43..7ef4534fea2fb 100644
> --- a/drivers/input/misc/cs40l50-vibra.c
> +++ b/drivers/input/misc/cs40l50-vibra.c
[ ... ]
> @@ -326,6 +326,12 @@ static int cs40l50_add(struct input_dev *dev, struct ff_effect *effect,
>  		return -EINVAL;
>  	}
>  
> +	if (periodic->custom_len < CS40L50_OWT_CUSTOM_DATA_SIZE) {

[Severity: Medium]
This is a pre-existing issue, but would this check ensure `custom_len` is even?

Since `custom_len` represents the number of 16-bit words, an odd value results
in a byte length that is not a multiple of 4. Later in `cs40l50_upload_owt()`:

cs40l50_upload_owt() {
    ...
    header.data_words = len / sizeof(u32);
    ...
}

Could this integer division truncate the unaligned payload length before it is
sent to the DSP?

> +		dev_err(vib->dev, "Invalid custom data length (%u)\n",
> +			periodic->custom_len);
> +		return -EINVAL;
> +	}
> +
>  	work_data.custom_data = memdup_array_user(effect->u.periodic.custom_data,

[Severity: High]
This is a pre-existing issue, but is it possible that updating an existing OWT
effect leaks DSP memory and desynchronizes indices?

When an existing OWT effect is updated, it seems `cs40l50_add_worker()` pushes
a new waveform to the DSP without deleting the old one:

cs40l50_add_worker() {
    ...
    effect = cs40l50_find_effect(work_data->effect->id, &vib->effect_head);
    ...
}

Might this leak DSP memory until it permanently errors with `-ENOSPC`?

Additionally, `cs40l50_effect_index_set()` computes the new index by counting
OWT elements in `vib->effect_head`:

cs40l50_effect_index_set() {
    ...
    list_for_each_entry(owt_effect, &vib->effect_head, list)
        if (owt_effect->type == CS40L50_WVFRM_BANK_OWT)
            effect->index++;
    ...
}

Since `CS40L50_EFFECTS_MAX` is 1, this count is always 1 during an update.
However, the DSP increments its internal index for each push. Will this cause
the software index to permanently desynchronize from the DSP after multiple
updates, resulting in the wrong effect being played?

[Severity: High]
This is a pre-existing issue, but could there be an out-of-bounds read in
`cs40l50_upload_owt()`?

cs40l50_upload_owt() {
    ...
    error = regmap_bulk_write(vib->regmap, vib->dsp.owt_base_reg +
                              (offset * sizeof(u32)), new_owt_effect_data,
                              sizeof(header) + len);
    ...
}

It appears `sizeof(header) + len` is passed as the `val_count` argument to
`regmap_bulk_write()`. Since the device's regmap configuration sets
`val_bits = 32`, `regmap_bulk_write()` interprets `val_count` as the number of
elements, not bytes.

Would this multiply the count by 4, reading 4x the allocated bytes from
`new_owt_effect_data` and sending arbitrary kernel memory to the DSP?

[Severity: Medium]
This is a pre-existing issue, but are memory structures leaked if the hardware
cannot be resumed?

In `cs40l50_stop_worker()`:

cs40l50_stop_worker() {
    ...
    if (pm_runtime_resume_and_get(vib->dev) < 0)
        return;
    ...
}

Would this early return bypass the `kfree(work_data)` call, leaking the
dynamically allocated work structure?

Similarly, in `cs40l50_erase_worker()`:

cs40l50_erase_worker() {
    ...
    error = pm_runtime_resume_and_get(vib->dev);
    if (error)
        goto err_exit;
    ...
}

Can this failure path skip removing the effect from `vib->effect_head` and
`kfree(erase_effect)`, leaving the effect state permanently stranded?

[Severity: High]
This is a pre-existing issue, but are devm resources allocated against the wrong
device?

cs40l50_vibra_probe() {
    ...
    vib = devm_kzalloc(pdev->dev.parent, sizeof(*vib), GFP_KERNEL);
    ...
}

Because `devm_kzalloc()`, `devm_input_allocate_device()`, and
`devm_add_action_or_reset()` are bound to `pdev->dev.parent` (the parent MFD
device), will these resources be leaked across unbind cycles when the child
driver is unbound via sysfs?

[Severity: High]
This is a pre-existing issue, but could there be an unbounded synchronous sleep
in the workqueue during playback?

When a playback event is triggered, `work_data->count` comes from the
user-controlled event `value`:

cs40l50_start_worker() {
    ...
    while (--work_data->count >= 0) {
        vib->dsp.write(vib->dev, vib->regmap, start_effect->index);
        usleep_range(work_data->effect->replay.length,
                     work_data->effect->replay.length + 100);
    }
    ...
}

Could a user pass `count = 0x7FFFFFFF`, causing the worker to sleep for up to
4.4 years?

Since `vib->vib_wq` is an ordered workqueue, would this completely block all
other work items?

If another process attempts to add, erase, or close the device descriptor, it
will call `flush_work()`:

cs40l50_add() {
    ...
    flush_work(&work_data.work);
    ...
}

Will this cause the process to hang in `TASK_UNINTERRUPTIBLE` indefinitely if
the workqueue is blocked?

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