Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620014 > unrolled thread

[patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup

Back to article view | Back to linux.kernel

Contents

  [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup Thomas Gleixner <tglx@linutronix.de> - 2017-04-10 17:20 +0200
    [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Thomas Gleixner <tglx@linutronix.de> - 2017-04-10 17:20 +0200
      Re: [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Andy Lutomirski <luto@amacapital.net> - 2017-04-10 18:00 +0200
      [tip:x86/urgent] x86/vdso: Plug race between mapping and ELF header  setup tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-10 20:00 +0200
    [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Thomas Gleixner <tglx@linutronix.de> - 2017-04-10 17:20 +0200
      Re: [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Andy Lutomirski <luto@amacapital.net> - 2017-04-10 18:00 +0200
        Re: [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Thomas Gleixner <tglx@linutronix.de> - 2017-04-10 18:30 +0200
    [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values  only Thomas Gleixner <tglx@linutronix.de> - 2017-04-10 17:20 +0200
      [tip:x86/urgent] x86/vdso: Ensure vdso32_enabled gets set to valid  values only tip-bot for Mathias Krause <tipbot@zytor.com> - 2017-04-10 18:40 +0200

#1620014 — [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup

The following series is a collection of Mathias enable fix, the plug of the
sysctl race and a cleanup of the vdso*_enable handling.

Thanks,

	tglx

[toc] | [next] | [standalone]

#1620019 — [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup

The vsyscall32 sysctl can racy against a concurrent fork when it switches
from disabled to enabled:

    arch_setup_additional_pages()
	if (vdso32_enabled)
           --> No mapping
                                        sysctl.vsysscall32()
                                          --> vdso32_enabled = true
    create_elf_tables()
      ARCH_DLINFO_IA32
        if (vdso32_enabled) {
           --> Add VDSO entry with NULL pointer

Make ARCH_DLINFO_IA32 check whether the VDSO mapping has been set up for
the newly forked process or not.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: stable@vger.kernel.org
---
 arch/x86/include/asm/elf.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -287,7 +287,7 @@ struct task_struct;
 
 #define	ARCH_DLINFO_IA32						\
 do {									\
-	if (vdso32_enabled) {						\
+	if (VDSO_CURRENT_BASE) {					\
 		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
 	}								\

[toc] | [prev] | [next] | [standalone]

#1620143 — Re: [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup

On Mon, Apr 10, 2017 at 8:14 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> The vsyscall32 sysctl can racy against a concurrent fork when it switches
> from disabled to enabled:
>
>     arch_setup_additional_pages()
>         if (vdso32_enabled)
>            --> No mapping
>                                         sysctl.vsysscall32()
>                                           --> vdso32_enabled = true
>     create_elf_tables()
>       ARCH_DLINFO_IA32
>         if (vdso32_enabled) {
>            --> Add VDSO entry with NULL pointer
>
> Make ARCH_DLINFO_IA32 check whether the VDSO mapping has been set up for
> the newly forked process or not.

Acked-by: Andy Lutomirski <luto@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1620558 — [tip:x86/urgent] x86/vdso: Plug race between mapping and ELF header setup

Commit-ID:  6fdc6dd90272ce7e75d744f71535cfbd8d77da81
Gitweb:     http://git.kernel.org/tip/6fdc6dd90272ce7e75d744f71535cfbd8d77da81
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 10 Apr 2017 17:14:28 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 10 Apr 2017 18:31:41 +0200

x86/vdso: Plug race between mapping and ELF header setup

The vsyscall32 sysctl can racy against a concurrent fork when it switches
from disabled to enabled:

    arch_setup_additional_pages()
	if (vdso32_enabled)
           --> No mapping
                                        sysctl.vsysscall32()
                                          --> vdso32_enabled = true
    create_elf_tables()
      ARCH_DLINFO_IA32
        if (vdso32_enabled) {
           --> Add VDSO entry with NULL pointer

Make ARCH_DLINFO_IA32 check whether the VDSO mapping has been set up for
the newly forked process or not.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/20170410151723.602367196@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/include/asm/elf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 9d49c18..3762536 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -287,7 +287,7 @@ struct task_struct;
 
 #define	ARCH_DLINFO_IA32						\
 do {									\
-	if (vdso32_enabled) {						\
+	if (VDSO_CURRENT_BASE) {					\
 		NEW_AUX_ENT(AT_SYSINFO,	VDSO_ENTRY);			\
 		NEW_AUX_ENT(AT_SYSINFO_EHDR, VDSO_CURRENT_BASE);	\
 	}								\

[toc] | [prev] | [next] | [standalone]

#1620020 — [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling

vdso*_enabled is conceptionally a boolean. Though there are leftovers from
the original implementation which required an int. That's just confusing
and error prone.

Convert evrything to boolean and correct stale comments.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mathias Krause <minipli@googlemail.com>
---
 arch/x86/entry/vdso/vdso32-setup.c |   40 +++++++++++++++++++++----------------
 arch/x86/entry/vdso/vma.c          |   12 +++++++----
 arch/x86/include/asm/elf.h         |    4 +--
 arch/x86/um/vdso/vma.c             |    4 +--
 4 files changed, 35 insertions(+), 25 deletions(-)

--- a/arch/x86/entry/vdso/vdso32-setup.c
+++ b/arch/x86/entry/vdso/vdso32-setup.c
@@ -24,27 +24,19 @@
  * Should the kernel map a VDSO page into processes and pass its
  * address down to glibc upon exec()?
  */
-unsigned int __read_mostly vdso32_enabled = VDSO_DEFAULT;
+bool __read_mostly vdso32_enabled = VDSO_DEFAULT;
 
 static int __init vdso32_setup(char *s)
 {
-	vdso32_enabled = simple_strtoul(s, NULL, 0);
-
-	if (vdso32_enabled > 1) {
-		pr_warn("vdso32 values other than 0 and 1 are no longer allowed; vdso disabled\n");
-		vdso32_enabled = 0;
-	}
+	unsigned int ena = simple_strtoul(s, NULL, 0);
 
+	if (ena > 1)
+		pr_warn("vdso32=%u out of range [0-1], disabling vdso32\n", ena);
+	vdso32_enabled = ena == 1;
 	return 1;
 }
-
-/*
- * For consistency, the argument vdso32=[012] affects the 32-bit vDSO
- * behavior on both 64-bit and 32-bit kernels.
- * On 32-bit kernels, vdso=[012] means the same thing.
- */
+/* The vdso32 option is valid on 64bit and 32bit kernels */
 __setup("vdso32=", vdso32_setup);
-
 #ifdef CONFIG_X86_32
 __setup_param("vdso=", vdso_setup, vdso32_setup, 0);
 #endif
@@ -66,14 +58,27 @@ subsys_initcall(sysenter_setup);
 
 static const int zero;
 static const int one = 1;
+static int vdso32_sysctl;
+
+static int vdso_update_handler(struct ctl_table *table, int write,
+			       void __user *buffer, size_t *lenp,
+			       loff_t *ppos)
+{
+	int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+
+	if (ret || !write)
+		return ret;
+	vdso32_enabled = !!vdso32_sysctl;
+	return 0;
+}
 
 static struct ctl_table abi_table2[] = {
 	{
 		.procname	= "vsyscall32",
-		.data		= &vdso32_enabled,
-		.maxlen		= sizeof(int),
+		.data		= &vdso32_sysctl,
+		.maxlen		= sizeof(bool),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= vdso_update_handler,
 		.extra1		= (int *)&zero,
 		.extra2		= (int *)&one,
 	},
@@ -91,6 +96,7 @@ static struct ctl_table abi_root_table2[
 
 static __init int ia32_binfmt_init(void)
 {
+	vdso32_sysctl = vdso32_enabled;
 	register_sysctl_table(abi_root_table2);
 	return 0;
 }
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -24,7 +24,7 @@
 #include <asm/cpufeature.h>
 
 #if defined(CONFIG_X86_64)
-unsigned int __read_mostly vdso64_enabled = 1;
+bool __read_mostly vdso64_enabled = true;
 #endif
 
 void __init init_vdso_image(const struct vdso_image *image)
@@ -279,7 +279,7 @@ int map_vdso_once(const struct vdso_imag
 #if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
 static int load_vdso32(void)
 {
-	if (vdso32_enabled != 1)  /* Other values all mean "disabled" */
+	if (!vdso32_enabled)
 		return 0;
 
 	return map_vdso(&vdso_image_32, 0);
@@ -323,8 +323,12 @@ int arch_setup_additional_pages(struct l
 #ifdef CONFIG_X86_64
 static __init int vdso_setup(char *s)
 {
-	vdso64_enabled = simple_strtoul(s, NULL, 0);
-	return 0;
+	unsigned int ena = simple_strtoul(s, NULL, 0);
+
+	if (ena > 1)
+		pr_warn("vdso=%u out of range [0-1], disabling vdso\n",	ena);
+	vdso64_enabled = ena == 1;
+	return 1;
 }
 __setup("vdso=", vdso_setup);
 #endif
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -76,10 +76,10 @@ typedef struct user_fxsr_struct elf_fpxr
 #include <asm/vdso.h>
 
 #ifdef CONFIG_X86_64
-extern unsigned int vdso64_enabled;
+extern bool vdso64_enabled;
 #endif
 #if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
-extern unsigned int vdso32_enabled;
+extern bool vdso32_enabled;
 #endif
 
 /*
--- a/arch/x86/um/vdso/vma.c
+++ b/arch/x86/um/vdso/vma.c
@@ -13,7 +13,7 @@
 #include <asm/elf.h>
 #include <linux/init.h>
 
-static unsigned int __read_mostly vdso_enabled = 1;
+static bool __read_mostly vdso_enabled = true;
 unsigned long um_vdso_addr;
 
 extern unsigned long task_size;
@@ -47,7 +47,7 @@ static int __init init_vdso(void)
 
 oom:
 	printk(KERN_ERR "Cannot allocate vdso\n");
-	vdso_enabled = 0;
+	vdso_enabled = false;
 
 	return -ENOMEM;
 }

[toc] | [prev] | [next] | [standalone]

#1620132 — Re: [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling

On Mon, Apr 10, 2017 at 8:14 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> +static int vdso32_sysctl;
> +
> +static int vdso_update_handler(struct ctl_table *table, int write,
> +                              void __user *buffer, size_t *lenp,
> +                              loff_t *ppos)
> +{
> +       int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);

Using a global for the temporary is gross and mildly racy.  I would
just open-code the conversion.  Or, even better, add proc_dobool().

I'm not convinced that the current patch is a cleanup.

--Andy

[toc] | [prev] | [next] | [standalone]

#1620218 — Re: [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling

On Mon, 10 Apr 2017, Andy Lutomirski wrote:

> On Mon, Apr 10, 2017 at 8:14 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > +static int vdso32_sysctl;
> > +
> > +static int vdso_update_handler(struct ctl_table *table, int write,
> > +                              void __user *buffer, size_t *lenp,
> > +                              loff_t *ppos)
> > +{
> > +       int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> 
> Using a global for the temporary is gross and mildly racy.  I would
> just open-code the conversion.  Or, even better, add proc_dobool().

Dammit, thought about it and then got lazy. :(

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1620023 — [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only

From: Mathias Krause <minipli@googlemail.com>

vdso_enabled can be set to arbitrary integer values via the kernel command
line 'vdso32=' parameter or via 'sysctl abi.vsyscall32'.

load_vdso32() only maps VDSO if vdso_enabled == 1, but ARCH_DLINFO_IA32
merily checks for vdso_enabled != 0. As a consequence the AT_SYSINFO_EHDR
auxiliary vector for the VDSO_ENTRY is emitted with a NULL pointer which
causes a segfault when the application tries to use the VDSO.

Restrict the valid arguments on the command line and the sysctl to 0 and 1.

Fixes: b0b49f2673f0 ("x86, vdso: Remove compat vdso support")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Cc: Roland McGrath <roland@redhat.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/1491424561-7187-1-git-send-email-minipli@googlemail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/entry/vdso/vdso32-setup.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--- a/arch/x86/entry/vdso/vdso32-setup.c
+++ b/arch/x86/entry/vdso/vdso32-setup.c
@@ -30,8 +30,10 @@ static int __init vdso32_setup(char *s)
 {
 	vdso32_enabled = simple_strtoul(s, NULL, 0);
 
-	if (vdso32_enabled > 1)
+	if (vdso32_enabled > 1) {
 		pr_warn("vdso32 values other than 0 and 1 are no longer allowed; vdso disabled\n");
+		vdso32_enabled = 0;
+	}
 
 	return 1;
 }
@@ -62,13 +64,18 @@ subsys_initcall(sysenter_setup);
 /* Register vsyscall32 into the ABI table */
 #include <linux/sysctl.h>
 
+static const int zero;
+static const int one = 1;
+
 static struct ctl_table abi_table2[] = {
 	{
 		.procname	= "vsyscall32",
 		.data		= &vdso32_enabled,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= (int *)&zero,
+		.extra2		= (int *)&one,
 	},
 	{}
 };

[toc] | [prev] | [next] | [standalone]

#1620238 — [tip:x86/urgent] x86/vdso: Ensure vdso32_enabled gets set to valid values only

Commit-ID:  c06989da39cdb10604d572c8c7ea8c8c97f3c483
Gitweb:     http://git.kernel.org/tip/c06989da39cdb10604d572c8c7ea8c8c97f3c483
Author:     Mathias Krause <minipli@googlemail.com>
AuthorDate: Mon, 10 Apr 2017 17:14:27 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 10 Apr 2017 18:31:41 +0200

x86/vdso: Ensure vdso32_enabled gets set to valid values only

vdso_enabled can be set to arbitrary integer values via the kernel command
line 'vdso32=' parameter or via 'sysctl abi.vsyscall32'.

load_vdso32() only maps VDSO if vdso_enabled == 1, but ARCH_DLINFO_IA32
merily checks for vdso_enabled != 0. As a consequence the AT_SYSINFO_EHDR
auxiliary vector for the VDSO_ENTRY is emitted with a NULL pointer which
causes a segfault when the application tries to use the VDSO.

Restrict the valid arguments on the command line and the sysctl to 0 and 1.

Fixes: b0b49f2673f0 ("x86, vdso: Remove compat vdso support")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: stable@vger.kernel.org
Cc: Roland McGrath <roland@redhat.com>
Link: http://lkml.kernel.org/r/1491424561-7187-1-git-send-email-minipli@googlemail.com
Link: http://lkml.kernel.org/r/20170410151723.518412863@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/entry/vdso/vdso32-setup.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/vdso/vdso32-setup.c b/arch/x86/entry/vdso/vdso32-setup.c
index 7853b53..3f9d1a8 100644
--- a/arch/x86/entry/vdso/vdso32-setup.c
+++ b/arch/x86/entry/vdso/vdso32-setup.c
@@ -30,8 +30,10 @@ static int __init vdso32_setup(char *s)
 {
 	vdso32_enabled = simple_strtoul(s, NULL, 0);
 
-	if (vdso32_enabled > 1)
+	if (vdso32_enabled > 1) {
 		pr_warn("vdso32 values other than 0 and 1 are no longer allowed; vdso disabled\n");
+		vdso32_enabled = 0;
+	}
 
 	return 1;
 }
@@ -62,13 +64,18 @@ subsys_initcall(sysenter_setup);
 /* Register vsyscall32 into the ABI table */
 #include <linux/sysctl.h>
 
+static const int zero;
+static const int one = 1;
+
 static struct ctl_table abi_table2[] = {
 	{
 		.procname	= "vsyscall32",
 		.data		= &vdso32_enabled,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= (int *)&zero,
+		.extra2		= (int *)&one,
 	},
 	{}
 };

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web