Re: [PATCH v4 1/3] serdev: handle devices without an ACPI or device tree node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serdev: type confusion and OOB read in serdev_device_match() for controllers
- [Medium] serdev: missing mod_devicetable and file2alias wiring for serdev_device_id
- [Low] serdev: unupdated serdev_device_add() callers break build at this commit
--

commit 1b3d9973e0d0ff0b39b8ad4f9814e44ecd6e06a9
Author: ChunAn Wu <an.wu@canonical.com>

serdev: handle devices without an ACPI or device tree node

This commit adds modalias and id_table support to the serdev bus so that
serdev client devices instantiated without an ACPI companion or device tree
node can emit uevents and match against serdev device drivers. It also moves
struct serdev_device_id into include/linux/device-id/serdev.h.

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc218..45175403bd551 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c

[ ... ]

> @@ -70,11 +85,6 @@ static const struct device_type serdev_device_type = {
>  	.release	= serdev_device_release,
>  };
>  
> -static bool is_serdev_device(const struct device *dev)
> -{
> -	return dev->type == &serdev_device_type;
> -}
> -
>  static void serdev_ctrl_release(struct device *dev)
>  {
>  	struct serdev_controller *ctrl = to_serdev_controller(dev);
> @@ -86,28 +96,51 @@ static const struct device_type serdev_ctrl_type = {
>  	.release	= serdev_ctrl_release,
>  };
>  
> +static const struct serdev_device_id *serdev_match_id(const struct serdev_device_id *id,
> +						      const char *modalias)
> +{
> +	if (!id || !modalias[0])
> +		return NULL;
> +
> +	while (id->name[0]) {
> +		if (!strcmp(modalias, id->name))
> +			return id;
> +		id++;
> +	}
> +
> +	return NULL;
> +}
> +
>  static int serdev_device_match(struct device *dev, const struct device_driver *drv)
>  {
> -	if (!is_serdev_device(dev))
> -		return 0;
> +	const struct serdev_device *serdev = to_serdev_device(dev);
> +	const struct serdev_device_driver *sdrv = to_serdev_device_driver(drv);
>  
> -	/* TODO: platform matching */
>  	if (acpi_driver_match_device(dev, drv))
>  		return 1;
>  
> -	return of_driver_match_device(dev, drv);
> +	if (of_driver_match_device(dev, drv))
> +		return 1;
> +
> +	return !!serdev_match_id(sdrv->id_table, serdev->modalias);
>  }

[Severity: High]
Is it safe to remove the is_serdev_device(dev) check from
serdev_device_match()?

Unlike the SPI bus where controllers are registered on a class rather than
spi_bus_type, serdev registers both struct serdev_controller and struct
serdev_device onto 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));
}

Commit 7ee69102dbc4ee ("serdev: only match serdev devices") added the
is_serdev_device(dev) check so controller devices on serdev_bus_type are
ignored during driver matching.

Without that check, when serdev_device_match() is called with dev pointing to
&ctrl->dev, it first evaluates acpi_driver_match_device() and
of_driver_match_device() against the controller's fwnode, which could match
and probe a struct serdev_controller as a struct serdev_device.

If neither firmware match succeeds, serdev_device_match() casts &ctrl->dev via
to_serdev_device(dev) and passes serdev->modalias to serdev_match_id(). In
struct serdev_device, modalias[SERDEV_NAME_SIZE] starts at offset
sizeof(struct device) + sizeof(int), which in struct serdev_controller aliases
the upper bytes of ctrl->host, followed by ctrl->nr, ctrl->serdev, ctrl->ops,
and extends 12 bytes past the end of struct serdev_controller on 64-bit
platforms. Because the upper bytes of the kernel pointer ctrl->host are
non-zero, modalias[0] is non-zero and strcmp(modalias, id->name) will read out
of bounds past struct serdev_controller when sdrv->id_table is set.

> diff --git a/include/linux/device-id/serdev.h b/include/linux/device-id/serdev.h
> new file mode 100644
> index 0000000000000..96644953ceced
> --- /dev/null
> +++ b/include/linux/device-id/serdev.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LINUX_DEVICE_ID_SERDEV_H
> +#define LINUX_DEVICE_ID_SERDEV_H
> +
> +#ifdef __KERNEL__
> +typedef unsigned long kernel_ulong_t;
> +#endif
> +
> +/* serdev */
> +
> +#define SERDEV_NAME_SIZE	40
> +#define SERDEV_MODULE_PREFIX	"serial:"
> +
> +struct serdev_device_id {
> +	char name[SERDEV_NAME_SIZE];
> +	kernel_ulong_t driver_data;	/* Data private to the driver */
> +};
> +
> +#endif /* ifndef LINUX_DEVICE_ID_SERDEV_H */

[Severity: Medium]
Should struct serdev_device_id also be wired up in
include/linux/mod_devicetable.h, scripts/mod/devicetable-offsets.c, and
scripts/mod/file2alias.c?

serdev_device_uevent() emits MODALIAS=serial:<modalias> to userspace, and this
header follows the include/linux/device-id/ pattern used by modpost. However,
without including device-id/serdev.h in include/linux/mod_devicetable.h and
adding the corresponding DEVID(serdev_device_id) and devtable[] entries in
scripts/mod/, MODULE_DEVICE_TABLE(serdev, ...) is silently ignored by
handle_moddevtable() and no serial:<name> aliases are generated for module
autoloading.

> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index b6c3d957ec155..96a441abc1ace 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h

[ ... ]

> @@ -158,7 +168,7 @@ static inline void serdev_controller_put(struct serdev_controller *ctrl)
>  }
>  
>  struct serdev_device *serdev_device_alloc(struct serdev_controller *);
> -int serdev_device_add(struct serdev_device *);
> +int serdev_device_add(struct serdev_device *serdev, const char *modalias);
>  void serdev_device_remove(struct serdev_device *);

[Severity: Low]
Does changing the signature of serdev_device_add() here break the build for
bisecting before the follow-up patches in the series?

Two callers in drivers/platform/x86/ still pass a single argument at this
commit:

drivers/platform/x86/dell/dell-uart-backlight.c:dell_uart_bl_pdev_probe() {
	...
	ret = serdev_device_add(serdev);
}

drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c:yt2_1380_fc_pdev_probe() {
	...
	ret = serdev_device_add(serdev);
}

Those two callers are updated to pass KBUILD_MODNAME in the subsequent commits
"platform/x86: dell-uart-backlight: Bind through normal serdev matching" and
"platform/x86: lenovo-yoga-tab2-pro-1380-fastcharger: Bind through normal
serdev matching", but at this commit (and the next commit for
yoga-tab2-pro-1380-fastcharger.c) building with CONFIG_DELL_UART_BACKLIGHT or
CONFIG_YOGA_TAB2_PRO_1380_FASTCHARGER enabled fails with too few arguments to
serdev_device_add(). Could those callers be updated to pass NULL in this
commit so every patch in the series builds cleanly?

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