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
| ||
|
Date: Wed, 20 Nov 2013 16:40:31 -0500 From: Boris Ostrovsky <boris.ostrovsky@...cle.com> To: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com> CC: Ian.Campbell@...rix.com, xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org, JBeulich@...e.com, david.vrabel@...rix.com Subject: Re: [PATCH 3/4] xen/manage: Guard against user-space initiated poweroff and XenBus. On 11/08/2013 12:38 PM, Konrad Rzeszutek Wilk wrote: > There is a race case where the user does 'poweroff' > and at the same time the system admin does 'xl shutdown'. > > Depending on the race, the system_state will be SYSTEM_RUNNING or > SYSTEM_POWER_OFF. If SYSTEM_RUNNING we just end up making > a duplicate call to 'poweroff' (while it is running). > > That will fail or execute (And if executed then it will be > stuck in the reboot_mutex mutex). But nobody will care b/c the > machine is in poweroff sequence. > > If the system_state is SYSTEM_POWER_OFF then we end up making > a duplicate call to kernel_power_off. There is no locking > there so we walk in the same steps as what 'poweroff' > has been doing. > > The code in kernel/reboot.c has a mutex guarding against multiple > 'poweroff' operations. But not against the kernel 'orderly_poweroff'. > > As such, lets detect this so that we don't invoke orderly_poweroff > if the user had initiated a poweroff. > > This is code by changing the 'shutting_down' to an atomic and > having a reboot notifier. If the 'shutting_down' is set to anything > but SHUTDOWN_INVALID the XenBus handler will not run. > > That is exactly what we do in the reboot notifier - we set the > 'shutting_down' to SHUTDOWN_POWEROFF. > > The reason we change the 'shutting_down' to an atomic is that > the changes to said variable were normally guarded by the XenBus > mutex - "xenwatch_mutex" - guarantting only one caller changing > shutting_down. Since we have now the reboot notifier we have > another user of this variable. Surfacing the 'xenwatch_mutex' > out of XenBus is not a nice way of doing it. Having the > variable however be atomic solves the problem easily. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com> > [v2: Don't expose xenwatch_mutex, add comments] Reviewed-by: Boris Ostrovsky <boris.ostrovsky@...cle.com> > --- > drivers/xen/manage.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 42 insertions(+), 9 deletions(-) > > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c > index 3f8496c..323703a 100644 > --- a/drivers/xen/manage.c > +++ b/drivers/xen/manage.c > @@ -36,8 +36,16 @@ enum shutdown_state { > SHUTDOWN_HALT = 4, > }; > > -/* Ignore multiple shutdown requests. */ > -static enum shutdown_state shutting_down = SHUTDOWN_INVALID; > +/* Ignore multiple shutdown requests. There are two potential race conditions: > + * - Multiple XenStore 'shutdown' requests. We don't want to run any off > + * the callbacks in parallel. > + * - In progress 'poweroff' (initiated inside the guest) and a XenStore > + * 'shutdown' request. If the poweroff has transitioned 'system_state' to > + * SYSTEM_POWER_OFF we do not want to call orderly_poweroff. 'system_state' > + * is not SMP safe so we depend on reboot notifiers to set 'shutting_down' > + * so that we will ignore XenBus shutdown requests. > + */ > +static atomic_t shutting_down = ATOMIC_INIT(SHUTDOWN_INVALID); > > struct suspend_info { > int cancelled; > @@ -109,7 +117,7 @@ static void do_suspend(void) > int err; > struct suspend_info si; > > - shutting_down = SHUTDOWN_SUSPEND; > + atomic_set(&shutting_down, SHUTDOWN_SUSPEND); > > #ifdef CONFIG_PREEMPT > /* If the kernel is preemptible, we need to freeze all the processes > @@ -173,7 +181,7 @@ out_thaw: > thaw_processes(); > out: > #endif > - shutting_down = SHUTDOWN_INVALID; > + atomic_set(&shutting_down, SHUTDOWN_INVALID); > } > #endif /* CONFIG_HIBERNATE_CALLBACKS */ > > @@ -184,7 +192,7 @@ struct shutdown_handler { > > static void do_poweroff(void) > { > - shutting_down = SHUTDOWN_POWEROFF; > + atomic_set(&shutting_down, SHUTDOWN_POWEROFF); > switch (system_state) { > case SYSTEM_BOOTING: > orderly_poweroff(true); > @@ -201,7 +209,7 @@ static void do_poweroff(void) > > static void do_reboot(void) > { > - shutting_down = SHUTDOWN_POWEROFF; /* ? */ > + atomic_set(&shutting_down, SHUTDOWN_POWEROFF); /* ? */ > ctrl_alt_del(); > } > > @@ -222,7 +230,7 @@ static void shutdown_handler(struct xenbus_watch *watch, > }; > static struct shutdown_handler *handler; > > - if (shutting_down != SHUTDOWN_INVALID) > + if (atomic_read(&shutting_down) != SHUTDOWN_INVALID) > return; > > again: > @@ -256,12 +264,29 @@ static void shutdown_handler(struct xenbus_watch *watch, > handler->cb(); > } else { > pr_info("Ignoring shutdown request: %s\n", str); > - shutting_down = SHUTDOWN_INVALID; > + atomic_set(&shutting_down, SHUTDOWN_INVALID); > } > > kfree(str); > } > +/* > + * This function is called when the system is being rebooted. > + */ > +static int > +xen_system_reboot(struct notifier_block *nb, unsigned long event, void *unused) > +{ > + switch (event) { > + case SYS_RESTART: > + case SYS_HALT: > + case SYS_POWER_OFF: > + atomic_set(&shutting_down, SHUTDOWN_POWEROFF); > + break; > + default: > + break; > + } > > + return NOTIFY_DONE; > +} > #ifdef CONFIG_MAGIC_SYSRQ > static void sysrq_handler(struct xenbus_watch *watch, const char **vec, > unsigned int len) > @@ -302,6 +327,10 @@ static struct xenbus_watch shutdown_watch = { > .callback = shutdown_handler > }; > > +static struct notifier_block xen_shutdown_notifier = { > + .notifier_call = xen_system_reboot, > +}; > + > static int setup_shutdown_watcher(void) > { > int err; > @@ -319,7 +348,11 @@ static int setup_shutdown_watcher(void) > return err; > } > #endif > - > + err = register_reboot_notifier(&xen_shutdown_notifier); > + if (err) { > + pr_warn("Failed to register shutdown notifier\n"); > + return err; > + } > return 0; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists