[PATCH v4 01/19] media: rc: Ensure registered is cleared in error path

[PATCH v4 01/19] media: rc: Ensure registered is cleared in error path

[Sean Young]

If rc_register_device() fails, ensure that registered is not set to true.
If lirc_register() succeeded, then userspace could have an open file
descriptor open. This leads to a use-after-free.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/rc-main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index d93e98189c1a..1f99540456f1 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1934,7 +1934,8 @@ int rc_register_device(struct rc_dev *dev)
 			goto out_raw;
 	}
 
-	dev->registered = true;
+	scoped_guard(mutex, &dev->lock)
+		dev->registered = true;
 
 	rc = device_add(&dev->dev);
 	if (rc)
@@ -1982,6 +1983,8 @@ int rc_register_device(struct rc_dev *dev)
 out_dev:
 	device_del(&dev->dev);
 out_rx_free:
+	scoped_guard(mutex, &dev->lock)
+		dev->registered = false;
 	ir_free_table(&dev->rc_map);
 out_raw:
 	ida_free(&rc_ida, minor);
-- 
2.55.0

[PATCH v4 02/19] media: rc: Ensure that rc_unregister_device() does not free input device

[Sean Young]

During or after rc_unregister_device(), IR may still be reported which
results in a input event being reported. This could result in a null
pointer deref in rc_keydown() or a use-after-free of the input device if
the pointer was read before it is set to NULL.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/rc-main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 1f99540456f1..88d8f7d4aab3 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1753,7 +1753,7 @@ void rc_free_device(struct rc_dev *dev)
 	if (!dev)
 		return;
 
-	input_free_device(dev->input_dev);
+	input_put_device(dev->input_dev);
 
 	put_device(&dev->dev);
 
@@ -1865,6 +1865,8 @@ static int rc_setup_rx_device(struct rc_dev *dev)
 	if (rc)
 		return rc;
 
+	input_get_device(dev->input_dev);
+
 	/*
 	 * Default delay of 250ms is too short for some protocols, especially
 	 * since the timeout is currently set to 250ms. Increase it to 500ms,
@@ -1891,10 +1893,8 @@ static void rc_free_rx_device(struct rc_dev *dev)
 	if (!dev)
 		return;
 
-	if (dev->input_dev) {
+	if (dev->input_dev)
 		input_unregister_device(dev->input_dev);
-		dev->input_dev = NULL;
-	}
 
 	ir_free_table(&dev->rc_map);
 }
-- 
2.55.0

[PATCH v4 03/19] media: rc: Fix ABBA deadlock by making locks more fine grained

[Sean Young]

The ir_raw_event_unregister() function takes the ir_raw_handler_lock
first and then the rc_dev->lock. Other functions like change_protocol
do this in the reverse order.

This means that an ir decoder module unload and writing to the protocol
sysfs file can cause a deadlock.

ir_raw_handler_lock protects the client list, amongst other things.
Split this out into a separate lock so we can ensure the locking
order is always the same.

Fixes: 93cffffc18f6 ("[media] media: rc: fix decoder module unloading")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/rc-ir-raw.c | 39 ++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 54b323becb1f..75dee4a483d4 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -10,7 +10,8 @@
 #include <linux/sched.h>
 #include "rc-core-priv.h"
 
-/* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
+/* Used to keep track of IR raw clients, protected by ir_raw_client_lock */
+static DEFINE_MUTEX(ir_raw_client_lock);
 static LIST_HEAD(ir_raw_client_list);
 
 /* Used to handle IR raw handler extensions */
@@ -273,13 +274,6 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_proto)
 	return 0;
 }
 
-static void ir_raw_disable_protocols(struct rc_dev *dev, u64 protocols)
-{
-	mutex_lock(&dev->lock);
-	dev->enabled_protocols &= ~protocols;
-	mutex_unlock(&dev->lock);
-}
-
 /**
  * ir_raw_gen_manchester() - Encode data with Manchester (bi-phase) modulation.
  * @ev:		Pointer to pointer to next free event. *@ev is incremented for
@@ -615,15 +609,18 @@ int ir_raw_event_register(struct rc_dev *dev)
 {
 	struct task_struct *thread;
 
+	/* Holding dev->lock could result in a dead-lock */
+	lockdep_assert_not_held(&dev->lock);
+
 	thread = kthread_run(ir_raw_event_thread, dev->raw, "rc%u", dev->minor);
 	if (IS_ERR(thread))
 		return PTR_ERR(thread);
 
 	dev->raw->thread = thread;
 
-	mutex_lock(&ir_raw_handler_lock);
+	mutex_lock(&ir_raw_client_lock);
 	list_add_tail(&dev->raw->list, &ir_raw_client_list);
-	mutex_unlock(&ir_raw_handler_lock);
+	mutex_unlock(&ir_raw_client_lock);
 
 	return 0;
 }
@@ -656,16 +653,19 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 	kthread_stop(dev->raw->thread);
 	timer_delete_sync(&dev->raw->edge_handle);
 
-	mutex_lock(&ir_raw_handler_lock);
+	mutex_lock(&ir_raw_client_lock);
 	list_del(&dev->raw->list);
+
+	mutex_lock(&ir_raw_handler_lock);
 	list_for_each_entry(handler, &ir_raw_handler_list, list)
 		if (handler->raw_unregister &&
 		    (handler->protocols & dev->enabled_protocols))
 			handler->raw_unregister(dev);
 
 	lirc_bpf_free(dev);
-
 	mutex_unlock(&ir_raw_handler_lock);
+
+	mutex_unlock(&ir_raw_client_lock);
 }
 
 /*
@@ -688,15 +688,24 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
 	struct ir_raw_event_ctrl *raw;
 	u64 protocols = ir_raw_handler->protocols;
 
+	mutex_lock(&ir_raw_client_lock);
+
 	mutex_lock(&ir_raw_handler_lock);
 	list_del(&ir_raw_handler->list);
+	atomic64_andnot(protocols, &available_protocols);
+	mutex_unlock(&ir_raw_handler_lock);
+
 	list_for_each_entry(raw, &ir_raw_client_list, list) {
+		mutex_lock(&raw->dev->lock);
+		mutex_lock(&ir_raw_handler_lock);
 		if (ir_raw_handler->raw_unregister &&
 		    (raw->dev->enabled_protocols & protocols))
 			ir_raw_handler->raw_unregister(raw->dev);
-		ir_raw_disable_protocols(raw->dev, protocols);
+		raw->dev->enabled_protocols &= ~protocols;
+		mutex_unlock(&ir_raw_handler_lock);
+		mutex_unlock(&raw->dev->lock);
 	}
-	atomic64_andnot(protocols, &available_protocols);
-	mutex_unlock(&ir_raw_handler_lock);
+
+	mutex_unlock(&ir_raw_client_lock);
 }
 EXPORT_SYMBOL(ir_raw_handler_unregister);
-- 
2.55.0

[PATCH v4 04/19] media: rc: Add missing locking for keymap

[Sean Young]

When the rc_map for an rc_dev gets updated, locking is required but is
missing in places. A concurrent scancode lookup and a call to
rc_{register,unregistered}_device() could result in a use-after-free;
this could happen if IR is decoded during those function calls.

We also fix some ugliness like open-coded krealloc() and removing the
pointless alloc member of rc_map.

Add lockdep assertions where locks are required.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/rc-main.c | 122 ++++++++++++++++++++++---------------
 include/media/rc-map.h     |   2 -
 2 files changed, 74 insertions(+), 50 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 88d8f7d4aab3..04204559959f 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -17,9 +17,8 @@
 #include <linux/module.h>
 #include "rc-core-priv.h"
 
-/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */
-#define IR_TAB_MIN_SIZE	256
-#define IR_TAB_MAX_SIZE	8192
+#define IR_TAB_MIN_SIZE	32
+#define IR_TAB_MAX_SIZE	1024
 
 static const struct {
 	const char *name;
@@ -105,7 +104,6 @@ static struct rc_map_list *seek_rc_map(const char *name)
 
 struct rc_map *rc_map_get(const char *name)
 {
-
 	struct rc_map_list *map;
 
 	map = seek_rc_map(name);
@@ -202,7 +200,7 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
  * ir_create_table() - initializes a scancode table
  * @dev:	the rc_dev device
  * @rc_map:	the rc_map to initialize
- * @name:	name to assign to the table
+ * @map_name:	name to assign to the table
  * @rc_proto:	ir type to assign to the new table
  * @size:	initial size of the table
  *
@@ -212,23 +210,33 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
  * return:	zero on success or a negative error code
  */
 static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
-			   const char *name, u64 rc_proto, size_t size)
+			   const char *map_name, u64 rc_proto, size_t size)
 {
-	rc_map->name = kstrdup(name, GFP_KERNEL);
-	if (!rc_map->name)
+	struct rc_map_table *scan;
+	unsigned int alloc;
+	char *name;
+
+	name = kstrdup(map_name, GFP_KERNEL);
+	if (!name)
 		return -ENOMEM;
-	rc_map->rc_proto = rc_proto;
-	rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
-	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
-	rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
-	if (!rc_map->scan) {
-		kfree(rc_map->name);
-		rc_map->name = NULL;
+
+	alloc = roundup_pow_of_two(size);
+	scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);
+	if (!scan) {
+		kfree(name);
 		return -ENOMEM;
 	}
 
-	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%u bytes)\n",
-		rc_map->size, rc_map->alloc);
+	scoped_guard(spinlock_irqsave, &rc_map->lock) {
+		rc_map->name = name;
+		rc_map->scan = scan;
+		rc_map->rc_proto = rc_proto;
+		rc_map->len = 0;
+		rc_map->size = alloc;
+	}
+
+	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%zu bytes)\n",
+		alloc, alloc * sizeof(struct rc_map_table));
 	return 0;
 }
 
@@ -236,16 +244,26 @@ static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
  * ir_free_table() - frees memory allocated by a scancode table
  * @rc_map:	the table whose mappings need to be freed
  *
- * This routine will free memory alloctaed for key mappings used by given
+ * This routine will free memory allocated for key mappings used by given
  * scancode table.
  */
 static void ir_free_table(struct rc_map *rc_map)
 {
-	rc_map->size = 0;
-	kfree(rc_map->name);
-	rc_map->name = NULL;
-	kfree(rc_map->scan);
-	rc_map->scan = NULL;
+	struct rc_map_table *scan;
+	const char *name;
+
+	scoped_guard(spinlock_irqsave, &rc_map->lock) {
+		name = rc_map->name;
+		scan = rc_map->scan;
+
+		rc_map->size = 0;
+		rc_map->len = 0;
+		rc_map->name = NULL;
+		rc_map->scan = NULL;
+	}
+
+	kfree(name);
+	kfree(scan);
 }
 
 /**
@@ -262,38 +280,38 @@ static void ir_free_table(struct rc_map *rc_map)
 static int ir_resize_table(struct rc_dev *dev, struct rc_map *rc_map,
 			   gfp_t gfp_flags)
 {
-	unsigned int oldalloc = rc_map->alloc;
-	unsigned int newalloc = oldalloc;
-	struct rc_map_table *oldscan = rc_map->scan;
+	unsigned int newsize = rc_map->size;
 	struct rc_map_table *newscan;
 
+	lockdep_assert_held(&rc_map->lock);
+
 	if (rc_map->size == rc_map->len) {
 		/* All entries in use -> grow keytable */
-		if (rc_map->alloc >= IR_TAB_MAX_SIZE)
+		if (newsize >= IR_TAB_MAX_SIZE)
 			return -ENOMEM;
 
-		newalloc *= 2;
-		dev_dbg(&dev->dev, "Growing table to %u bytes\n", newalloc);
+		newsize *= 2;
+
+		dev_dbg(&dev->dev, "Growing table to %u entries\n", newsize);
 	}
 
-	if ((rc_map->len * 3 < rc_map->size) && (oldalloc > IR_TAB_MIN_SIZE)) {
+	if (rc_map->len * 3 < rc_map->size && rc_map->size > IR_TAB_MIN_SIZE) {
 		/* Less than 1/3 of entries in use -> shrink keytable */
-		newalloc /= 2;
-		dev_dbg(&dev->dev, "Shrinking table to %u bytes\n", newalloc);
+		newsize /= 2;
+		dev_dbg(&dev->dev, "Shrinking table to %u entries\n", newsize);
 	}
 
-	if (newalloc == oldalloc)
+	if (newsize == rc_map->size)
 		return 0;
 
-	newscan = kmalloc(newalloc, gfp_flags);
+	newscan = krealloc_array(rc_map->scan, newsize,
+				 sizeof(struct rc_map_table), gfp_flags);
 	if (!newscan)
 		return -ENOMEM;
 
-	memcpy(newscan, rc_map->scan, rc_map->len * sizeof(struct rc_map_table));
 	rc_map->scan = newscan;
-	rc_map->alloc = newalloc;
-	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
-	kfree(oldscan);
+	rc_map->size = newsize;
+
 	return 0;
 }
 
@@ -318,6 +336,8 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
 	int old_keycode = rc_map->scan[index].keycode;
 	int i;
 
+	lockdep_assert_held(&rc_map->lock);
+
 	/* Did the user wish to remove the mapping? */
 	if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
 		dev_dbg(&dev->dev, "#%d: Deleting scan 0x%04llx\n",
@@ -373,6 +393,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
 {
 	unsigned int i;
 
+	lockdep_assert_held(&rc_map->lock);
+
 	/*
 	 * Unfortunately, some hardware-based IR decoders don't provide
 	 * all bits for the complete IR code. In general, they provide only
@@ -397,7 +419,7 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
 	/* No previous mapping found, we might need to grow the table */
 	if (rc_map->size == rc_map->len) {
 		if (!resize || ir_resize_table(dev, rc_map, GFP_ATOMIC))
-			return -1U;
+			return UINT_MAX;
 	}
 
 	/* i is the proper index to insert our new keycode */
@@ -479,16 +501,18 @@ static int ir_setkeytable(struct rc_dev *dev, const struct rc_map *from)
 	if (rc)
 		return rc;
 
-	for (i = 0; i < from->size; i++) {
-		index = ir_establish_scancode(dev, rc_map,
-					      from->scan[i].scancode, false);
-		if (index >= rc_map->len) {
-			rc = -ENOMEM;
-			break;
-		}
+	scoped_guard(spinlock_irqsave, &rc_map->lock) {
+		for (i = 0; i < from->size; i++) {
+			index = ir_establish_scancode(dev, rc_map,
+						      from->scan[i].scancode, false);
+			if (index >= rc_map->len) {
+				rc = -ENOMEM;
+				break;
+			}
 
-		ir_update_mapping(dev, rc_map, index,
-				  from->scan[i].keycode);
+			ir_update_mapping(dev, rc_map, index,
+					  from->scan[i].keycode);
+		}
 	}
 
 	if (rc)
@@ -524,6 +548,8 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
 {
 	struct rc_map_table *res;
 
+	lockdep_assert_held(&rc_map->lock);
+
 	res = bsearch(&scancode, rc_map->scan, rc_map->len,
 		      sizeof(struct rc_map_table), rc_map_cmp);
 	if (!res)
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index d95ed3e96de2..f167c37179c8 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -148,7 +148,6 @@ struct rc_map_table {
  * @scan: pointer to struct &rc_map_table
  * @size: Max number of entries
  * @len: Number of entries that are in use
- * @alloc: size of \*scan, in bytes
  * @rc_proto: type of the remote controller protocol, as defined at
  *	     enum &rc_proto
  * @name: name of the key map table
@@ -158,7 +157,6 @@ struct rc_map {
 	struct rc_map_table	*scan;
 	unsigned int		size;
 	unsigned int		len;
-	unsigned int		alloc;
 	enum rc_proto		rc_proto;
 	const char		*name;
 	spinlock_t		lock;
-- 
2.55.0

[PATCH v4 05/19] media: rc: Fix race between bpf(BPG_PROG_ATTACH) and device unregister

[Sean Young]

Mark the device as unregistered before tearing down raw IR/bpf
state, so that racing bpf(BPF_PROG_{ATTACH,DETACH,QUERY}) calls
cannot slip in between ir_raw_event_unregister() completing and
dev->registered being cleared.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/rc-main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 04204559959f..924b13fff753 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -2050,18 +2050,18 @@ void rc_unregister_device(struct rc_dev *dev)
 	if (!dev)
 		return;
 
+	mutex_lock(&dev->lock);
+	dev->registered = false;
+	if (dev->users && dev->close)
+		dev->close(dev);
+	mutex_unlock(&dev->lock);
+
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		ir_raw_event_unregister(dev);
 
 	timer_delete_sync(&dev->timer_keyup);
 	timer_delete_sync(&dev->timer_repeat);
 
-	mutex_lock(&dev->lock);
-	if (dev->users && dev->close)
-		dev->close(dev);
-	dev->registered = false;
-	mutex_unlock(&dev->lock);
-
 	rc_free_rx_device(dev);
 
 	/*
-- 
2.55.0

[PATCH v4 06/19] media: rc: mce_kbd: Fix inconsistent locking of keylock

[Sean Young]

data->keylock is taken with spin_lock() in ir_mce_kbd_decode(), which
runs in the raw IR decode kthread with local interrupts enabled, but
with spin_lock_irqsave() in mce_kbd_rx_timeout(), the rx_timeout timer
callback, which runs in softirq context.

If the decode kthread is holding keylock via spin_lock() when a timer
interrupt fires on the same CPU and the softirq runs
mce_kbd_rx_timeout(), the softirq spins forever waiting for a lock
that only the now-preempted kthread can release, while the kthread
cannot run again until the softirq gives up the CPU. This deadlocks
the CPU.

Fixes: 53a62800efb2 ("media: rc: mce_kbd decoder: fix race condition")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/ir-mce_kbd-decoder.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-mce_kbd-decoder.c b/drivers/media/rc/ir-mce_kbd-decoder.c
index bb2d7c37c263..d1c9263511b0 100644
--- a/drivers/media/rc/ir-mce_kbd-decoder.c
+++ b/drivers/media/rc/ir-mce_kbd-decoder.c
@@ -219,6 +219,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev)
 	struct mce_kbd_dec *data = &dev->raw->mce_kbd;
 	u32 scancode;
 	unsigned long delay;
+	unsigned long flags;
 	struct lirc_scancode lsc = {};
 
 	if (!is_timing_event(ev)) {
@@ -319,7 +320,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			scancode = data->body & 0xffffff;
 			dev_dbg(&dev->dev, "keyboard data 0x%08x\n",
 				data->body);
-			spin_lock(&data->keylock);
+			spin_lock_irqsave(&data->keylock, flags);
 			if (scancode) {
 				delay = usecs_to_jiffies(dev->timeout) +
 					msecs_to_jiffies(100);
@@ -329,7 +330,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			}
 			/* Pass data to keyboard buffer parser */
 			ir_mce_kbd_process_keyboard_data(dev, scancode);
-			spin_unlock(&data->keylock);
+			spin_unlock_irqrestore(&data->keylock, flags);
 			lsc.rc_proto = RC_PROTO_MCIR2_KBD;
 			break;
 		case MCIR2_MOUSE_NBITS:
-- 
2.55.0

[PATCH v4 07/19] media: ene_ir: Ensure teardown is done in the correct order

[Sean Young]

The timer can be re-armed from the irq, so ensure that the timers are
deleted after the irq and that we only free the rc device once we are
sure that the irq has been silenced.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/ene_ir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index 6f7dccc965e7..f98d76277b62 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -1103,15 +1103,15 @@ static void ene_remove(struct pnp_dev *pnp_dev)
 	unsigned long flags;
 
 	rc_unregister_device(dev->rdev);
-	timer_delete_sync(&dev->tx_sim_timer);
 	spin_lock_irqsave(&dev->hw_lock, flags);
 	ene_rx_disable(dev);
 	ene_rx_restore_hw_buffer(dev);
 	spin_unlock_irqrestore(&dev->hw_lock, flags);
 
-	rc_free_device(dev->rdev);
 	free_irq(dev->irq, dev);
+	timer_delete_sync(&dev->tx_sim_timer);
 	release_region(dev->hw_io, ENE_IO_SIZE);
+	rc_free_device(dev->rdev);
 	kfree(dev);
 }
 
-- 
2.55.0

[PATCH v4 15/19] media: serial_ir: Fix race condition where timer can be re-armed

[Sean Young]

Ensure the timer cannot be re-armed.

Fixes: 2940c7e49775 ("[media] serial_ir: generate timeout")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/serial_ir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/serial_ir.c b/drivers/media/rc/serial_ir.c
index 992fff82b524..f6702b3df9a0 100644
--- a/drivers/media/rc/serial_ir.c
+++ b/drivers/media/rc/serial_ir.c
@@ -798,7 +798,7 @@ static int __init serial_ir_init_module(void)
 
 static void __exit serial_ir_exit_module(void)
 {
-	timer_delete_sync(&serial_ir.timeout_timer);
+	timer_shutdown_sync(&serial_ir.timeout_timer);
 	serial_ir_exit();
 }
 
-- 
2.55.0

[PATCH v4 16/19] media: rc: After rc_unregister_device() timers can be re-armed

[Sean Young]

If rc_keydown() is called after rc_register_device(), then the timers
can be re-armed.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/rc-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 795ac0fe4858..088d935d8347 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -2065,8 +2065,8 @@ void rc_unregister_device(struct rc_dev *dev)
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		ir_raw_event_unregister(dev);
 
-	timer_delete_sync(&dev->timer_keyup);
-	timer_delete_sync(&dev->timer_repeat);
+	timer_shutdown_sync(&dev->timer_keyup);
+	timer_shutdown_sync(&dev->timer_repeat);
 
 	rc_free_rx_device(dev);
 
-- 
2.55.0

[PATCH v4 18/19] media: ir_toy: Validate the maximum tx carrier

[Sean Young]

Reject any carrier the hardware cannot generate. This also fixes
a divide by zero if a carrier of U32_MAX/16 is set.

Fixes: 8bff1386d62d ("media: ir_toy: allow tx carrier to be set")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/ir_toy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/ir_toy.c b/drivers/media/rc/ir_toy.c
index 286c8543aa5a..58b23d08a897 100644
--- a/drivers/media/rc/ir_toy.c
+++ b/drivers/media/rc/ir_toy.c
@@ -372,7 +372,7 @@ static int irtoy_tx_carrier(struct rc_dev *rc, uint32_t carrier)
 	u8 buf[3];
 	int err;
 
-	if (carrier < 11800)
+	if (carrier < 11800 || carrier > 3000000)
 		return -EINVAL;
 
 	buf[0] = 0x06;
-- 
2.55.0

[PATCH v4 19/19] media: meson-ir-tx: Validate carrier and duty_cycle

[Sean Young]

It is possible to set a combination of carrier and duty cycle that do
not produce a correct signal. In addition, setting a carrier larger
than USEC_PER_SEC will result in a divide by zero in
meson_irtx_prepare_pulse() during transmit.

Fixes: 49be1c78d575 ("media: rc: introduce Meson IR TX driver")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/meson-ir-tx.c | 54 +++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/media/rc/meson-ir-tx.c b/drivers/media/rc/meson-ir-tx.c
index 1ab898069e67..a410464ede22 100644
--- a/drivers/media/rc/meson-ir-tx.c
+++ b/drivers/media/rc/meson-ir-tx.c
@@ -76,11 +76,24 @@ struct meson_irtx {
 	unsigned long clk_rate;
 };
 
-static void meson_irtx_set_mod(struct meson_irtx *ir)
+static bool meson_irtx_calc_mod(unsigned long clk_rate, u32 carrier,
+				u32 duty_cycle, unsigned int *pulse_cnt,
+				unsigned int *space_cnt)
 {
-	unsigned int cnt = DIV_ROUND_CLOSEST(ir->clk_rate, ir->carrier);
-	unsigned int pulse_cnt = DIV_ROUND_CLOSEST(cnt * ir->duty_cycle, 100);
-	unsigned int space_cnt = cnt - pulse_cnt;
+	unsigned int cnt;
+
+	cnt = DIV_ROUND_CLOSEST(clk_rate, carrier);
+	*pulse_cnt = DIV_ROUND_CLOSEST(cnt * duty_cycle, 100);
+	*space_cnt = cnt - *pulse_cnt;
+
+	return *pulse_cnt >= 1 && *pulse_cnt <= 65536 &&
+	       *space_cnt >= 1 && *space_cnt <= 65536;
+}
+
+static void meson_irtx_write_mod(struct meson_irtx *ir, unsigned int pulse_cnt,
+				 unsigned int space_cnt)
+{
+	unsigned int cnt = pulse_cnt + space_cnt;
 
 	dev_dbg(ir->dev, "F_mod = %uHz, T_mod = %luns, duty_cycle = %u%%\n",
 		ir->carrier, NSEC_PER_SEC / ir->clk_rate * cnt,
@@ -90,8 +103,10 @@ static void meson_irtx_set_mod(struct meson_irtx *ir)
 	       ir->reg_base + IRB_ADDR1);
 }
 
-static void meson_irtx_setup(struct meson_irtx *ir, unsigned int clk_nr)
+static int meson_irtx_setup(struct meson_irtx *ir, unsigned int clk_nr)
 {
+	unsigned int pulse_cnt, space_cnt;
+
 	/*
 	 * Disable the TX, set modulator clock tick and set initialize
 	 * output to be high. Set up carrier frequency and duty cycle. Then
@@ -100,13 +115,18 @@ static void meson_irtx_setup(struct meson_irtx *ir, unsigned int clk_nr)
 	 */
 	writel(~IRB_ENABLE & (IRB_MOD_CLK(clk_nr) | IRB_INIT_HIGH),
 	       ir->reg_base + IRB_ADDR0);
-	meson_irtx_set_mod(ir);
+	if (!meson_irtx_calc_mod(ir->clk_rate, ir->carrier, ir->duty_cycle,
+				 &pulse_cnt, &space_cnt))
+		return -EINVAL;
+	meson_irtx_write_mod(ir, pulse_cnt, space_cnt);
 	writel(readl(ir->reg_base + IRB_ADDR0) & ~IRB_INIT_HIGH,
 	       ir->reg_base + IRB_ADDR0);
 	writel(IRB_FIFO_IRQ_ENABLE | MIRTX_FIFO_THD,
 	       ir->reg_base + IRB_ADDR3);
 	writel(readl(ir->reg_base + IRB_ADDR0) | IRB_ENABLE,
 	       ir->reg_base + IRB_ADDR0);
+
+	return 0;
 }
 
 static u32 meson_irtx_prepare_pulse(struct meson_irtx *ir, unsigned int time)
@@ -215,12 +235,17 @@ static irqreturn_t meson_irtx_irqhandler(int irq, void *data)
 static int meson_irtx_set_carrier(struct rc_dev *rc, u32 carrier)
 {
 	struct meson_irtx *ir = rc->priv;
+	unsigned int pulse_cnt, space_cnt;
+
+	if (!carrier)
+		return -EINVAL;
 
-	if (carrier == 0)
+	if (!meson_irtx_calc_mod(ir->clk_rate, carrier, ir->duty_cycle,
+				 &pulse_cnt, &space_cnt))
 		return -EINVAL;
 
 	ir->carrier = carrier;
-	meson_irtx_set_mod(ir);
+	meson_irtx_write_mod(ir, pulse_cnt, space_cnt);
 
 	return 0;
 }
@@ -228,9 +253,14 @@ static int meson_irtx_set_carrier(struct rc_dev *rc, u32 carrier)
 static int meson_irtx_set_duty_cycle(struct rc_dev *rc, u32 duty_cycle)
 {
 	struct meson_irtx *ir = rc->priv;
+	unsigned int pulse_cnt, space_cnt;
+
+	if (!meson_irtx_calc_mod(ir->clk_rate, ir->carrier, duty_cycle,
+				 &pulse_cnt, &space_cnt))
+		return -EINVAL;
 
 	ir->duty_cycle = duty_cycle;
-	meson_irtx_set_mod(ir);
+	meson_irtx_write_mod(ir, pulse_cnt, space_cnt);
 
 	return 0;
 }
@@ -337,7 +367,11 @@ static int meson_irtx_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "modulator clock setup failed\n");
 
-	meson_irtx_setup(ir, clk_nr);
+	ret = meson_irtx_setup(ir, clk_nr);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "carrier %u, duty_cycle %u%% not supported at clk_rate %luHz\n",
+				     ir->carrier, ir->duty_cycle, ir->clk_rate);
 
 	ret = devm_request_irq(dev, irq,
 			       meson_irtx_irqhandler,
-- 
2.55.0

Re: [PATCH v4 01/19] media: rc: Ensure registered is cleared in error path

[Hans Verkuil]

On 08/09/2026 17:51, Sean Young wrote:
> If rc_register_device() fails, ensure that registered is not set to true.
> If lirc_register() succeeded, then userspace could have an open file
> descriptor open. This leads to a use-after-free.
> 
> Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
> Signed-off-by: Sean Young <sean@mess.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/media/rc/rc-main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index d93e98189c1a..1f99540456f1 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1934,7 +1934,8 @@ int rc_register_device(struct rc_dev *dev)
>  			goto out_raw;
>  	}
>  
> -	dev->registered = true;
> +	scoped_guard(mutex, &dev->lock)
> +		dev->registered = true;

Is the guard actually needed?

>  
>  	rc = device_add(&dev->dev);
>  	if (rc)
> @@ -1982,6 +1983,8 @@ int rc_register_device(struct rc_dev *dev)
>  out_dev:
>  	device_del(&dev->dev);
>  out_rx_free:
> +	scoped_guard(mutex, &dev->lock)
> +		dev->registered = false;

Same question here. I think the real fix is setting dev->registered to
false, but I'm not sure the guard makes a difference.

The commit log mentions nothing about that change either.

Regards,

	Hans

>  	ir_free_table(&dev->rc_map);
>  out_raw:
>  	ida_free(&rc_ida, minor);

Re: [PATCH v4 04/19] media: rc: Add missing locking for keymap

[Hans Verkuil]

On 08/09/2026 17:51, Sean Young wrote:
> When the rc_map for an rc_dev gets updated, locking is required but is
> missing in places. A concurrent scancode lookup and a call to
> rc_{register,unregistered}_device() could result in a use-after-free;
> this could happen if IR is decoded during those function calls.
> 
> We also fix some ugliness like open-coded krealloc() and removing the
> pointless alloc member of rc_map.

I would recommend splitting off the krealloc changes etc. into a separate
patch. It made it hard to review this patch with that change thrown in.

> 
> Add lockdep assertions where locks are required.
> 
> Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
> Signed-off-by: Sean Young <sean@mess.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/media/rc/rc-main.c | 122 ++++++++++++++++++++++---------------
>  include/media/rc-map.h     |   2 -
>  2 files changed, 74 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 88d8f7d4aab3..04204559959f 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -17,9 +17,8 @@
>  #include <linux/module.h>
>  #include "rc-core-priv.h"
>  
> -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */
> -#define IR_TAB_MIN_SIZE	256
> -#define IR_TAB_MAX_SIZE	8192
> +#define IR_TAB_MIN_SIZE	32
> +#define IR_TAB_MAX_SIZE	1024

It doesn't hurt to keep a comment mentioning that these are the
min and max number of entries.

In general I found it a bit confusing that 'size' refers to number
of elements in variable names, when it is usually the number of
entries. Perhaps something to address in a future patch.

Regards,

	Hans

>  
>  static const struct {
>  	const char *name;
> @@ -105,7 +104,6 @@ static struct rc_map_list *seek_rc_map(const char *name)
>  
>  struct rc_map *rc_map_get(const char *name)
>  {
> -
>  	struct rc_map_list *map;
>  
>  	map = seek_rc_map(name);
> @@ -202,7 +200,7 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
>   * ir_create_table() - initializes a scancode table
>   * @dev:	the rc_dev device
>   * @rc_map:	the rc_map to initialize
> - * @name:	name to assign to the table
> + * @map_name:	name to assign to the table
>   * @rc_proto:	ir type to assign to the new table
>   * @size:	initial size of the table
>   *
> @@ -212,23 +210,33 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
>   * return:	zero on success or a negative error code
>   */
>  static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
> -			   const char *name, u64 rc_proto, size_t size)
> +			   const char *map_name, u64 rc_proto, size_t size)
>  {
> -	rc_map->name = kstrdup(name, GFP_KERNEL);
> -	if (!rc_map->name)
> +	struct rc_map_table *scan;
> +	unsigned int alloc;
> +	char *name;
> +
> +	name = kstrdup(map_name, GFP_KERNEL);
> +	if (!name)
>  		return -ENOMEM;
> -	rc_map->rc_proto = rc_proto;
> -	rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
> -	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> -	rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
> -	if (!rc_map->scan) {
> -		kfree(rc_map->name);
> -		rc_map->name = NULL;
> +
> +	alloc = roundup_pow_of_two(size);
> +	scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);
> +	if (!scan) {
> +		kfree(name);
>  		return -ENOMEM;
>  	}
>  
> -	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%u bytes)\n",
> -		rc_map->size, rc_map->alloc);
> +	scoped_guard(spinlock_irqsave, &rc_map->lock) {
> +		rc_map->name = name;
> +		rc_map->scan = scan;
> +		rc_map->rc_proto = rc_proto;
> +		rc_map->len = 0;
> +		rc_map->size = alloc;
> +	}
> +
> +	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%zu bytes)\n",
> +		alloc, alloc * sizeof(struct rc_map_table));
>  	return 0;
>  }
>  
> @@ -236,16 +244,26 @@ static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
>   * ir_free_table() - frees memory allocated by a scancode table
>   * @rc_map:	the table whose mappings need to be freed
>   *
> - * This routine will free memory alloctaed for key mappings used by given
> + * This routine will free memory allocated for key mappings used by given
>   * scancode table.
>   */
>  static void ir_free_table(struct rc_map *rc_map)
>  {
> -	rc_map->size = 0;
> -	kfree(rc_map->name);
> -	rc_map->name = NULL;
> -	kfree(rc_map->scan);
> -	rc_map->scan = NULL;
> +	struct rc_map_table *scan;
> +	const char *name;
> +
> +	scoped_guard(spinlock_irqsave, &rc_map->lock) {
> +		name = rc_map->name;
> +		scan = rc_map->scan;
> +
> +		rc_map->size = 0;
> +		rc_map->len = 0;
> +		rc_map->name = NULL;
> +		rc_map->scan = NULL;
> +	}
> +
> +	kfree(name);
> +	kfree(scan);
>  }
>  
>  /**
> @@ -262,38 +280,38 @@ static void ir_free_table(struct rc_map *rc_map)
>  static int ir_resize_table(struct rc_dev *dev, struct rc_map *rc_map,
>  			   gfp_t gfp_flags)
>  {
> -	unsigned int oldalloc = rc_map->alloc;
> -	unsigned int newalloc = oldalloc;
> -	struct rc_map_table *oldscan = rc_map->scan;
> +	unsigned int newsize = rc_map->size;
>  	struct rc_map_table *newscan;
>  
> +	lockdep_assert_held(&rc_map->lock);
> +
>  	if (rc_map->size == rc_map->len) {
>  		/* All entries in use -> grow keytable */
> -		if (rc_map->alloc >= IR_TAB_MAX_SIZE)
> +		if (newsize >= IR_TAB_MAX_SIZE)
>  			return -ENOMEM;
>  
> -		newalloc *= 2;
> -		dev_dbg(&dev->dev, "Growing table to %u bytes\n", newalloc);
> +		newsize *= 2;
> +
> +		dev_dbg(&dev->dev, "Growing table to %u entries\n", newsize);
>  	}
>  
> -	if ((rc_map->len * 3 < rc_map->size) && (oldalloc > IR_TAB_MIN_SIZE)) {
> +	if (rc_map->len * 3 < rc_map->size && rc_map->size > IR_TAB_MIN_SIZE) {
>  		/* Less than 1/3 of entries in use -> shrink keytable */
> -		newalloc /= 2;
> -		dev_dbg(&dev->dev, "Shrinking table to %u bytes\n", newalloc);
> +		newsize /= 2;
> +		dev_dbg(&dev->dev, "Shrinking table to %u entries\n", newsize);
>  	}
>  
> -	if (newalloc == oldalloc)
> +	if (newsize == rc_map->size)
>  		return 0;
>  
> -	newscan = kmalloc(newalloc, gfp_flags);
> +	newscan = krealloc_array(rc_map->scan, newsize,
> +				 sizeof(struct rc_map_table), gfp_flags);
>  	if (!newscan)
>  		return -ENOMEM;
>  
> -	memcpy(newscan, rc_map->scan, rc_map->len * sizeof(struct rc_map_table));
>  	rc_map->scan = newscan;
> -	rc_map->alloc = newalloc;
> -	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> -	kfree(oldscan);
> +	rc_map->size = newsize;
> +
>  	return 0;
>  }
>  
> @@ -318,6 +336,8 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
>  	int old_keycode = rc_map->scan[index].keycode;
>  	int i;
>  
> +	lockdep_assert_held(&rc_map->lock);
> +
>  	/* Did the user wish to remove the mapping? */
>  	if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
>  		dev_dbg(&dev->dev, "#%d: Deleting scan 0x%04llx\n",
> @@ -373,6 +393,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
>  {
>  	unsigned int i;
>  
> +	lockdep_assert_held(&rc_map->lock);
> +
>  	/*
>  	 * Unfortunately, some hardware-based IR decoders don't provide
>  	 * all bits for the complete IR code. In general, they provide only
> @@ -397,7 +419,7 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
>  	/* No previous mapping found, we might need to grow the table */
>  	if (rc_map->size == rc_map->len) {
>  		if (!resize || ir_resize_table(dev, rc_map, GFP_ATOMIC))
> -			return -1U;
> +			return UINT_MAX;
>  	}
>  
>  	/* i is the proper index to insert our new keycode */
> @@ -479,16 +501,18 @@ static int ir_setkeytable(struct rc_dev *dev, const struct rc_map *from)
>  	if (rc)
>  		return rc;
>  
> -	for (i = 0; i < from->size; i++) {
> -		index = ir_establish_scancode(dev, rc_map,
> -					      from->scan[i].scancode, false);
> -		if (index >= rc_map->len) {
> -			rc = -ENOMEM;
> -			break;
> -		}
> +	scoped_guard(spinlock_irqsave, &rc_map->lock) {
> +		for (i = 0; i < from->size; i++) {
> +			index = ir_establish_scancode(dev, rc_map,
> +						      from->scan[i].scancode, false);
> +			if (index >= rc_map->len) {
> +				rc = -ENOMEM;
> +				break;
> +			}
>  
> -		ir_update_mapping(dev, rc_map, index,
> -				  from->scan[i].keycode);
> +			ir_update_mapping(dev, rc_map, index,
> +					  from->scan[i].keycode);
> +		}
>  	}
>  
>  	if (rc)
> @@ -524,6 +548,8 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
>  {
>  	struct rc_map_table *res;
>  
> +	lockdep_assert_held(&rc_map->lock);
> +
>  	res = bsearch(&scancode, rc_map->scan, rc_map->len,
>  		      sizeof(struct rc_map_table), rc_map_cmp);
>  	if (!res)
> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> index d95ed3e96de2..f167c37179c8 100644
> --- a/include/media/rc-map.h
> +++ b/include/media/rc-map.h
> @@ -148,7 +148,6 @@ struct rc_map_table {
>   * @scan: pointer to struct &rc_map_table
>   * @size: Max number of entries
>   * @len: Number of entries that are in use
> - * @alloc: size of \*scan, in bytes
>   * @rc_proto: type of the remote controller protocol, as defined at
>   *	     enum &rc_proto
>   * @name: name of the key map table
> @@ -158,7 +157,6 @@ struct rc_map {
>  	struct rc_map_table	*scan;
>  	unsigned int		size;
>  	unsigned int		len;
> -	unsigned int		alloc;
>  	enum rc_proto		rc_proto;
>  	const char		*name;
>  	spinlock_t		lock;

Re: [PATCH v4 01/19] media: rc: Ensure registered is cleared in error path

[Sean Young]

On Fri, Sep 11, 2026 at 09:53:10AM +0200, Hans Verkuil wrote:
> On 08/09/2026 17:51, Sean Young wrote:
> > If rc_register_device() fails, ensure that registered is not set to true.
> > If lirc_register() succeeded, then userspace could have an open file
> > descriptor open. This leads to a use-after-free.
> > 
> > Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
> > Signed-off-by: Sean Young <sean@mess.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/media/rc/rc-main.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> > index d93e98189c1a..1f99540456f1 100644
> > --- a/drivers/media/rc/rc-main.c
> > +++ b/drivers/media/rc/rc-main.c
> > @@ -1934,7 +1934,8 @@ int rc_register_device(struct rc_dev *dev)
> >  			goto out_raw;
> >  	}
> >  
> > -	dev->registered = true;
> > +	scoped_guard(mutex, &dev->lock)
> > +		dev->registered = true;
> 
> Is the guard actually needed?

Yes, we read registered under lock in e.g. in lirc_open(). It s
racey without.

> >  	rc = device_add(&dev->dev);
> >  	if (rc)
> > @@ -1982,6 +1983,8 @@ int rc_register_device(struct rc_dev *dev)
> >  out_dev:
> >  	device_del(&dev->dev);
> >  out_rx_free:
> > +	scoped_guard(mutex, &dev->lock)
> > +		dev->registered = false;
> 
> Same question here. I think the real fix is setting dev->registered to
> false, but I'm not sure the guard makes a difference.
> 
> The commit log mentions nothing about that change either.

You are right, the commit message needs to mention this.


Sean

Re: [PATCH v4 04/19] media: rc: Add missing locking for keymap

[Sean Young]

On Fri, Sep 11, 2026 at 10:08:03AM +0200, Hans Verkuil wrote:
> On 08/09/2026 17:51, Sean Young wrote:
> > When the rc_map for an rc_dev gets updated, locking is required but is
> > missing in places. A concurrent scancode lookup and a call to
> > rc_{register,unregistered}_device() could result in a use-after-free;
> > this could happen if IR is decoded during those function calls.
> > 
> > We also fix some ugliness like open-coded krealloc() and removing the
> > pointless alloc member of rc_map.
> 
> I would recommend splitting off the krealloc changes etc. into a separate
> patch. It made it hard to review this patch with that change thrown in.

You're right, this commit has become to big and needs splitting.

> > Add lockdep assertions where locks are required.
> > 
> > Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
> > Signed-off-by: Sean Young <sean@mess.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/media/rc/rc-main.c | 122 ++++++++++++++++++++++---------------
> >  include/media/rc-map.h     |   2 -
> >  2 files changed, 74 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> > index 88d8f7d4aab3..04204559959f 100644
> > --- a/drivers/media/rc/rc-main.c
> > +++ b/drivers/media/rc/rc-main.c
> > @@ -17,9 +17,8 @@
> >  #include <linux/module.h>
> >  #include "rc-core-priv.h"
> >  
> > -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */
> > -#define IR_TAB_MIN_SIZE	256
> > -#define IR_TAB_MAX_SIZE	8192
> > +#define IR_TAB_MIN_SIZE	32
> > +#define IR_TAB_MAX_SIZE	1024
> 
> It doesn't hurt to keep a comment mentioning that these are the
> min and max number of entries.
> 
> In general I found it a bit confusing that 'size' refers to number
> of elements in variable names, when it is usually the number of
> entries. Perhaps something to address in a future patch.

That is confusing. Let me re-write it.

Thanks,

Sean

> 
> Regards,
> 
> 	Hans
> 
> >  
> >  static const struct {
> >  	const char *name;
> > @@ -105,7 +104,6 @@ static struct rc_map_list *seek_rc_map(const char *name)
> >  
> >  struct rc_map *rc_map_get(const char *name)
> >  {
> > -
> >  	struct rc_map_list *map;
> >  
> >  	map = seek_rc_map(name);
> > @@ -202,7 +200,7 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
> >   * ir_create_table() - initializes a scancode table
> >   * @dev:	the rc_dev device
> >   * @rc_map:	the rc_map to initialize
> > - * @name:	name to assign to the table
> > + * @map_name:	name to assign to the table
> >   * @rc_proto:	ir type to assign to the new table
> >   * @size:	initial size of the table
> >   *
> > @@ -212,23 +210,33 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
> >   * return:	zero on success or a negative error code
> >   */
> >  static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
> > -			   const char *name, u64 rc_proto, size_t size)
> > +			   const char *map_name, u64 rc_proto, size_t size)
> >  {
> > -	rc_map->name = kstrdup(name, GFP_KERNEL);
> > -	if (!rc_map->name)
> > +	struct rc_map_table *scan;
> > +	unsigned int alloc;
> > +	char *name;
> > +
> > +	name = kstrdup(map_name, GFP_KERNEL);
> > +	if (!name)
> >  		return -ENOMEM;
> > -	rc_map->rc_proto = rc_proto;
> > -	rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
> > -	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> > -	rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
> > -	if (!rc_map->scan) {
> > -		kfree(rc_map->name);
> > -		rc_map->name = NULL;
> > +
> > +	alloc = roundup_pow_of_two(size);
> > +	scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);
> > +	if (!scan) {
> > +		kfree(name);
> >  		return -ENOMEM;
> >  	}
> >  
> > -	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%u bytes)\n",
> > -		rc_map->size, rc_map->alloc);
> > +	scoped_guard(spinlock_irqsave, &rc_map->lock) {
> > +		rc_map->name = name;
> > +		rc_map->scan = scan;
> > +		rc_map->rc_proto = rc_proto;
> > +		rc_map->len = 0;
> > +		rc_map->size = alloc;
> > +	}
> > +
> > +	dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%zu bytes)\n",
> > +		alloc, alloc * sizeof(struct rc_map_table));
> >  	return 0;
> >  }
> >  
> > @@ -236,16 +244,26 @@ static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
> >   * ir_free_table() - frees memory allocated by a scancode table
> >   * @rc_map:	the table whose mappings need to be freed
> >   *
> > - * This routine will free memory alloctaed for key mappings used by given
> > + * This routine will free memory allocated for key mappings used by given
> >   * scancode table.
> >   */
> >  static void ir_free_table(struct rc_map *rc_map)
> >  {
> > -	rc_map->size = 0;
> > -	kfree(rc_map->name);
> > -	rc_map->name = NULL;
> > -	kfree(rc_map->scan);
> > -	rc_map->scan = NULL;
> > +	struct rc_map_table *scan;
> > +	const char *name;
> > +
> > +	scoped_guard(spinlock_irqsave, &rc_map->lock) {
> > +		name = rc_map->name;
> > +		scan = rc_map->scan;
> > +
> > +		rc_map->size = 0;
> > +		rc_map->len = 0;
> > +		rc_map->name = NULL;
> > +		rc_map->scan = NULL;
> > +	}
> > +
> > +	kfree(name);
> > +	kfree(scan);
> >  }
> >  
> >  /**
> > @@ -262,38 +280,38 @@ static void ir_free_table(struct rc_map *rc_map)
> >  static int ir_resize_table(struct rc_dev *dev, struct rc_map *rc_map,
> >  			   gfp_t gfp_flags)
> >  {
> > -	unsigned int oldalloc = rc_map->alloc;
> > -	unsigned int newalloc = oldalloc;
> > -	struct rc_map_table *oldscan = rc_map->scan;
> > +	unsigned int newsize = rc_map->size;
> >  	struct rc_map_table *newscan;
> >  
> > +	lockdep_assert_held(&rc_map->lock);
> > +
> >  	if (rc_map->size == rc_map->len) {
> >  		/* All entries in use -> grow keytable */
> > -		if (rc_map->alloc >= IR_TAB_MAX_SIZE)
> > +		if (newsize >= IR_TAB_MAX_SIZE)
> >  			return -ENOMEM;
> >  
> > -		newalloc *= 2;
> > -		dev_dbg(&dev->dev, "Growing table to %u bytes\n", newalloc);
> > +		newsize *= 2;
> > +
> > +		dev_dbg(&dev->dev, "Growing table to %u entries\n", newsize);
> >  	}
> >  
> > -	if ((rc_map->len * 3 < rc_map->size) && (oldalloc > IR_TAB_MIN_SIZE)) {
> > +	if (rc_map->len * 3 < rc_map->size && rc_map->size > IR_TAB_MIN_SIZE) {
> >  		/* Less than 1/3 of entries in use -> shrink keytable */
> > -		newalloc /= 2;
> > -		dev_dbg(&dev->dev, "Shrinking table to %u bytes\n", newalloc);
> > +		newsize /= 2;
> > +		dev_dbg(&dev->dev, "Shrinking table to %u entries\n", newsize);
> >  	}
> >  
> > -	if (newalloc == oldalloc)
> > +	if (newsize == rc_map->size)
> >  		return 0;
> >  
> > -	newscan = kmalloc(newalloc, gfp_flags);
> > +	newscan = krealloc_array(rc_map->scan, newsize,
> > +				 sizeof(struct rc_map_table), gfp_flags);
> >  	if (!newscan)
> >  		return -ENOMEM;
> >  
> > -	memcpy(newscan, rc_map->scan, rc_map->len * sizeof(struct rc_map_table));
> >  	rc_map->scan = newscan;
> > -	rc_map->alloc = newalloc;
> > -	rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> > -	kfree(oldscan);
> > +	rc_map->size = newsize;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -318,6 +336,8 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
> >  	int old_keycode = rc_map->scan[index].keycode;
> >  	int i;
> >  
> > +	lockdep_assert_held(&rc_map->lock);
> > +
> >  	/* Did the user wish to remove the mapping? */
> >  	if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
> >  		dev_dbg(&dev->dev, "#%d: Deleting scan 0x%04llx\n",
> > @@ -373,6 +393,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
> >  {
> >  	unsigned int i;
> >  
> > +	lockdep_assert_held(&rc_map->lock);
> > +
> >  	/*
> >  	 * Unfortunately, some hardware-based IR decoders don't provide
> >  	 * all bits for the complete IR code. In general, they provide only
> > @@ -397,7 +419,7 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
> >  	/* No previous mapping found, we might need to grow the table */
> >  	if (rc_map->size == rc_map->len) {
> >  		if (!resize || ir_resize_table(dev, rc_map, GFP_ATOMIC))
> > -			return -1U;
> > +			return UINT_MAX;
> >  	}
> >  
> >  	/* i is the proper index to insert our new keycode */
> > @@ -479,16 +501,18 @@ static int ir_setkeytable(struct rc_dev *dev, const struct rc_map *from)
> >  	if (rc)
> >  		return rc;
> >  
> > -	for (i = 0; i < from->size; i++) {
> > -		index = ir_establish_scancode(dev, rc_map,
> > -					      from->scan[i].scancode, false);
> > -		if (index >= rc_map->len) {
> > -			rc = -ENOMEM;
> > -			break;
> > -		}
> > +	scoped_guard(spinlock_irqsave, &rc_map->lock) {
> > +		for (i = 0; i < from->size; i++) {
> > +			index = ir_establish_scancode(dev, rc_map,
> > +						      from->scan[i].scancode, false);
> > +			if (index >= rc_map->len) {
> > +				rc = -ENOMEM;
> > +				break;
> > +			}
> >  
> > -		ir_update_mapping(dev, rc_map, index,
> > -				  from->scan[i].keycode);
> > +			ir_update_mapping(dev, rc_map, index,
> > +					  from->scan[i].keycode);
> > +		}
> >  	}
> >  
> >  	if (rc)
> > @@ -524,6 +548,8 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
> >  {
> >  	struct rc_map_table *res;
> >  
> > +	lockdep_assert_held(&rc_map->lock);
> > +
> >  	res = bsearch(&scancode, rc_map->scan, rc_map->len,
> >  		      sizeof(struct rc_map_table), rc_map_cmp);
> >  	if (!res)
> > diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> > index d95ed3e96de2..f167c37179c8 100644
> > --- a/include/media/rc-map.h
> > +++ b/include/media/rc-map.h
> > @@ -148,7 +148,6 @@ struct rc_map_table {
> >   * @scan: pointer to struct &rc_map_table
> >   * @size: Max number of entries
> >   * @len: Number of entries that are in use
> > - * @alloc: size of \*scan, in bytes
> >   * @rc_proto: type of the remote controller protocol, as defined at
> >   *	     enum &rc_proto
> >   * @name: name of the key map table
> > @@ -158,7 +157,6 @@ struct rc_map {
> >  	struct rc_map_table	*scan;
> >  	unsigned int		size;
> >  	unsigned int		len;
> > -	unsigned int		alloc;
> >  	enum rc_proto		rc_proto;
> >  	const char		*name;
> >  	spinlock_t		lock;
>