linux-kernel - Re: rtc: fix chardev initialization races

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <nycvar.YFH.7.76.1805221726300.27054@cbobk.fhfr.pm>
Date:   Tue, 22 May 2018 17:33:40 +0200 (CEST)
From:   Jiri Kosina <jikos@...nel.org>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
cc:     Ales Novak <alnovak@...e.cz>, a.zummo@...ertech.it,
        akpm@...ux-foundation.org, rtc-linux@...glegroups.com,
        linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: rtc: fix chardev initialization races

On Tue, 22 May 2018, Uwe Kleine-König wrote:
> [adding linux-rtc ML and Alexandre to Cc:]
> 
> Hello,
> 
> On Tue, May 22, 2018 at 02:09:36PM +0200, Jiri Kosina wrote:
> > On Mon, 21 May 2018, Uwe Kleine-König wrote:
> > 
> > > > 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* */
> > > 
> > > [Context: For a patch to rtc-pcf2127.c Alexandre Belloni asked not to
> > > fail after rtc_device_register successfully finished and pointed to this
> > > reasoning as explaination.]
> > > 
> > > If there is really such a race then (I hope) there is
> > > something in the cdev code that needs fixing. According to my
> > > understanding, when rtc_device_unregister returned, the cdev is gone and
> > > so chrdev_open is supposed to fail.
> > 
> > Oh wow, hello back to 4 years ago!
> 
> :-)
> 

> > Looking at the current code, I don't think there is no such race any more, 
> > as the last thing cmos_do_probe() -> __rtc_register_device() does that can 
> > potentially fail is the chardev creation itself.
> 
> OK, so you agree that it's also save to do something in a driver's probe
> after rtc_device_register() and call rtc_device_unregister() in the error
> path, right? 

Hmm, not really; that's what the code apparently did 4 years ago (judging 
from the scenario in the old mail, I of course forgot all the details), 
but doesn't do it any more.

Looking at the current code, if you call rtc_device_unregister() in the 
probe path, where is the guarantee that cdev_get() will not derefernce 
already freed struct module*?

Thanks,

-- 
Jiri Kosina
SUSE Labs