linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: rtc: fix chardev initialization races
       [not found] <1393410793-7315-1-git-send-email-alnovak@suse.cz>
@ 2018-05-21 12:25 ` Uwe Kleine-König
       [not found]   ` <nycvar.YFH.7.76.1805221407100.27054@cbobk.fhfr.pm>
  0 siblings, 1 reply; 4+ messages in thread
From: Uwe Kleine-König @ 2018-05-21 12:25 UTC (permalink / raw)
  To: Ales Novak; +Cc: a.zummo, akpm, rtc-linux, linux-kernel, jkosina

[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]

On Wed, Feb 26, 2014 at 11:33:13AM +0100, Ales Novak 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* */

[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.

Otherwise the same problem can be triggered when the device is unbound
and the module unloaded while another cpu opens the char dev.

I added a msleep(5000) to chrdev_open like this:

        if (current->pid != 1) {
                pr_info("%s:%d: sleeping to open race window\n", __func__, __LINE__);
                msleep(5000);
                pr_info("%s:%d: done sleeping\n", __func__, __LINE__);
        }

before the spinlock is taken. If I trigger that (using

	a = open("/dev/rtc0")

in a Python shell) and then do rmmod rtc_pcf2127 (which is the driver
backing my rtc0), I get:

	OSError: [Errno 6] No such device or address: '/dev/rtc0'

> 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.

So this split is not a complete fix but only a work around for some
cases at best.

Maybe there isn't even a problem?

Best regards
Uwe
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: rtc: fix chardev initialization races
       [not found]   ` <nycvar.YFH.7.76.1805221407100.27054@cbobk.fhfr.pm>
@ 2018-05-22 13:39     ` Uwe Kleine-König
  2018-05-22 15:33       ` Jiri Kosina
  0 siblings, 1 reply; 4+ messages in thread
From: Uwe Kleine-König @ 2018-05-22 13:39 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Ales Novak, a.zummo, akpm, rtc-linux, linux-kernel, linux-rtc,
	Alexandre Belloni

[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/  |

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: rtc: fix chardev initialization races
  2018-05-22 13:39     ` Uwe Kleine-König
@ 2018-05-22 15:33       ` Jiri Kosina
  2018-09-04 22:02         ` Alexandre Belloni
  0 siblings, 1 reply; 4+ messages in thread
From: Jiri Kosina @ 2018-05-22 15:33 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Ales Novak, a.zummo, akpm, rtc-linux, linux-kernel, linux-rtc,
	Alexandre Belloni

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: rtc: fix chardev initialization races
  2018-05-22 15:33       ` Jiri Kosina
@ 2018-09-04 22:02         ` Alexandre Belloni
  0 siblings, 0 replies; 4+ messages in thread
From: Alexandre Belloni @ 2018-09-04 22:02 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Uwe Kleine-König, Ales Novak, a.zummo, akpm, rtc-linux,
	linux-kernel, linux-rtc

On 22/05/2018 17:33:40+0200, Jiri Kosina wrote:
> 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*?
> 

I've also tried to produce it and couldn't. The whole magic happens in
kobj_lookup. Freeing the module before ends up with the application
opening the char dev getting -ENXIO. Freeing the module after
kobj_lookup is not possible and ends up with -EBUSY.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-09-04 22:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1393410793-7315-1-git-send-email-alnovak@suse.cz>
2018-05-21 12:25 ` rtc: fix chardev initialization races Uwe Kleine-König
     [not found]   ` <nycvar.YFH.7.76.1805221407100.27054@cbobk.fhfr.pm>
2018-05-22 13:39     ` Uwe Kleine-König
2018-05-22 15:33       ` Jiri Kosina
2018-09-04 22:02         ` Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).