* [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only
@ 2017-04-05 20:36 Mathias Krause
2017-04-06 15:59 ` Andy Lutomirski
2017-04-10 13:13 ` Thomas Gleixner
0 siblings, 2 replies; 13+ messages in thread
From: Mathias Krause @ 2017-04-05 20:36 UTC (permalink / raw)
To: x86
Cc: Mathias Krause, linux-kernel, Andy Lutomirski, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Roland McGrath
If either via kernel command line 'vdso32=' or via 'sysctl abi.vsyscall32'
vdso32_enabled gets set to a value below 0 or above 1, load_vdso32() won't
map the vDSO but ARCH_DLINFO_IA32 would still pass an AT_SYSINFO_EHDR
auxiliary vector, however with a NULL pointer. That'll make any program
trying to make use of it fail with a segmentation fault. At least musl
makes use of it if the kernel provides it.
Ensure vdso32_enabled gets set to valid values only to fix this corner
case.
Fixes: b0b49f2673f0 ("x86, vdso: Remove compat vdso support")
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Roland McGrath <roland@redhat.com>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
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 7853b53959cd..ca312c174d6f 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 @@ int __init sysenter_setup(void)
/* 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,
},
{}
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only
2017-04-05 20:36 [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only Mathias Krause
@ 2017-04-06 15:59 ` Andy Lutomirski
2017-04-10 13:13 ` Thomas Gleixner
1 sibling, 0 replies; 13+ messages in thread
From: Andy Lutomirski @ 2017-04-06 15:59 UTC (permalink / raw)
To: Mathias Krause
Cc: X86 ML, linux-kernel, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Roland McGrath
On Wed, Apr 5, 2017 at 1:36 PM, Mathias Krause <minipli@googlemail.com> wrote:
> If either via kernel command line 'vdso32=' or via 'sysctl abi.vsyscall32'
> vdso32_enabled gets set to a value below 0 or above 1, load_vdso32() won't
> map the vDSO but ARCH_DLINFO_IA32 would still pass an AT_SYSINFO_EHDR
> auxiliary vector, however with a NULL pointer. That'll make any program
> trying to make use of it fail with a segmentation fault. At least musl
> makes use of it if the kernel provides it.
>
> Ensure vdso32_enabled gets set to valid values only to fix this corner
> case.
Acked-by: Andy Lutomirski <luto@kernel.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only
2017-04-05 20:36 [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only Mathias Krause
2017-04-06 15:59 ` Andy Lutomirski
@ 2017-04-10 13:13 ` Thomas Gleixner
2017-04-10 13:41 ` Thomas Gleixner
1 sibling, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 13:13 UTC (permalink / raw)
To: Mathias Krause
Cc: x86, linux-kernel, Andy Lutomirski, Ingo Molnar, H. Peter Anvin,
Roland McGrath
On Wed, 5 Apr 2017, Mathias Krause wrote:
> @@ -62,13 +64,18 @@ int __init sysenter_setup(void)
> /* 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,
This is still bustable. Let's start with: vdso32_enabled = false
arch_setup_additional_pages()
--> No mapping
sysctl.vsysscall32()
--> vdso32_enabled = true
create_elf_tables()
if (vdso32_enabled) {
--> Add VDSO entry with NULL pointer
The vdso map code needs to store a flag in current which can be checked in
ARCH_DLINFO_IA32.
Thanks,
tglx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only
2017-04-10 13:13 ` Thomas Gleixner
@ 2017-04-10 13:41 ` Thomas Gleixner
0 siblings, 0 replies; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 13:41 UTC (permalink / raw)
To: Mathias Krause
Cc: x86, linux-kernel, Andy Lutomirski, Ingo Molnar, H. Peter Anvin,
Roland McGrath
On Mon, 10 Apr 2017, Thomas Gleixner wrote:
> On Wed, 5 Apr 2017, Mathias Krause wrote:
> > @@ -62,13 +64,18 @@ int __init sysenter_setup(void)
> > /* 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,
>
> This is still bustable. Let's start with: vdso32_enabled = false
>
> arch_setup_additional_pages()
> --> No mapping
>
> sysctl.vsysscall32()
> --> vdso32_enabled = true
>
> create_elf_tables()
> if (vdso32_enabled) {
> --> Add VDSO entry with NULL pointer
>
> The vdso map code needs to store a flag in current which can be checked in
> ARCH_DLINFO_IA32.
It's ways simpler. Patch below.
Thanks,
tglx
--- 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); \
} \
^ permalink raw reply [flat|nested] 13+ messages in thread
* [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup
@ 2017-04-10 15:14 Thomas Gleixner
2017-04-10 15:14 ` [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only Thomas Gleixner
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 15:14 UTC (permalink / raw)
To: LKML; +Cc: x86, Peter Zijlstra, Andy Lutomirski, Mathias Krause
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only
2017-04-10 15:14 [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup Thomas Gleixner
@ 2017-04-10 15:14 ` Thomas Gleixner
2017-04-10 16:34 ` [tip:x86/urgent] " tip-bot for Mathias Krause
2017-04-10 15:14 ` [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Thomas Gleixner
2017-04-10 15:14 ` [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Thomas Gleixner
2 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 15:14 UTC (permalink / raw)
To: LKML
Cc: x86, Peter Zijlstra, Andy Lutomirski, Mathias Krause,
Roland McGrath, stable
[-- Attachment #1: x86vdso_Ensure_vdso32_enabled_gets_set_to_valid_values_only.patch --]
[-- Type: text/plain, Size: 1870 bytes --]
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,
},
{}
};
^ permalink raw reply [flat|nested] 13+ messages in thread
* [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup
2017-04-10 15:14 [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup Thomas Gleixner
2017-04-10 15:14 ` [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only Thomas Gleixner
@ 2017-04-10 15:14 ` Thomas Gleixner
2017-04-10 15:56 ` Andy Lutomirski
2017-04-10 16:34 ` [tip:x86/urgent] " tip-bot for Thomas Gleixner
2017-04-10 15:14 ` [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Thomas Gleixner
2 siblings, 2 replies; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 15:14 UTC (permalink / raw)
To: LKML; +Cc: x86, Peter Zijlstra, Andy Lutomirski, Mathias Krause, stable
[-- Attachment #1: x86-vdso--Plug-race-between-mapping-and-ELF-header-setup.patch --]
[-- Type: text/plain, Size: 1131 bytes --]
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); \
} \
^ permalink raw reply [flat|nested] 13+ messages in thread
* [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling
2017-04-10 15:14 [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup Thomas Gleixner
2017-04-10 15:14 ` [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only Thomas Gleixner
2017-04-10 15:14 ` [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Thomas Gleixner
@ 2017-04-10 15:14 ` Thomas Gleixner
2017-04-10 15:55 ` Andy Lutomirski
2 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 15:14 UTC (permalink / raw)
To: LKML; +Cc: x86, Peter Zijlstra, Andy Lutomirski, Mathias Krause
[-- Attachment #1: x86-vdso--Sanitize-vdso*_enabled-handling.patch --]
[-- Type: text/plain, Size: 4587 bytes --]
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;
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling
2017-04-10 15:14 ` [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Thomas Gleixner
@ 2017-04-10 15:55 ` Andy Lutomirski
2017-04-10 16:25 ` Thomas Gleixner
0 siblings, 1 reply; 13+ messages in thread
From: Andy Lutomirski @ 2017-04-10 15:55 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: LKML, X86 ML, Peter Zijlstra, Mathias Krause
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup
2017-04-10 15:14 ` [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Thomas Gleixner
@ 2017-04-10 15:56 ` Andy Lutomirski
2017-04-10 16:34 ` [tip:x86/urgent] " tip-bot for Thomas Gleixner
1 sibling, 0 replies; 13+ messages in thread
From: Andy Lutomirski @ 2017-04-10 15:56 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: LKML, X86 ML, Peter Zijlstra, Mathias Krause, stable
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling
2017-04-10 15:55 ` Andy Lutomirski
@ 2017-04-10 16:25 ` Thomas Gleixner
0 siblings, 0 replies; 13+ messages in thread
From: Thomas Gleixner @ 2017-04-10 16:25 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: LKML, X86 ML, Peter Zijlstra, Mathias Krause
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [tip:x86/urgent] x86/vdso: Ensure vdso32_enabled gets set to valid values only
2017-04-10 15:14 ` [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only Thomas Gleixner
@ 2017-04-10 16:34 ` tip-bot for Mathias Krause
0 siblings, 0 replies; 13+ messages in thread
From: tip-bot for Mathias Krause @ 2017-04-10 16:34 UTC (permalink / raw)
To: linux-tip-commits
Cc: minipli, linux-kernel, peterz, mingo, luto, hpa, tglx, roland
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,
},
{}
};
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [tip:x86/urgent] x86/vdso: Plug race between mapping and ELF header setup
2017-04-10 15:14 ` [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Thomas Gleixner
2017-04-10 15:56 ` Andy Lutomirski
@ 2017-04-10 16:34 ` tip-bot for Thomas Gleixner
1 sibling, 0 replies; 13+ messages in thread
From: tip-bot for Thomas Gleixner @ 2017-04-10 16:34 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, peterz, minipli, hpa, luto, tglx, mingo
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); \
} \
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-04-10 17:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05 20:36 [PATCH] x86/vdso: ensure vdso32_enabled gets set to valid values only Mathias Krause
2017-04-06 15:59 ` Andy Lutomirski
2017-04-10 13:13 ` Thomas Gleixner
2017-04-10 13:41 ` Thomas Gleixner
2017-04-10 15:14 [patch 0/3] x86/vdso: Fix enable inconsistencies and cleanup Thomas Gleixner
2017-04-10 15:14 ` [patch 1/3] x86/vdso: Ensure vdso32_enabled gets set to valid values only Thomas Gleixner
2017-04-10 16:34 ` [tip:x86/urgent] " tip-bot for Mathias Krause
2017-04-10 15:14 ` [patch 2/3] x86/vdso: Plug race between mapping and ELF header setup Thomas Gleixner
2017-04-10 15:56 ` Andy Lutomirski
2017-04-10 16:34 ` [tip:x86/urgent] " tip-bot for Thomas Gleixner
2017-04-10 15:14 ` [patch 3/3] x86/vdso: Sanitize vdso*_enabled handling Thomas Gleixner
2017-04-10 15:55 ` Andy Lutomirski
2017-04-10 16:25 ` Thomas Gleixner
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).