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

[Benjamin Tissoires <bentiss@kernel.org>]

On May 20 2026, sashiko-bot@kernel.org wrote:
> 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?

Cleaning up the refcounting in the previous patch would solve this
(false positive, admitedly).

> 
> 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?

I think we should fix this:
i2c_parse_fw_timings(&dev->adap.dev, &timings, false);
if (timings.bus_freq_hz)
    config.clock_speed = cpu_to_be32(timings.bus_freq_hz);

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

This one would be nice to fix in a follow up patch.

> 
> 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
> 

Cheers,
Benjamin