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

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

[HyeongJun An]

cs40l50_add() copies the custom data of an FF_PERIODIC/FF_CUSTOM effect
straight from the ff_effect the user passed to EVIOCSFF, without
requiring it to hold anything:

    work_data.custom_data = memdup_array_user(periodic->custom_data,
                                              periodic->custom_len,
                                              sizeof(s16));
    work_data.custom_len = periodic->custom_len;

The driver then reads two words out of that buffer: custom_data[0] as the
waveform bank in cs40l50_effect_bank_set(), and custom_data[1] as the
index within the bank in cs40l50_effect_index_set().  Neither read is
covered by a length check, and custom_len is fully user controlled:

  - custom_len == 0 makes memdup_array_user() call memdup_user() with a
    length of zero, which returns ZERO_SIZE_PTR rather than an error, so
    custom_data[0] dereferences it.

  - custom_len == 1 allocates two bytes.  A bank of ROM or RAM keeps
    effect->type out of the OWT case, and custom_data[1] is then read one
    word past the allocation.

The bank value itself is also mishandled.  It is masked with
CS40L50_CUSTOM_DATA_MASK (0xffff) but stored in an s16, so a
custom_data[0] of 0x8000 or above wraps to a negative value that passes
the "bank_type >= CS40L50_WVFRM_BANK_NUM" test.
cs40l50_effect_index_set() indexes vib->dsp.banks[] with it before the
switch statement's default case gets a chance to reject it:

    base_index = vib->dsp.banks[effect->type].base_index;
    max_index = vib->dsp.banks[effect->type].max_index;

Require the two words the driver reads to be present, and hold the masked
bank in a u32 so the existing upper-bound test covers the whole range.
The da7280 haptic driver already range checks custom_len this way.

Fixes: c38fe1bb5d21 ("Input: cs40l50 - Add support for the CS40L50 haptic driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
 drivers/input/misc/cs40l50-vibra.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/cs40l50-vibra.c b/drivers/input/misc/cs40l50-vibra.c
index 996d6c38cca4..7ef4534fea2f 100644
--- a/drivers/input/misc/cs40l50-vibra.c
+++ b/drivers/input/misc/cs40l50-vibra.c
@@ -139,10 +139,10 @@ static struct cs40l50_effect *cs40l50_find_effect(int id, struct list_head *effe
 static int cs40l50_effect_bank_set(struct cs40l50_work *work_data,
 				   struct cs40l50_effect *effect)
 {
-	s16 bank_type = work_data->custom_data[0] & CS40L50_CUSTOM_DATA_MASK;
+	u32 bank_type = work_data->custom_data[0] & CS40L50_CUSTOM_DATA_MASK;
 
 	if (bank_type >= CS40L50_WVFRM_BANK_NUM) {
-		dev_err(work_data->vib->dev, "Invalid bank (%d)\n", bank_type);
+		dev_err(work_data->vib->dev, "Invalid bank (%u)\n", bank_type);
 		return -EINVAL;
 	}
 
@@ -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) {
+		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,
 						  effect->u.periodic.custom_len,
 						  sizeof(s16));
-- 
2.43.0

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

[sashiko-bot]

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