lkml.org 
[lkml]   [2014]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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().



\
 
 \ /
  Last update: 2014-02-27 02:01    [W:0.968 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site