Re: [PATCH 1/2] Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The function devices created during probe are leaked and can trigger a Use-After-Free if `rmi_f34_create_sysfs()` fails.
- [High] Failure of `irq_create_mapping()` returns 0, which is unchecked and passed directly to IRQ manipulation functions, hijacking or corrupting system IRQ 0.
- [Medium] The IRQ domain created during probe is leaked on almost all error paths in `rmi_driver_probe()`.
--

--- Patch [1]: [PATCH 1/2] Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources ---
commit 048bbd16c16ceb54800fa66f025c5162730cc787
Author: Wei Jie Law <98lawweijie@gmail.com>

Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources

This commit fixes an out-of-bounds array access by expanding the irq array
size from 6 to 7 to match the maximum interrupt source count specified by the
RMI4 Page Description Table entry.

> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index 90122df21f746..faf2ebb00d521 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -12,10 +12,13 @@
>  struct rmi_device;
>  
>  /*
> - * The interrupt source count in the function descriptor can represent up to
> - * 6 interrupt sources in the normal manner.
> + * The interrupt source count in the function descriptor is a three bit field
> + * (RMI_PDT_INT_SOURCE_COUNT_MASK), so a device can legitimately declare up to
> + * 7 interrupt sources for a single function.  irq[] must be able to hold all
> + * of them: rmi_create_function_irq() and rmi_unregister_function() both walk
> + * it up to fn->num_of_irqs.
>   */
> -#define RMI_FN_MAX_IRQS	6
> +#define RMI_FN_MAX_IRQS	7
>  
>  /**
>   * struct rmi_function - represents the implementation of an RMI4

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free and memory leak in the error handling of rmi_driver_probe():

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
    ...
	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 to the err label, skipping
the call to rmi_free_function_list(rmi_dev). Doesn't this leave the function
devices orphaned on the RMI bus with a soon-to-be-freed parent drvdata,
leading to a use-after-free if a driver later attempts to bind to them?

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed the IRQ domain might
be leaking on probe error 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;
}

The rmi_probe_interrupts() function allocates a linear IRQ domain using
irq_domain_create_linear(), which is not managed by devres. If any subsequent
initialization step fails, the probe function returns without calling
irq_domain_remove(). Is it expected that the IRQ domain is leaked here?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how mapped IRQs are handled:

drivers/input/rmi4/rmi_bus.c:rmi_create_function_irq() {
    ...
		fn->irq[i] = irq_create_mapping(drvdata->irqdomain,
						fn->irq_pos + i);

		irq_set_chip_data(fn->irq[i], fn);
		irq_set_chip_and_handler(fn->irq[i], &rmi_irq_chip,
					 handle_simple_irq);
    ...
}

If irq_create_mapping() fails due to memory or descriptor exhaustion, it
returns 0. Is it safe to pass 0 directly to irq_set_chip_data() and
irq_set_chip_and_handler() without checking the return value? Could this
overwrite the chip and handler for system IRQ 0?

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