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]

Date:   Tue, 22 May 2018 15:39:55 +0200
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Jiri Kosina <jikos@...nel.org>
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

[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? (That's the motivation for me to discuss this old topic.)

> IOW if it exists, it's guaranteed to not go away on a probe error handling 
> path (and the rest is protected via ops_lock mutex).

Putting it in other words (assuming I got you right):

If there is a race, it needs fixing in the cdev code. The rtc driver can
stay as is.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |