Re: [PATCH v4 2/3] platform/x86: dell-uart-backlight: Bind through normal serdev matching

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] platform/x86: dell-uart-backlight: type confusion in serdev_device_match()
--

commit 845cea88ea8fd97f6d009ac4a558f36871acb863
Author: ChunAn Wu <an.wu@canonical.com>

platform/x86: dell-uart-backlight: Bind through normal serdev matching

This commit switches the dell-uart-backlight driver from manually binding
its serdev client via device_driver_attach() to passing a modalias to
serdev_device_add() and matching via a serdev_device_id table.

> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> index f323a667dc2d2..9b8ef91bb0a20 100644
> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> @@ -325,11 +325,17 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev)
>  	return PTR_ERR_OR_ZERO(dell_bl->bl);
>  }
>  
> +static const struct serdev_device_id dell_uart_bl_serdev_id[] = {
> +	{ KBUILD_MODNAME },
> +	{ }
> +};
> +
>  static struct serdev_device_driver dell_uart_bl_serdev_driver = {
>  	.probe = dell_uart_bl_serdev_probe,
>  	.driver = {
>  		.name = KBUILD_MODNAME,
>  	},
> +	.id_table = dell_uart_bl_serdev_id,
>  };

[Severity: High]
Does adding id_table to dell_uart_bl_serdev_driver trigger a type confusion
and out-of-bounds read in serdev_device_match() when
dell_uart_bl_pdev_probe() calls serdev_device_driver_register()?

Both struct serdev_controller and struct serdev_device are registered on
serdev_bus_type:

drivers/tty/serdev/core.c:serdev_controller_alloc() {
	...
	device_initialize(&ctrl->dev);
	ctrl->dev.type = &serdev_ctrl_type;
	ctrl->dev.bus = &serdev_bus_type;
	ctrl->dev.parent = parent;
	ctrl->host = host;
	device_set_node(&ctrl->dev, dev_fwnode(host));
	...
}

Earlier in the series, commit 1b3d9973e0d0 ("serdev: handle devices without
an ACPI or device tree node") removed the is_serdev_device(dev) check from
serdev_device_match(), leaving an unconditional cast via to_serdev_device():

drivers/tty/serdev/core.c:serdev_device_match() {
	const struct serdev_device *serdev = to_serdev_device(dev);
	const struct serdev_device_driver *sdrv = to_serdev_device_driver(drv);

	if (acpi_driver_match_device(dev, drv))
		return 1;

	if (of_driver_match_device(dev, drv))
		return 1;

	return !!serdev_match_id(sdrv->id_table, serdev->modalias);
}

When dell_uart_bl_pdev_probe() calls:

  serdev_device_driver_register(&dell_uart_bl_serdev_driver)
    -> driver_register()
      -> bus_add_driver()
        -> driver_attach()
          -> serdev_device_match()

the driver core iterates over every device on serdev_bus_type, including
the struct serdev_controller instance ("serial0").

Because dell_uart_bl_serdev_driver has no ACPI or OF match table and sets a
non-NULL id_table, serdev_device_match() reaches
serdev_match_id(sdrv->id_table, serdev->modalias) with serdev pointing to a
struct serdev_controller.

In struct serdev_device, modalias is at offset sizeof(struct device) + 4
and is 40 bytes long. In struct serdev_controller, that offset overlaps
struct device *host, unsigned int nr, struct serdev_device *serdev,
const struct serdev_controller_ops *ops, and extends 12 bytes past the end
of struct serdev_controller, passing those bytes into strcmp(modalias,
id->name) in serdev_match_id().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024022930.2704030-1-an.wu@canonical.com?part=2