[PATCH 1/2] media: rc: uevent sysfs file races with rc_register_device()

[PATCH 1/2] media: rc: uevent sysfs file races with rc_register_device()

[Sean Young]

Only report uevent file contents if device still exists, else we might
read freed memory.

Reported-by: syzbot+ceef16277388d6f24898@syzkaller.appspotmail.com
Cc: Hillf Danton <hdanton@sina.com>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/rc-main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 7b53066d9d07..503ae4f3dec3 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1612,6 +1612,12 @@ static int rc_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 {
 	struct rc_dev *dev = to_rc_dev(device);
 
+	mutex_lock(&dev->lock);
+	if (!dev->registered) {
+		mutex_unlock(&dev->lock);
+		return -ENODEV;
+	}
+
 	if (dev->rc_map.name)
 		ADD_HOTPLUG_VAR("NAME=%s", dev->rc_map.name);
 	if (dev->driver_name)
@@ -1619,6 +1625,8 @@ static int rc_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 	if (dev->device_name)
 		ADD_HOTPLUG_VAR("DEV_NAME=%s", dev->device_name);
 
+	mutex_unlock(&dev->lock);
+
 	return 0;
 }
 
@@ -2011,14 +2019,14 @@ void rc_unregister_device(struct rc_dev *dev)
 	del_timer_sync(&dev->timer_keyup);
 	del_timer_sync(&dev->timer_repeat);
 
-	rc_free_rx_device(dev);
-
 	mutex_lock(&dev->lock);
 	if (dev->users && dev->close)
 		dev->close(dev);
 	dev->registered = false;
 	mutex_unlock(&dev->lock);
 
+	rc_free_rx_device(dev);
+
 	/*
 	 * lirc device should be freed with dev->registered = false, so
 	 * that userspace polling will get notified.
-- 
2.26.2

[PATCH 2/2] media: rc: do not access device via sysfs after rc_unregister_device()

[Sean Young]

Device drivers do not expect to have change_protocol or wakeup
re-programming to be accesed after rc_unregister_device(). This can
cause the device driver to access deallocated resources.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/rc-main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 503ae4f3dec3..c629941f309a 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1292,6 +1292,10 @@ static ssize_t store_protocols(struct device *device,
 	}
 
 	mutex_lock(&dev->lock);
+	if (!dev->registered) {
+		mutex_unlock(&dev->lock);
+		return -ENODEV;
+	}
 
 	old_protocols = *current_protocols;
 	new_protocols = old_protocols;
@@ -1430,6 +1434,10 @@ static ssize_t store_filter(struct device *device,
 		return -EINVAL;
 
 	mutex_lock(&dev->lock);
+	if (!dev->registered) {
+		mutex_unlock(&dev->lock);
+		return -ENODEV;
+	}
 
 	new_filter = *filter;
 	if (fattr->mask)
@@ -1544,6 +1552,10 @@ static ssize_t store_wakeup_protocols(struct device *device,
 	int i;
 
 	mutex_lock(&dev->lock);
+	if (!dev->registered) {
+		mutex_unlock(&dev->lock);
+		return -ENODEV;
+	}
 
 	allowed = dev->allowed_wakeup_protocols;
 
-- 
2.26.2

Re: [PATCH 1/2] media: rc: uevent sysfs file races with rc_register_device()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]

Hi Sean,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.8 next-20200807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sean-Young/media-rc-uevent-sysfs-file-races-with-rc_register_device/20200808-203329
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-m001-20200808 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
drivers/media/rc/rc-main.c:1630 rc_dev_uevent() warn: inconsistent returns 'dev->lock'.

vim +1630 drivers/media/rc/rc-main.c

d8b4b5822f51e2 David Härdeman       2010-10-29  1603  
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1604  #define ADD_HOTPLUG_VAR(fmt, val...)					\
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1605  	do {								\
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1606  		int err = add_uevent_var(env, fmt, val);		\
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1607  		if (err)						\
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1608  			return err;					\
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1609  	} while (0)
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1610  
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1611  static int rc_dev_uevent(struct device *device, struct kobj_uevent_env *env)
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1612  {
d8b4b5822f51e2 David Härdeman       2010-10-29  1613  	struct rc_dev *dev = to_rc_dev(device);
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1614  
e4b7677d34e789 Sean Young            2020-08-08  1615  	mutex_lock(&dev->lock);
e4b7677d34e789 Sean Young            2020-08-08  1616  	if (!dev->registered) {
e4b7677d34e789 Sean Young            2020-08-08  1617  		mutex_unlock(&dev->lock);
e4b7677d34e789 Sean Young            2020-08-08  1618  		return -ENODEV;
e4b7677d34e789 Sean Young            2020-08-08  1619  	}
e4b7677d34e789 Sean Young            2020-08-08  1620  
b088ba658b3438 Mauro Carvalho Chehab 2010-11-17  1621  	if (dev->rc_map.name)
b088ba658b3438 Mauro Carvalho Chehab 2010-11-17  1622  		ADD_HOTPLUG_VAR("NAME=%s", dev->rc_map.name);
d8b4b5822f51e2 David Härdeman       2010-10-29  1623  	if (dev->driver_name)
d8b4b5822f51e2 David Härdeman       2010-10-29  1624  		ADD_HOTPLUG_VAR("DRV_NAME=%s", dev->driver_name);
b9f407e31c5073 Sean Young            2017-09-01  1625  	if (dev->device_name)
b9f407e31c5073 Sean Young            2017-09-01  1626  		ADD_HOTPLUG_VAR("DEV_NAME=%s", dev->device_name);
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1627  
e4b7677d34e789 Sean Young            2020-08-08  1628  	mutex_unlock(&dev->lock);
e4b7677d34e789 Sean Young            2020-08-08  1629  
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09 @1630  	return 0;
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1631  }
bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1632  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50215 bytes --]

Re: [PATCH 1/2] media: rc: uevent sysfs file races with rc_register_device()

[Sean Young]

On Sat, Aug 08, 2020 at 10:42:00PM +0800, kernel test robot wrote:
> Hi Sean,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on linuxtv-media/master]
> [also build test WARNING on v5.8 next-20200807]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Sean-Young/media-rc-uevent-sysfs-file-races-with-rc_register_device/20200808-203329
> base:   git://linuxtv.org/media_tree.git master
> config: x86_64-randconfig-m001-20200808 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> smatch warnings:
> drivers/media/rc/rc-main.c:1630 rc_dev_uevent() warn: inconsistent returns 'dev->lock'.

smatch is totally right here, there are code paths where the mutex is not
unlocked. Oops.

I'll send out a v2 shortly.

Sean

> 
> vim +1630 drivers/media/rc/rc-main.c
> 
> d8b4b5822f51e2 David Härdeman       2010-10-29  1603  
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1604  #define ADD_HOTPLUG_VAR(fmt, val...)					\
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1605  	do {								\
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1606  		int err = add_uevent_var(env, fmt, val);		\
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1607  		if (err)						\
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1608  			return err;					\
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1609  	} while (0)
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1610  
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1611  static int rc_dev_uevent(struct device *device, struct kobj_uevent_env *env)
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1612  {
> d8b4b5822f51e2 David Härdeman       2010-10-29  1613  	struct rc_dev *dev = to_rc_dev(device);
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1614  
> e4b7677d34e789 Sean Young            2020-08-08  1615  	mutex_lock(&dev->lock);
> e4b7677d34e789 Sean Young            2020-08-08  1616  	if (!dev->registered) {
> e4b7677d34e789 Sean Young            2020-08-08  1617  		mutex_unlock(&dev->lock);
> e4b7677d34e789 Sean Young            2020-08-08  1618  		return -ENODEV;
> e4b7677d34e789 Sean Young            2020-08-08  1619  	}
> e4b7677d34e789 Sean Young            2020-08-08  1620  
> b088ba658b3438 Mauro Carvalho Chehab 2010-11-17  1621  	if (dev->rc_map.name)
> b088ba658b3438 Mauro Carvalho Chehab 2010-11-17  1622  		ADD_HOTPLUG_VAR("NAME=%s", dev->rc_map.name);
> d8b4b5822f51e2 David Härdeman       2010-10-29  1623  	if (dev->driver_name)
> d8b4b5822f51e2 David Härdeman       2010-10-29  1624  		ADD_HOTPLUG_VAR("DRV_NAME=%s", dev->driver_name);
> b9f407e31c5073 Sean Young            2017-09-01  1625  	if (dev->device_name)
> b9f407e31c5073 Sean Young            2017-09-01  1626  		ADD_HOTPLUG_VAR("DEV_NAME=%s", dev->device_name);
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1627  
> e4b7677d34e789 Sean Young            2020-08-08  1628  	mutex_unlock(&dev->lock);
> e4b7677d34e789 Sean Young            2020-08-08  1629  
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09 @1630  	return 0;
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1631  }
> bc2a6c5719efd7 Mauro Carvalho Chehab 2010-11-09  1632  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org