Fix locking issues in rc core
Subsystems (4)
Sean Young
maintainerv2 cover Fix locking issues in rc core
Changes since v1:
- Fix issue where input device was still leaked
- Add: Fix race between bpf(BPG_PROG_ATTACH) and device unregister
- Add: Fix inconsistent locking of keylock
Sean Young (6):
media: rc: Ensure registered is cleared in error path
media: rc: Ensure that rc_unregister_device() does not free input
device
media: rc: Fix ABBA deadlock by making locks more fine grained
media: rc: Add missing locking for keymap
media: rc: Fix race between bpf(BPG_PROG_ATTACH) and device unregister
media: rc: mce_kbd: Fix inconsistent locking of keylock
drivers/media/rc/ir-mce_kbd-decoder.c | 5 +- drivers/media/rc/rc-ir-raw.c | 39 ++++--- drivers/media/rc/rc-main.c | 147 +++++++++++++++----------- include/media/rc-map.h | 2 - 4 files changed, 115 insertions(+), 78 deletions(-)
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v2 1/6 media: rc: Ensure registered is cleared in error path
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.
--- drivers/media/rc/rc-main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/media/rc/rc-main.c +4 -1
@@ -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);
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v2 2/6 media: rc: Ensure that rc_unregister_device() does not free input device
In reply to Sean YoungDuring 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.
--- drivers/media/rc/rc-main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/media/rc/rc-main.c +4 -4
@@ -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); }
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v2 3/6 media: rc: Fix ABBA deadlock by making locks more fine grained
In reply to Sean YoungThe 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.
--- drivers/media/rc/rc-ir-raw.c | 39 ++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 15 deletions(-)
drivers/media/rc/rc-ir-raw.c +24 -15
@@ -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);
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v2 4/6 media: rc: Add missing locking for keymap
In reply to Sean YoungWhen 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.
--- drivers/media/rc/rc-main.c | 122 ++++++++++++++++++++++--------------- include/media/rc-map.h | 2 - 2 files changed, 74 insertions(+), 50 deletions(-)
drivers/media/rc/rc-main.c +74 -48
@@ -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, &dev->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" , + rc_map->size, rc_map->size *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 ) + newsize *=2 ; + +if (newsize >=IR_TAB_MAX_SIZE )return -ENOMEM ; - newalloc *=2 ; -dev_dbg (&dev->dev,"Growing table to %u bytes\n" , newalloc); +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, &dev->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)
include/media/rc-map.h +0 -2
@@ -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;
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v2 5/6 media: rc: Fix race between bpf(BPG_PROG_ATTACH) and device unregister
In reply to Sean YoungMark 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.
--- drivers/media/rc/rc-main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/media/rc/rc-main.c +6 -6
@@ -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);/*
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v2 6/6 media: rc: mce_kbd: Fix inconsistent locking of keylock
In reply to Sean Youngdata->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.
--- drivers/media/rc/ir-mce_kbd-decoder.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/media/rc/ir-mce_kbd-decoder.c +3 -2
@@ -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 :
Signature · 1 line
2.55.0