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

[Sean Young <sean@mess.org>]

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;
>