Re: [PATCH v14 1/2] HID: cp2112: Add fwnode support

[Benjamin Tissoires <bentiss@kernel.org>]

On May 20 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Premature release of firmware node references results in use-after-free vulnerabilities when assigning the I2C adapter and GPIO chip firmware nodes.
> --
> 
> commit 3dfaf368e855835bf23009205c4b93a2a070aae1
> Author: Danny Kaehn <danny.kaehn@plexus.com>
> 
> HID: cp2112: Add fwnode support
> 
> This commit adds firmware node support to describe the CP2112's I2C and
> GPIO interfaces. It differentiates between ACPI and DeviceTree bindings,
> mapping the child nodes to the respective adapter and gpio_chip devices.
> 
> > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> > index 803b883ae8750..e960fc988058f 100644
> > --- a/drivers/hid/hid-cp2112.c
> > +++ b/drivers/hid/hid-cp2112.c
> [ ... ]
> > @@ -1226,6 +1241,28 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >  		return ret;
> >  	}
> >  
> > +	cp2112_fwnode = dev_fwnode(&hdev->dev);
> > +	if (is_acpi_device_node(cp2112_fwnode)) {
> > +		fwnode_for_each_child_node(cp2112_fwnode, child) {
> > +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > +			if (ret)
> > +				continue;
> > +
> > +			switch (addr) {
> > +			case CP2112_I2C_ADR:
> > +				device_set_node(&dev->adap.dev, child);
> > +				break;
> > +			case CP2112_GPIO_ADR:
> > +				dev->gc.fwnode = child;
> > +				break;
> > +			}
> 
> [Severity: High]
> Does this code leave dangling pointers in the device structures?
> 
> The fwnode_for_each_child_node() macro automatically drops the reference to
> the child node when advancing to the next node or exiting the loop.
> 
> Because device_set_node() and direct pointer assignments do not increment
> the reference count, the nodes assigned here could be prematurely freed
> resulting in a use-after-free.

Sashiko is overly cautious here. ACPI handles are not refcounted, so we
are fine.

That being said, we could unify the OF and ACPI behaviour by calling the
noops fwnode_handle_get(child) for ACPI as well and clean them up in
cp2112_remove(). This would also shut up the next warning from Sashiko
below.

> 
> > +		}
> > +	} 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);
> 
> [Severity: High]
> Are we releasing the firmware node prematurely here?
> 
> fwnode_get_named_child_node() returns a node with an incremented reference
> count. However, since device_set_node() does not take its own reference on
> the child node, dropping the reference immediately after could leave
> dev->adap.dev with a dangling pointer if the node is later removed.

OF children are tied to the lifespan of the OF device, so we are fine,
but technically incorrect, as mentioned by Sashiko.

If you regroup the 2 behaviours and clean them in cp2112_remove(), this
would be cleaner IMO.

Cheers,
Benjamin

> 
> > +	}
> > +
> >  	ret = hid_parse(hdev);
> >  	if (ret) {
> >  		hid_err(hdev, "parse failed\n");
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260520-cp2112-dt-v14-0-b1b4b6734b6f@plexus.com?part=1
>