Re: [PATCH v14 2/2] HID: cp2112: Configure I2C bus speed from firmware

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The driver silently overwrites custom hardware I2C clock speeds with a 100kHz default for devices without firmware nodes.

Pre-existing issues:
- [High] Firmware nodes are attached to device structures without holding a reference, leading to a dangling pointer and use-after-free.
- [High] The delayed work `gpio_poll_worker` is not canceled in the probe error path, leading to a use-after-free if the probe fails.
--

commit 07ff467e4d8df706a7a27476591cd82570fb8287
Author: Danny Kaehn <danny.kaehn@plexus.com>

    HID: cp2112: Configure I2C bus speed from firmware
    
    Now that the I2C adapter on the CP2112 can have an associated firmware
    node, set the bus speed based on firmware configuration.

> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index e960fc988058f..04379db93571b 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
[ ... ]
> @@ -1308,6 +1309,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err_power_normal;
>  	}
>  
> +	i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this new call dereference
a dangling pointer?

Earlier in cp2112_probe(), the reference to the child firmware node is
dropped either explicitly via fwnode_handle_put(child) in the OF path, or
implicitly when the fwnode_for_each_child_node loop iterates or completes
in the ACPI path:

	} else if (is_of_node(cp2112_fwnode)) {
		child = fwnode_get_named_child_node(cp2112_fwnode, "i2c");
		device_set_node(&dev->adap.dev, child);
		fwnode_handle_put(child);
	}

device_set_node(&dev->adap.dev, child) assigns the pointer without taking
its own reference, leaving dev->adap.dev.fwnode as a dangling pointer.

This newly added i2c_parse_fw_timings() call dereferences this released
fwnode via device_property_read_u32().

Could this lead to a use-after-free?

> +	config.clock_speed = cpu_to_be32(timings.bus_freq_hz);

[Severity: Medium]
Since use_defaults is set to true, does this overwrite custom hardware I2C
clock speeds with a 100kHz default for devices without firmware nodes?

Before this patch, it looks like cp2112_probe() read the current SMBus
configuration from the device, only modified retry_time, and wrote it back.
This preserved the device's original configured clock speed (which might be
set to 400kHz in the EEPROM).

By calling i2c_parse_fw_timings() with an uninitialized local timings
struct and use_defaults = true, timings.bus_freq_hz will be
unconditionally set to 100kHz if the device lacks a firmware property.

Is it intended to silently downgrade the speed and override user-configured
EEPROM settings?

[Severity: High]
This is a pre-existing issue, but is there a missing cancellation of the
delayed work in the probe error path?

If sysfs_create_group() fails during probe, the code jumps to the
err_gpiochip_remove label, which calls gpiochip_remove(&dev->gc) and
returns:

	ret = sysfs_create_group(&hdev->dev.kobj, &cp2112_attr_group);
	if (ret < 0) {
		...
		goto err_gpiochip_remove;
	}
	...
err_gpiochip_remove:
	gpiochip_remove(&dev->gc);
err_free_i2c:

Because gpiochip_add_data() was already called, an IRQ consumer could have
concurrently requested a GPIO line, triggering cp2112_gpio_irq_startup()
and queuing dev->gpio_poll_worker.

Unlike cp2112_remove(), the error path lacks a call to
cancel_delayed_work_sync(&dev->gpio_poll_worker).

Could this mean the queued work will execute on freed memory?

>  	config.retry_time = cpu_to_be16(1);
>  
>  	ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-cp2112-dt-v14-0-b1b4b6734b6f@plexus.com?part=2