| 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