Re: [PATCH v2 3/3] input: misc: Add Qualcomm SPMI PMIC haptics driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Concurrent effect uploads or erasures free `fifo_data` while it is actively used by the playback worker or IRQ handler, leading to a Use-After-Free.
- [High] System suspend races with the asynchronous playback worker, leaving the hardware active during sleep and leaking a PM runtime reference.
- [High] Incorrect teardown ordering allows userspace to trigger Use-After-Free of driver structures after removal.
- [High] The driver fails to terminate FIFO playback when finished, permanently leaking a PM runtime reference.
- [Medium] Unbounded user-controlled kmalloc size allows memory exhaustion.
- [Medium] Missing signedness check on the custom play rate allows negative values to bypass boundary checks.
- [Medium] Lockless, non-atomic accesses to `play_request` and `cur_effect_id` cause new play requests to be lost.
- [Medium] The runtime PM usage counter is leaked during device removal if playback is active.
--

commit 06679c85ddc3489b6ff4ed3aec7d938287e570cd
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

input: misc: Add Qualcomm SPMI PMIC haptics driver

Add an initial driver for the Qualcomm PMIH0108 PMIC haptics module,
named as HAP530_HV. This module supports several play modes, including
DIRECT_PLAY, FIFO, PAT_MEM, and SWR, each with distinct data sourcing
and hardware data handling logic. Currently, the driver provides support
for two play modes using the input force-feedback framework: FF_CONSTANT
effect for DIRECT_PLAY mode and FF_PERIODIC effect with FF_CUSTOM
waveform for FIFO mode.

> diff --git a/drivers/input/misc/qcom-spmi-haptics.c b/drivers/input/misc/qcom-spmi-haptics.c
> new file mode 100644
> index 0000000000000..4b27638df9605
> --- /dev/null
> +++ b/drivers/input/misc/qcom-spmi-haptics.c

[ ... ]

> +static irqreturn_t haptics_fifo_empty_irq(int irq, void *dev_id)
> +{
> +	struct qcom_haptics *h = dev_id;
> +	u32 sts, to_write;
> +	int ret;
> +
> +	ret = regmap_read(h->regmap,
> +			  h->cfg_base + HAP_CFG_INT_RT_STS_REG, &sts);
> +	if (ret || !(sts & FIFO_EMPTY_BIT))
> +		return IRQ_HANDLED;
> +
> +	mutex_lock(&h->fifo_lock);
> +
> +	if (!h->fifo_data) {
> +		mutex_unlock(&h->fifo_lock);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (h->data_done) {
> +		ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);
> +		h->fifo_data = NULL;
> +		h->play_request = false;
> +		schedule_work(&h->play_work);
> +		mutex_unlock(&h->fifo_lock);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Refill: write the next chunk, conservatively sized to the threshold */
> +	to_write = min_t(u32, h->data_len - h->data_written,
> +			 h->fifo_len - FIFO_EMPTY_THRESH);
> +	haptics_write_fifo_chunk(h, &h->fifo_data[h->data_written], to_write);

[Severity: High]
Does this code dereference a freed pointer?

A userspace process can start a FIFO effect, then concurrently call 
EVIOCSFF (to re-upload) or EVIOCRMFF (to erase) the effect.

The input core calls haptics_erase() or haptics_upload_effect(), which 
immediately execute kfree(priv->fifo_data).

Since the IRQ handler or worker continues to use h->fifo_data under 
h->fifo_lock, it appears it will dereference the freed memory.

> +	h->data_written += to_write;
> +
> +	if (h->data_written >= h->data_len) {
> +		/* Last chunk enqueued; disable threshold to stop further IRQs */
> +		h->data_done = true;
> +		ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);

[Severity: High]
Does this fail to terminate FIFO playback when finished?

When the final chunk is written, the code sets FIFO_EMPTY_CFG_REG to 0,
disabling the empty interrupt.

Because the interrupt is disabled, the handler will never fire again to
process the h->data_done block at the start of the function. This leaves
the hardware active indefinitely and leaks the PM runtime usage counter.

> +	}
> +
> +	mutex_unlock(&h->fifo_lock);
> +	return IRQ_HANDLED;
> +}

[ ... ]

> +static int haptics_upload_effect(struct input_dev *dev,
> +				 struct ff_effect *effect,
> +				 struct ff_effect *old)
> +{
> +	struct qcom_haptics *h = input_get_drvdata(dev);
> +	struct qcom_haptics_effect *priv;
> +	int id = effect->id;
> +	u32 data_len;
> +	s16 *buf;
> +	s8 *fifo;
> +
> +	if (id < 0 || id >= HAPTICS_MAX_EFFECTS)
> +		return -EINVAL;
> +
> +	priv = &h->effects[id];
> +
> +	switch (effect->type) {
> +	case FF_CONSTANT:
> +		kfree(priv->fifo_data);
> +		priv->fifo_data = NULL;
> +		priv->data_len  = 0;
> +		priv->mode = HAPTICS_DIRECT_PLAY;
> +		return 0;
> +
> +	case FF_PERIODIC:
> +		if (effect->u.periodic.waveform != FF_CUSTOM)
> +			return -EINVAL;
> +		/*
> +		 * Minimum 3 elements: play-rate code + vmax + at least one sample.
> +		 * No upper bound: the FIFO is refilled continuously from the IRQ
> +		 * handler, so any length of PCM data is supported.
> +		 */
> +		if (effect->u.periodic.custom_len < 3)
> +			return -EINVAL;
> +
> +		buf = memdup_array_user(effect->u.periodic.custom_data,
> +					effect->u.periodic.custom_len,
> +					sizeof(s16));

[Severity: Medium]
Is there a missing upper bound check on custom_len?

A local user with write access to the input device can provide a very
large custom_len, and this directly allocates kernel memory without an
upper bound, which could lead to memory exhaustion.

> +		if (IS_ERR(buf))
> +			return PTR_ERR(buf);
> +
> +		if (buf[CUSTOM_DATA_RATE_IDX] > PLAY_RATE_MAX) {

[Severity: Medium]
Can negative values bypass this boundary check?

buf is an array of signed 16-bit integers. A user passing a negative value 
would bypass this check, get cast to u8 later, and be written to the
hardware registers as an out-of-bounds unsigned value.

> +			kfree(buf);
> +			return -EINVAL;
> +		}

[ ... ]

> +static int haptics_playback(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct qcom_haptics *h = input_get_drvdata(dev);
> +
> +	h->cur_effect_id = effect_id;
> +	h->play_request  = (val > 0);

[Severity: Medium]
Are play requests occasionally lost due to lockless accesses?

If the IRQ handler overwrites h->play_request to false immediately after a
new play event from userspace sets it to true here, the new request seems
to be lost. The worker will see play_request == false and stop the
hardware instead of starting the new effect.

> +	schedule_work(&h->play_work);
> +
> +	return 0;
> +}

[ ... ]

> +static void qcom_haptics_remove(struct platform_device *pdev)
> +{
> +	struct qcom_haptics *h = platform_get_drvdata(pdev);
> +	int i;
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +
> +	mutex_lock(&h->play_lock);
> +	haptics_stop_locked(h);
> +	mutex_unlock(&h->play_lock);

[Severity: Medium]
Does this leak the runtime PM usage counter?

If remove is invoked while a haptics effect is actively playing, 
h->pm_ref_held is true. This stops the hardware but does not call
pm_runtime_put_noidle(dev) to release the reference, leaving the PM
state unbalanced.

> +
> +	haptics_enable_module(h, false);
> +	cancel_work_sync(&h->play_work);
> +	for (i = 0; i < HAPTICS_MAX_EFFECTS; i++) {
> +		kfree(h->effects[i].fifo_data);
> +		h->effects[i].fifo_data = NULL;
> +	}
> +
> +	input_unregister_device(h->input);

[Severity: High]
Is it safe to tear down resources before unregistering the input device?

Because the input device is still registered and active, an unprivileged
user can concurrently issue an ioctl (like EVIOCSFF or EVIOCRMFF) on the
open event node. This could access the freed effect structures or schedule 
h->play_work after they have been destroyed.

> +}

[ ... ]

> +static int qcom_haptics_suspend(struct device *dev)
> +{
> +	struct qcom_haptics *h = dev_get_drvdata(dev);
> +
> +	mutex_lock(&h->play_lock);
> +	haptics_stop_locked(h);
> +	if (h->pm_ref_held) {
> +		pm_runtime_put_noidle(dev);
> +		h->pm_ref_held = false;
> +	}
> +	mutex_unlock(&h->play_lock);

[Severity: High]
Does this system suspend race with the asynchronous playback worker?

If userspace requests playback right before system suspend, h->play_request 
is set to true and play_work is scheduled.

This suspend function stops the hardware but forgets to clear
h->play_request.

When this drops the lock, play_work can execute, see play_request is still 
true, and re-enable the hardware just before the system suspends.

> +
> +	cancel_work_sync(&h->play_work);
> +	return pm_runtime_force_suspend(dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-qcom-spmi-haptics-v2-0-b9118e60f3e3@oss.qualcomm.com?part=3