Re: [PATCH 1/2] Input: synaptics-rmi4 - bound the F3A keymap to the GPIO count

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds shift and logic error in `rmi_f3a_report_button` when `button >= 8`.
- [High] Unbinding the `rmi4_f3a` driver leaves a dangling `input->keycode` pointer, leading to a Use-After-Free.
- [High] Failing to insert into the attention FIFO leaks the dynamically allocated duplicate buffer.
- [Medium] Multiple RMI4 functions concurrently overwrite the shared `input->keycode` map, making all but the last probed map inaccessible to userspace.
--

commit 1556a27ea7ad4e1c54736060ac2c334003a11681
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: synaptics-rmi4 - bound the F3A keymap to the GPIO count

This commit addresses an out-of-bounds access in the F3A keymap. It sizes the
gpio_key_map buffer to the full gpio_count rather than capping it at 6,
preventing reads or writes past the allocated array during attention
interrupts or keymap ioctls.

> diff --git a/drivers/input/rmi4/rmi_f3a.c b/drivers/input/rmi4/rmi_f3a.c
> index 0e8baed84dbb5..a0777644eef07 100644
> --- a/drivers/input/rmi4/rmi_f3a.c
> +++ b/drivers/input/rmi4/rmi_f3a.c
> @@ -132,7 +132,7 @@ static int rmi_f3a_map_gpios(struct rmi_function *fn, struct f3a_data *f3a,
>  	int button_count = min_t(u8, f3a->gpio_count, TRACKSTICK_RANGE_END);
>  
>  	f3a->gpio_key_map = devm_kcalloc(&fn->dev,
> -						button_count,
> +						f3a->gpio_count,
>  						sizeof(f3a->gpio_key_map[0]),
>  						GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
lifecycle of the keymap allocated here.

Since input->keycode is set to f3a->gpio_key_map later in this function, if
the rmi4_f3a function driver is unbound via sysfs, devres will free this
memory. Because rmi_f3a_handler lacks a .remove callback and does not set
.suppress_bind_attrs = true, does this leave input->keycode pointing to freed
memory? Could subsequent EVIOCGKEYCODE or EVIOCSKEYCODE ioctls on the shared
input_dev dereference this freed pointer?

[Severity: Medium]
This is a pre-existing issue, but I also noticed how this keymap interacts
with the shared input device.

Both F3A and F30 assign their own private arrays to the shared input->keycode:

drivers/input/rmi4/rmi_f3a.c:rmi_f3a_map_gpios() {
    ...
    input->keycode = f3a->gpio_key_map;
    ...
}

drivers/input/rmi4/rmi_f30.c:rmi_f30_map_gpios() {
    ...
    input->keycode = f30->gpioled_key_map;
    ...
}

Since multiple RMI4 functions concurrently overwrite the shared
input->keycode, does this make all but the last probed map inaccessible to
userspace? Would earlier functions then report events using their
unmodifiable, private arrays?

>  	if (!f3a->gpio_key_map) {

[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at rmi_f3a_report_button() in
drivers/input/rmi4/rmi_f3a.c:

drivers/input/rmi4/rmi_f3a.c:rmi_f3a_report_button() {
    ...
    bool key_down = !(f3a->data_regs[0] & BIT(button));
    ...
}

Since f3a->data_regs is a u8 array, could this incorrectly report the key as
constantly pressed if button >= 8? If button >= 32, could this result in an
out-of-bounds shift? Since userspace can map a high button index via
EVIOCSKEYCODE and trigger an interrupt, could this be triggered intentionally?

[Severity: High]
This is a pre-existing issue, but looking at rmi_set_attn_data() in
drivers/input/rmi4/rmi_driver.c:

drivers/input/rmi4/rmi_driver.c:rmi_set_attn_data() {
    ...
    fifo_data = kmemdup(data, size, GFP_ATOMIC);
    ...
    attn_data.data = fifo_data;
    ...
    kfifo_put(&drvdata->attn_fifo, attn_data);
    ...
}

If kfifo_put() fails because the FIFO is full, does this silently leak the
dynamically allocated fifo_data buffer? Could a flood of interrupts lead to
memory exhaustion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-disp-818d6bda-v1-0-cf39a3615085@proton.me?part=1