Re: [PATCH v24 02/12] power: reset: reboot-mode: Add support for predefined reboot modes

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Duplicate predefined reboot modes are not properly deduplicated against DT-defined modes, violating precedence rules and causing duplicate sysfs entries.
- [Low] The patch silently changes how device tree arrays are parsed, skipping valid properties with more than 3 elements.

Pre-existing issues:
- [High] Memory leak of `struct device` in `reboot_mode_unregister_device` due to a missing `put_device()`.
- [High] Multiple instances of the same reboot-mode driver cause sysfs name collisions and incorrect device unregistration, leading to a Use-After-Free.
--

--- Patch [2]: [PATCH v24 02/12] power: reset: reboot-mode: Add support for predefined reboot modes ---
commit a1dab4a1d45dc9572ca3952f97f12675bbb17e32
Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

power: reset: reboot-mode: Add support for predefined reboot modes

reboot-mode based drivers can define a reboot-mode by adding it under
the reboot-mode node in device tree. This limits such drivers, to define
any predefined reboot-modes statically within the driver and creates a
dependency on device-tree.

Extend the reboot-mode framework to support driver-defined predefined
reboot modes. Add a centralized initcall to initialize driver state
along with predefined modes.

> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index c7e9285c64e92..cf8978c0ce222 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
[ ... ]
> @@ -160,11 +234,38 @@ static int reboot_mode_create_device(struct reboot_mode_driver *reboot)
[ ... ]
> +static int reboot_mode_add_predefined_modes(struct reboot_mode_driver *reboot)
> +{
> +	const struct reboot_mode_entry *modes = reboot->predefined_modes;
> +	struct mode_info *info;
> +	size_t i;
> +
> +	if (!modes)
> +		return 0;
> +
> +	for (i = 0; i < reboot->predefined_mode_count; i++) {
> +		if (modes[i].name && strpbrk(modes[i].name, "\n ,/"))
> +			return -EINVAL;
> +
> +		if (!modes[i].count || modes[i].count > ARRAY_SIZE(modes[i].magic))
> +			return -EINVAL;
> +
> +		info = reboot_mode_create_info(reboot->dev, modes[i].name,
> +					       modes[i].magic, modes[i].count);
> +		if (IS_ERR(info))
> +			return PTR_ERR(info);
> +
> +		list_add_tail(&info->list, &reboot->head);

[Severity: Low]
Does this code append predefined modes to the list unconditionally without
checking if a mode with the same name was already populated from the DT?

If a predefined mode has the same name as a mode from the Device Tree, both
entries will be added to the list. When reboot_mode_create_device() iterates
the list, it will create duplicate sysfs entries. Should we check for existing
modes and deduplicate them to honor the DT precedence rules mentioned in the
commit message?

> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -183,52 +284,36 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>  
>  	INIT_LIST_HEAD(&reboot->head);
>  
> -	for_each_property_of_node(np, prop) {
> -		memset(magic, 0, sizeof(magic));
[ ... ]
> +	if (np) {
> +		for_each_property_of_node(np, prop) {
> +			memset(magic, 0, sizeof(magic));
> +			if (strncmp(prop->name, PREFIX, len))
> +				continue;
> +
> +			count = device_property_count_u32(reboot->dev, prop->name);
> +
> +			if (count <= 0 || count > ARRAY_SIZE(magic) ||
> +			    device_property_read_u32_array(reboot->dev, prop->name, magic, count)) {

[Severity: Low]
Does this condition strictly reject properties with more than 3 elements
instead of reading up to the maximum supported count?

If a DT configuration defines a reboot mode property with more than 3 items,
it appears this would skip the valid properties entirely. Previously, would
device_property_read_u32_array() have successfully read the first element
and functioned correctly while ignoring extra elements?

> +				dev_dbg(reboot->dev, "reboot mode %s without magic number\n",
> +					prop->name);
> +				continue;
> +			}
[ ... ]
> @@ -272,7 +356,7 @@ static inline void reboot_mode_unregister_device(struct reboot_mode_driver *rebo
>  	if (!priv)
>  		return;
>  
> -	reboot_mode_release_list(priv);
> +	reboot_mode_release_list(&priv->head);
>  	kfree(priv);

[Severity: High]
This is a pre-existing issue, but does reboot_mode_unregister_device() leak a
struct device here?

When finding the device, class_find_device() internally increments the
reference count of the returned device:

reboot_mode_unregister_device() {
    reboot_mode_device = class_find_device(&reboot_mode_class, NULL,
                                           reboot->dev->driver->name,
                                           reboot_mode_match_by_name);
    ...
    device_unregister(reboot_mode_device);
}

However, device_unregister() only drops the reference count incremented during
device registration. It does not drop the extra reference acquired by
class_find_device(). Could this lead to a memory leak every time a driver is
unregistered or fails to probe?


[Severity: High]
This is a pre-existing issue, but can having multiple instances of the same
reboot-mode driver cause sysfs name collisions and a Use-After-Free?

In reboot_mode_create_device(), the sysfs device is uniquely named after the
driver name:

reboot_mode_create_device() {
    priv->reboot_mode_device = device_create(&reboot_mode_class, NULL, 0,
                                             (void *)priv, "%s",
                                             reboot->dev->driver->name);
}

If multiple instances of the driver exist (e.g., "qcom-pon"), this guarantees
a naming collision. The second instance will fail to probe.

During the error path, reboot_mode_unregister_device() matches the device
purely by the shared driver name:

reboot_mode_unregister_device() {
    reboot_mode_device = class_find_device(&reboot_mode_class, NULL,
                                           reboot->dev->driver->name,
                                           reboot_mode_match_by_name);
}

This incorrectly finds and unregisters the first successfully initialized
instance's device instead of the failed instance's device, and frees its
private data. Subsequent sysfs operations on the first instance could then
trigger a Use-After-Free.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-arm-psci-system_reset2-vendor-reboots-v24-0-889281373870@oss.qualcomm.com?part=2