Messages in this thread | | | Date | Wed, 26 Feb 2014 16:33:47 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] rtc: fix chardev initialization races |
| |
On Wed, 26 Feb 2014 11:33:13 +0100 Ales Novak <alnovak@suse.cz> wrote:
> In many rtc modules, the chardevice file in rtc module probe is > being created prematurely. If the probe fails after the chardevice > file has been created (e.g. after rtc_device_register), it's possible > for a program to open() it, which subsequently can cause memory > corruption. > > The race looks like that (thanks Jiri): > > CPU0: CPU1: > sys_load_module() > do_init_module() > do_one_initcall() > cmos_do_probe() > rtc_device_register() > __register_chrdev() > cdev->owner = struct module* > open("/dev/rtc0") > rtc_device_unregister() > module_put() > free_module() > module_free(mod->module_core) > /* struct module *module is now > freed */ > chrdev_open() > spin_lock(cdev_lock) > cdev_get() > try_module_get() > module_is_live() > /* dereferences already > freed struct module* */ > > This patch is proposing a solution, splitting the function > {devm_,}rtc_device_register into {devm_,}rtc_device_register{_fs,}. > The {devm_}rtc_device_register_fs which is creating the files, should > be called after it is clear that the probe will pass. It will set the > RTC_DEV_FILES_EXIST into rtc_device->flags. > > In case of probe not passing, the rtc_device_unregister will try to > delete the files only if RTC_DEV_FILES_EXIST is set in rtc_device->flags. > > .. > > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -252,6 +245,21 @@ exit: > } > EXPORT_SYMBOL_GPL(rtc_device_register); > > +/** > + * rtc_device_register_fs - creates dev+sysfs+proc files > + * for rtc device > + */ > +void rtc_device_register_fs(struct device *dev, struct rtc_device *rtc) > +{ > + rtc_dev_add_device(rtc); > + rtc_sysfs_add_device(rtc); > + rtc_proc_add_device(rtc);
It's a bit sloppy that rtc core simply ignores the error codes from the driver core functions. All these things could have failed and the driver just keeps on trying to work.
I guess that can be fixed separately, but perhaps you'd like to take a look, see how complex it will be to correctly unwind after such errors?
> + set_bit(RTC_DEV_FILES_EXIST, &rtc->flags); > + dev_info(dev, "rtc core: registered %s as %s\n", > + rtc->name, dev_name(&rtc->dev)); > +} > +EXPORT_SYMBOL_GPL(rtc_device_register_fs); > > /** > * rtc_device_unregister - removes the previously registered RTC class device > @@ -262,13 +270,15 @@ void rtc_device_unregister(struct rtc_device *rtc) > { > if (get_device(&rtc->dev) != NULL) { > mutex_lock(&rtc->ops_lock); > - /* remove innards of this RTC, then disable it, before > - * letting any rtc_class_open() users access it again > - */ > - rtc_sysfs_del_device(rtc); > - rtc_dev_del_device(rtc); > - rtc_proc_del_device(rtc); > - device_unregister(&rtc->dev); > + if (test_and_clear_bit(RTC_DEV_FILES_EXIST, &rtc->flags)) { > + /* remove innards of this RTC, then disable it, before > + * letting any rtc_class_open() users access it again > + */ > + rtc_sysfs_del_device(rtc); > + rtc_dev_del_device(rtc); > + rtc_proc_del_device(rtc); > + device_unregister(&rtc->dev);
Presumably it doesn't matter, but it would be nice to do the del functions in the reverse order of the add functions. rtc_proc_del_device() then rtc_sysfs_del_device() then rtc_dev_del_device().
| |