Re: [PATCH 3/3] leds: trigger: netdev: serialize mode/interval stores with trigger lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unprotected cancel_delayed_work_sync() allows concurrent execution and UAF of net_device in the worker thread.
--

commit d9c1377958eda5198cea4d7f716e117467e638fb
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Subject: leds: trigger: netdev: serialize mode/interval stores with trigger lock

This patch updates netdev_led_attr_store() and interval_store() to hold
the trigger_data->lock. It prevents concurrent writes from dropping updates,
reading partially-updated device states, and causing data corruption on
shared fields.

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 89b48c522c227..6556f6d28547f 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -421,6 +421,16 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Serialize the read-modify-write of ->mode and the dependent
> +	 * ->hw_control update and set_baseline_state() against concurrent
> +	 * attribute stores and netdev_trig_notify().  netdev_trig_work() must
> +	 * never take this lock, otherwise the cancel_delayed_work_sync() below
> +	 * would deadlock.
> +	 */
> +	guard(mutex)(&trigger_data->lock);
> +
> +	mode = trigger_data->mode;

[Severity: Critical]
This is a pre-existing issue, but since this patch discusses lock interactions
with cancel_delayed_work_sync(), does the unprotected call to
cancel_delayed_work_sync() in set_device_name() allow a concurrent thread
to re-arm the work before the old device reference is put?

A sysfs write to device_name triggers set_device_name(), which calls
cancel_delayed_work_sync() locklessly:

drivers/leds/trigger/ledtrig-netdev.c:set_device_name() {
    ...
    cancel_delayed_work_sync(&trigger_data->work);
    ...
    mutex_lock(&trigger_data->lock);

    dev_put(trigger_data->net_dev);
    ...
}

Concurrently, a sysfs write to a mode attribute (e.g., here in
netdev_led_attr_store()) acquires trigger_data->lock and calls
set_baseline_state(), re-arming the workqueue. The delayed work runs
netdev_trig_work() locklessly, accessing trigger_data->net_dev:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_work() {
    ...
    dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
    ...
}

Meanwhile, set_device_name() acquires the lock, assigns a new device, and
calls dev_put() on the old device. Since netdev_trig_work() doesn't hold its
own reference or any locks, this dev_put() can drop the last reference and free
the net_device while the work thread is actively calling dev_get_stats() on it.

A similar race exists in netdev_trig_notify() on the NETDEV_UNREGISTER event.

Can this sequence lead to a Use-After-Free (UAF) of the net_device structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914132356.807004-1-alexander.sverdlin@siemens.com?part=3