Re: [PATCH 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The irq_domain allocated by irq_domain_create_linear() is leaked on error paths during probe and firmware updates.
- [High] The RMI firmware sysfs group created by rmi_f34_create_sysfs() is leaked on error paths during probe, leading to a NULL pointer dereference.
- [High] Failure in rmi_f34_create_sysfs during probe skips child device cleanup, leading to resource leaks and NULL pointer dereferences.
--

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

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

This patch fixes an out-of-bounds access in rmi_create_function(). It adds
a validation check for fn->irq_pos + fn->num_of_irqs to ensure that a device
that grows its interrupt count between scans cannot walk past the end of the
flexible array. The probe will now fail instead of overflowing memory.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..c0e91a372eff4 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);
> +		put_device(&fn->dev);
> +		return -EINVAL;
> +	}
> +
>  	for (i = 0; i < fn->num_of_irqs; i++)
>  		set_bit(fn->irq_pos + i, fn->irq_mask);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does rmi_driver_probe() leak
the irqdomain allocated in rmi_probe_interrupts() on failure paths?

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
...
	retval = rmi_probe_interrupts(data);
	if (retval)
		goto err;
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If any subsequent initialization step in rmi_driver_probe() fails, the error
path bypasses irq_domain_remove(), permanently leaking the domain. 

Does this leak also occur during firmware updates when rmi_probe_interrupts()
is called repeatedly without cleaning up the previous irqdomain?

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
...
	ret = rmi_probe_interrupts(data);
	if (ret)
		return ret;
...
}

[Severity: High]
This is a pre-existing issue, but does the error path in rmi_driver_probe()
leave the f34 sysfs group active, potentially leading to a null pointer
dereference?

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
...
	retval = rmi_f34_create_sysfs(rmi_dev);
	if (retval)
		goto err;

	if (data->input) {
		rmi_driver_set_input_name(rmi_dev, data->input);
		if (!rmi_dev->xport->input) {
			retval = input_register_device(data->input);
			if (retval) {
				dev_err(dev, "%s: Failed to register input device.\n",
					__func__);
				goto err_destroy_functions;
			}
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If input_register_device() fails, the code jumps to err_destroy_functions,
which cleans up the function list but neglects to call
rmi_f34_remove_sysfs().

When the probe fails, the driver core sets the device's drvdata to null. If
userspace accesses these stale files, callbacks will call dev_get_drvdata()
and dereference a null pointer.

[Severity: High]
This isn't a bug introduced by this patch, but does a failure in
rmi_f34_create_sysfs() bypass the cleanup for child functions?

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
...
	retval = rmi_init_functions(data);
	if (retval)
		goto err;

	retval = rmi_f34_create_sysfs(rmi_dev);
	if (retval)
		goto err;
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If rmi_f34_create_sysfs() fails, the code jumps directly to the err label,
completely skipping the err_destroy_functions cleanup for the already
registered child functions.

When probe fails, the driver core clears drvdata and devres frees data,
leaving the child devices active and causing a null pointer dereference if
accessed from userspace.

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