[PATCH 07/21] leds: trigger: netdev: Serialise work cancellation and state updates

[Lee Jones <lee@kernel.org>]

set_device_name() and netdev_trig_notify() cancel trigger_data->work
before acquiring trigger_data->lock, whilst netdev_led_attr_store() and
interval_store() cancel trigger_data->work, update trigger_data state,
and call set_baseline_state() without holding trigger_data->lock at all.

Because set_baseline_state() can re-arm trigger_data->work outside the
lock, a concurrent notifier or sysfs store can re-schedule
netdev_trig_work() after cancel_delayed_work_sync() has returned and
whilst set_device_name() or netdev_trig_notify() is replacing or
dropping trigger_data->net_dev, leading to a use-after-free in
netdev_trig_work().

Move cancel_delayed_work_sync() inside trigger_data->lock in
set_device_name() and netdev_trig_notify(), and hold trigger_data->lock
across work cancellation, state updates, and set_baseline_state() in
netdev_led_attr_store() and interval_store().

Fixes: 06f502f57d0d ("leds: trigger: Introduce a NETDEV trigger")
Closes: https://sashiko.dev/#/bug/linux-b2b6c3bb-8017-4623-94d4-2999efa6a663
Cc: stable@vger.kernel.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 35 +++++++++++++++++++--------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d69031bbd2b8..9fa56ac523c7 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -284,8 +284,6 @@ static int set_device_name(struct led_netdev_data *trigger_data,
 	if (size >= IFNAMSIZ)
 		return -EINVAL;
 
-	cancel_delayed_work_sync(&trigger_data->work);
-
 	memcpy(device_name, name, size);
 	device_name[size] = 0;
 	if (size > 0 && device_name[size - 1] == '\n')
@@ -301,6 +299,8 @@ static int set_device_name(struct led_netdev_data *trigger_data,
 	}
 	mutex_lock(&trigger_data->lock);
 
+	cancel_delayed_work_sync(&trigger_data->work);
+
 	dev_put(trigger_data->net_dev);
 	trigger_data->net_dev = new_dev;
 	strscpy(trigger_data->device_name, device_name);
@@ -384,7 +384,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
-	unsigned long state, mode = trigger_data->mode;
+	unsigned long state, mode;
 	int ret;
 	int bit;
 
@@ -416,6 +416,9 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return -EINVAL;
 	}
 
+	mutex_lock(&trigger_data->lock);
+
+	mode = trigger_data->mode;
 	assign_bit(bit, &mode, state);
 
 	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
@@ -428,8 +431,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	     test_bit(TRIGGER_NETDEV_LINK_25000, &mode) ||
 	     test_bit(TRIGGER_NETDEV_LINK_40000, &mode) ||
 	     test_bit(TRIGGER_NETDEV_LINK_50000, &mode) ||
-	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode)))
+	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode))) {
+		mutex_unlock(&trigger_data->lock);
 		return -EINVAL;
+	}
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
@@ -437,11 +442,15 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	trigger_data->hw_control = can_hw_control(trigger_data);
 
 	if (!led_cdev->brightness_set && !led_cdev->brightness_set_blocking &&
-	    !trigger_data->hw_control)
+	    !trigger_data->hw_control) {
+		mutex_unlock(&trigger_data->lock);
 		return -EOPNOTSUPP;
+	}
 
 	set_baseline_state(trigger_data);
 
+	mutex_unlock(&trigger_data->lock);
+
 	return size;
 }
 
@@ -493,13 +502,17 @@ static ssize_t interval_store(struct device *dev,
 	unsigned long value;
 	int ret;
 
-	if (trigger_data->hw_control)
-		return -EINVAL;
-
 	ret = kstrtoul(buf, 0, &value);
 	if (ret)
 		return ret;
 
+	mutex_lock(&trigger_data->lock);
+
+	if (trigger_data->hw_control) {
+		mutex_unlock(&trigger_data->lock);
+		return -EINVAL;
+	}
+
 	/* impose some basic bounds on the timer interval */
 	if (value >= 5 && value <= 10000) {
 		cancel_delayed_work_sync(&trigger_data->work);
@@ -508,6 +521,8 @@ static ssize_t interval_store(struct device *dev,
 		set_baseline_state(trigger_data);	/* resets timer */
 	}
 
+	mutex_unlock(&trigger_data->lock);
+
 	return size;
 }
 
@@ -628,10 +643,10 @@ static int netdev_trig_notify(struct notifier_block *nb,
 	      (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
 		return NOTIFY_DONE;
 
-	cancel_delayed_work_sync(&trigger_data->work);
-
 	mutex_lock(&trigger_data->lock);
 
+	cancel_delayed_work_sync(&trigger_data->work);
+
 	trigger_data->carrier_link_up = false;
 	trigger_data->link_speed = SPEED_UNKNOWN;
 	trigger_data->duplex = DUPLEX_UNKNOWN;
-- 
2.56.0.218.g1ec904b173-goog