Re: [PATCH v2 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Memory leak and kobject lifecycle violation on the error path in `rmi_create_function()`.
--

--- Patch [2]: [PATCH v2 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans ---
commit 846b91538dc6072de55b69393a47494582f0c196
Author: Wei Jie Law <98lawweijie@gmail.com>

Input: synaptics-rmi4 - reject a PDT that grows between scans

This commit adds a validation check in rmi_create_function() to reject
devices whose interrupt counts grow between PDT scans, preventing an
out-of-bounds bitmap write. When the check fails, it frees the function
using kfree().

> The rejection happens before rmi_register_function(), i.e. before
> device_initialize() has touched fn->dev, so the partially built function
> is disposed of with kfree(): put_device() on a kobject that was never
> initialised warns and then leaves the object behind, because the
> saturated refcount stops the release from ever running.

[Severity: High]
Is this assumption accurate? Looking at rmi_alloc_function() which is called
right at the beginning of rmi_create_function(), it appears
device_initialize() is actually called during allocation:

drivers/input/rmi4/rmi_bus.c:rmi_alloc_function() {
    ...
    device_initialize(&fn->dev);
    dev_set_name(&fn->dev, "%s.fn%02x", dev_name(&rmi_dev->dev), id);
    ...
}

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..8dcf088a7a819 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c

[ ... ]

> @@ -899,6 +899,24 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
>  	fn->irq_pos = *current_irq_count;
>  	*current_irq_count += fn->num_of_irqs;
>  
> +	/*
> +	 * irq_mask[] was sized from the interrupt count collected by the
> +	 * earlier rmi_count_irqs() scan of the PDT.  Nothing guarantees that
> +	 * this scan sees the same table -- the PDT is read back from the
> +	 * device every time -- so a device that grows its interrupt counts
> +	 * between the two scans would push these set_bit() calls past the end
> +	 * of the flexible array.  Refuse the function instead.
> +	 */
> +	if (fn->num_of_irqs > RMI_FN_MAX_IRQS ||
> +	    fn->irq_pos + fn->num_of_irqs > data->irq_count) {
> +		dev_err(dev,
> +			"F%02X: interrupt count changed between PDT scans (pos %u + %u > %d)\n",
> +			pdt->function_number, fn->irq_pos, fn->num_of_irqs,
> +			data->irq_count);
> +		kfree(fn);

[Severity: High]
Does this kfree() bypass the kobject lifecycle rules and cause a memory leak?

Since rmi_alloc_function() already initialized the kobject and dynamically
allocated the device name string via dev_set_name(), shouldn't we use
put_device(&fn->dev) here instead of kfree()?

Calling kfree() directly skips kobject_cleanup(), which permanently leaks
the heap-allocated name string and violates the driver core requirement that
initialized kobjects must be freed via their release method.

> +		return -EINVAL;
> +	}
> +
>  	for (i = 0; i < fn->num_of_irqs; i++)
>  		set_bit(fn->irq_pos + i, fn->irq_mask);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824122733.76321-1-98lawweijie@gmail.com?part=2