| 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
| ||
|
Message-Id: <201112082356.00747.rjw@sisk.pl>
Date: Thu, 8 Dec 2011 23:56:00 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc: gregkh@...e.de, dhowells@...hat.com, eparis@...hat.com,
kay.sievers@...y.org, jmorris@...ei.org, tj@...nel.org,
bp@...64.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, namhyung@...il.com
Subject: Re: [PATCH v2] PM / Usermodehelper: Fix freezer failures due to racy usermodehelper_is_disabled()
On Thursday, December 08, 2011, Srivatsa S. Bhat wrote:
> Commit a144c6a (PM: Print a warning if firmware is requested when tasks
> are frozen) introduced usermodehelper_is_disabled() to warn and exit
> immediately if firmware is requested when usermodehelpers are disabled.
>
> However, it is racy. Consider the following scenario, currently used in
> drivers/base/firmware_class.c:
>
> ...
> if (usermodehelper_is_disabled())
> goto out;
>
> /* Do actual work */
> ...
>
> out:
> return err;
>
> Nothing prevents someone from disabling usermodehelpers just after the check
> in the 'if' condition, which means that it is quite possible to try doing the
> "actual work" with usermodehelpers disabled, leading to undesirable
> consequences.
>
> In particular, this race condition in _request_firmware() causes task freezing
> failures whenever suspend/hibernation is in progress because, it wrongly waits
> to get the firmware/microcode image from userspace when actually the
> usermodehelpers are disabled or userspace has been frozen.
> Some of the example scenarios that cause freezing failures due to this race
> are those that depend on userspace via request_firmware(), such as x86
> microcode module initialization and microcode image reload.
>
> Previous discussions about this issue can be found at:
> http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591
>
> This patch adds proper synchronization to fix this issue.
>
> It is to be noted that this patchset fixes the freezing failures but doesn't
> remove the warnings. IOW, it does not attempt to add explicit synchronization
> to x86 microcode driver to avoid requesting microcode image at inopportune
> moments. Because, the warnings were introduced to highlight such cases, in the
> first place. And we need not silence the warnings, since we take care of the
> *real* problem (freezing failure) and hence, after that, the warnings are
> pretty harmless anyway.
>
> v2: Used rwsemaphores to implement the synchronization, as Tejun suggested.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
Applied to linux-pm/linux-next.
Thanks,
Rafael
> ---
>
> drivers/base/firmware_class.c | 4 ++++
> include/linux/kmod.h | 2 ++
> kernel/kmod.c | 23 ++++++++++++++++++++++-
> 3 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 06ed6b4..d5585da 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -534,6 +534,8 @@ static int _request_firmware(const struct firmware **firmware_p,
> return 0;
> }
>
> + read_lock_usermodehelper();
> +
> if (WARN_ON(usermodehelper_is_disabled())) {
> dev_err(device, "firmware: %s will not be loaded\n", name);
> retval = -EBUSY;
> @@ -572,6 +574,8 @@ static int _request_firmware(const struct firmware **firmware_p,
> fw_destroy_instance(fw_priv);
>
> out:
> + read_unlock_usermodehelper();
> +
> if (retval) {
> release_firmware(firmware);
> *firmware_p = NULL;
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index b16f653..722f477 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -117,5 +117,7 @@ extern void usermodehelper_init(void);
> extern int usermodehelper_disable(void);
> extern void usermodehelper_enable(void);
> extern bool usermodehelper_is_disabled(void);
> +extern void read_lock_usermodehelper(void);
> +extern void read_unlock_usermodehelper(void);
>
> #endif /* __LINUX_KMOD_H__ */
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 2142687..2b656fe 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -34,6 +34,7 @@
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/resource.h>
> +#include <linux/rwsem.h>
> #include <asm/uaccess.h>
>
> #include <trace/events/module.h>
> @@ -48,6 +49,7 @@ static struct workqueue_struct *khelper_wq;
> static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
> static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
> static DEFINE_SPINLOCK(umh_sysctl_lock);
> +static DECLARE_RWSEM(umhelper_sem);
>
> #ifdef CONFIG_MODULES
>
> @@ -273,6 +275,7 @@ static void __call_usermodehelper(struct work_struct *work)
> * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
> * (used for preventing user land processes from being created after the user
> * land has been frozen during a system-wide hibernation or suspend operation).
> + * Should always be manipulated under umhelper_sem acquired for write.
> */
> static int usermodehelper_disabled = 1;
>
> @@ -291,6 +294,18 @@ static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
> */
> #define RUNNING_HELPERS_TIMEOUT (5 * HZ)
>
> +void read_lock_usermodehelper(void)
> +{
> + down_read(&umhelper_sem);
> +}
> +EXPORT_SYMBOL_GPL(read_lock_usermodehelper);
> +
> +void read_unlock_usermodehelper(void)
> +{
> + up_read(&umhelper_sem);
> +}
> +EXPORT_SYMBOL_GPL(read_unlock_usermodehelper);
> +
> /**
> * usermodehelper_disable - prevent new helpers from being started
> */
> @@ -298,8 +313,10 @@ int usermodehelper_disable(void)
> {
> long retval;
>
> + down_write(&umhelper_sem);
> usermodehelper_disabled = 1;
> - smp_mb();
> + up_write(&umhelper_sem);
> +
> /*
> * From now on call_usermodehelper_exec() won't start any new
> * helpers, so it is sufficient if running_helpers turns out to
> @@ -312,7 +329,9 @@ int usermodehelper_disable(void)
> if (retval)
> return 0;
>
> + down_write(&umhelper_sem);
> usermodehelper_disabled = 0;
> + up_write(&umhelper_sem);
> return -EAGAIN;
> }
>
> @@ -321,7 +340,9 @@ int usermodehelper_disable(void)
> */
> void usermodehelper_enable(void)
> {
> + down_write(&umhelper_sem);
> usermodehelper_disabled = 0;
> + up_write(&umhelper_sem);
> }
>
> /**
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
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