On Wed, 26 Feb 2014 11:33:13 +0100 Ales Novak <alno...@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().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
