* [PATCH] ARM: fix randomized task_struct
@ 2017-06-30 15:34 Arnd Bergmann
2017-06-30 15:49 ` Ard Biesheuvel
0 siblings, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2017-06-30 15:34 UTC (permalink / raw)
To: Kees Cook
Cc: kernel-hardening, Ard Biesheuvel, Arnd Bergmann, Russell King,
Will Deacon, linux-arm-kernel, linux-kernel
With the new task struct randomization, we can run into a build
failure for certain random seeds:
arch/arm/kernel/entry-armv.S: Assembler messages:
arch/arm/kernel/entry-armv.S:803: Error: bad immediate value for offset (4096)
Only two constants in asm-offset.h are affected, and I'm changing
both of them here to work correctly in all configurations.
One more macro has the problem, but is currently unused, so this
removes it instead of adding complexity.
Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Fixes: c33d8b12fbbd ("task_struct: Allow randomized layout")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/arm/kernel/entry-armv.S | 5 ++++-
arch/arm/mm/proc-macros.S | 10 ++++------
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 9f157e7c51e7..db6d22b23bd8 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -797,7 +797,10 @@ ENTRY(__switch_to)
#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
ldr r7, [r2, #TI_TASK]
ldr r8, =__stack_chk_guard
- ldr r7, [r7, #TSK_STACK_CANARY]
+ .if (TSK_STACK_CANARY > PAGE_MASK)
+ add r7, r7, #TSK_STACK_CANARY & PAGE_MASK
+ .endif
+ ldr r7, [r7, #TSK_STACK_CANARY & ~PAGE_MASK]
#endif
#ifdef CONFIG_CPU_USE_DOMAINS
mcr p15, 0, r6, c3, c0, 0 @ Set domain register
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index 0d40c285bd86..2c5f2a0a708b 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -25,11 +25,6 @@
ldr \rd, [\rn, #VMA_VM_FLAGS]
.endm
- .macro tsk_mm, rd, rn
- ldr \rd, [\rn, #TI_TASK]
- ldr \rd, [\rd, #TSK_ACTIVE_MM]
- .endm
-
/*
* act_mm - get current->active_mm
*/
@@ -37,7 +32,10 @@
bic \rd, sp, #8128
bic \rd, \rd, #63
ldr \rd, [\rd, #TI_TASK]
- ldr \rd, [\rd, #TSK_ACTIVE_MM]
+ .if (TSK_ACTIVE_MM > PAGE_MASK)
+ add \rd, \rd, #TSK_ACTIVE_MM & PAGE_MASK
+ .endif
+ ldr \rd, [\rd, #TSK_ACTIVE_MM & ~PAGE_MASK]
.endm
/*
--
2.9.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: fix randomized task_struct
2017-06-30 15:34 [PATCH] ARM: fix randomized task_struct Arnd Bergmann
@ 2017-06-30 15:49 ` Ard Biesheuvel
2017-06-30 15:55 ` Russell King - ARM Linux
2017-06-30 15:55 ` Arnd Bergmann
0 siblings, 2 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2017-06-30 15:49 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kees Cook, kernel-hardening, Russell King, Will Deacon,
linux-arm-kernel, linux-kernel
> On 30 Jun 2017, at 15:34, Arnd Bergmann <arnd@arndb.de> wrote:
>
> With the new task struct randomization, we can run into a build
> failure for certain random seeds:
>
> arch/arm/kernel/entry-armv.S: Assembler messages:
> arch/arm/kernel/entry-armv.S:803: Error: bad immediate value for offset (4096)
>
> Only two constants in asm-offset.h are affected, and I'm changing
> both of them here to work correctly in all configurations.
>
> One more macro has the problem, but is currently unused, so this
> removes it instead of adding complexity.
>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Fixes: c33d8b12fbbd ("task_struct: Allow randomized layout")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> arch/arm/kernel/entry-armv.S | 5 ++++-
> arch/arm/mm/proc-macros.S | 10 ++++------
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> index 9f157e7c51e7..db6d22b23bd8 100644
> --- a/arch/arm/kernel/entry-armv.S
> +++ b/arch/arm/kernel/entry-armv.S
> @@ -797,7 +797,10 @@ ENTRY(__switch_to)
> #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
> ldr r7, [r2, #TI_TASK]
> ldr r8, =__stack_chk_guard
> - ldr r7, [r7, #TSK_STACK_CANARY]
> + .if (TSK_STACK_CANARY > PAGE_MASK)
Shouldn't this be ~PAGE_MASK?
I think
.if (TSK_STACK_CANARY & PAGE_MASK) != 0
is better and clearer as well
> + add r7, r7, #TSK_STACK_CANARY & PAGE_MASK
> + .endif
> + ldr r7, [r7, #TSK_STACK_CANARY & ~PAGE_MASK]
> #endif
> #ifdef CONFIG_CPU_USE_DOMAINS
> mcr p15, 0, r6, c3, c0, 0 @ Set domain register
> diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> index 0d40c285bd86..2c5f2a0a708b 100644
> --- a/arch/arm/mm/proc-macros.S
> +++ b/arch/arm/mm/proc-macros.S
> @@ -25,11 +25,6 @@
> ldr \rd, [\rn, #VMA_VM_FLAGS]
> .endm
>
> - .macro tsk_mm, rd, rn
> - ldr \rd, [\rn, #TI_TASK]
> - ldr \rd, [\rd, #TSK_ACTIVE_MM]
> - .endm
> -
> /*
> * act_mm - get current->active_mm
> */
> @@ -37,7 +32,10 @@
> bic \rd, sp, #8128
> bic \rd, \rd, #63
> ldr \rd, [\rd, #TI_TASK]
> - ldr \rd, [\rd, #TSK_ACTIVE_MM]
> + .if (TSK_ACTIVE_MM > PAGE_MASK)
> + add \rd, \rd, #TSK_ACTIVE_MM & PAGE_MASK
> + .endif
> + ldr \rd, [\rd, #TSK_ACTIVE_MM & ~PAGE_MASK]
> .endm
>
> /*
> --
> 2.9.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: fix randomized task_struct
2017-06-30 15:49 ` Ard Biesheuvel
@ 2017-06-30 15:55 ` Russell King - ARM Linux
2017-06-30 17:34 ` Ard Biesheuvel
2017-06-30 15:55 ` Arnd Bergmann
1 sibling, 1 reply; 5+ messages in thread
From: Russell King - ARM Linux @ 2017-06-30 15:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arnd Bergmann, Kees Cook, kernel-hardening, Will Deacon,
linux-arm-kernel, linux-kernel
On Fri, Jun 30, 2017 at 03:49:41PM +0000, Ard Biesheuvel wrote:
>
>
> > On 30 Jun 2017, at 15:34, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > With the new task struct randomization, we can run into a build
> > failure for certain random seeds:
> >
> > arch/arm/kernel/entry-armv.S: Assembler messages:
> > arch/arm/kernel/entry-armv.S:803: Error: bad immediate value for offset (4096)
> >
> > Only two constants in asm-offset.h are affected, and I'm changing
> > both of them here to work correctly in all configurations.
> >
> > One more macro has the problem, but is currently unused, so this
> > removes it instead of adding complexity.
> >
> > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Fixes: c33d8b12fbbd ("task_struct: Allow randomized layout")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > arch/arm/kernel/entry-armv.S | 5 ++++-
> > arch/arm/mm/proc-macros.S | 10 ++++------
> > 2 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
> > index 9f157e7c51e7..db6d22b23bd8 100644
> > --- a/arch/arm/kernel/entry-armv.S
> > +++ b/arch/arm/kernel/entry-armv.S
> > @@ -797,7 +797,10 @@ ENTRY(__switch_to)
> > #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
> > ldr r7, [r2, #TI_TASK]
> > ldr r8, =__stack_chk_guard
> > - ldr r7, [r7, #TSK_STACK_CANARY]
> > + .if (TSK_STACK_CANARY > PAGE_MASK)
>
> Shouldn't this be ~PAGE_MASK?
>
> I think
>
> .if (TSK_STACK_CANARY & PAGE_MASK) != 0
>
> is better and clearer as well
It's not really that much clearer - what has any of this got to do with
the size of a page? Just because a definition appears to be numerically
the same, it doesn't mean it should be used!
The LDR instruction takes a maximum of a 12-bit constant. This 12-bit
constant has nothing to do with the page size; it's been that way since
the early ARMs that knew nothing about page tables.
Please instead create a LDR_IMM12_MASK or similar definition for this.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ARM: fix randomized task_struct
2017-06-30 15:55 ` Russell King - ARM Linux
@ 2017-06-30 17:34 ` Ard Biesheuvel
0 siblings, 0 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2017-06-30 17:34 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Arnd Bergmann, Kees Cook, kernel-hardening, Will Deacon,
linux-arm-kernel, linux-kernel
On 30 June 2017 at 15:55, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Jun 30, 2017 at 03:49:41PM +0000, Ard Biesheuvel wrote:
>>
>>
>> > On 30 Jun 2017, at 15:34, Arnd Bergmann <arnd@arndb.de> wrote:
>> >
>> > With the new task struct randomization, we can run into a build
>> > failure for certain random seeds:
>> >
>> > arch/arm/kernel/entry-armv.S: Assembler messages:
>> > arch/arm/kernel/entry-armv.S:803: Error: bad immediate value for offset (4096)
>> >
>> > Only two constants in asm-offset.h are affected, and I'm changing
>> > both of them here to work correctly in all configurations.
>> >
>> > One more macro has the problem, but is currently unused, so this
>> > removes it instead of adding complexity.
>> >
>> > Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Fixes: c33d8b12fbbd ("task_struct: Allow randomized layout")
>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> > ---
>> > arch/arm/kernel/entry-armv.S | 5 ++++-
>> > arch/arm/mm/proc-macros.S | 10 ++++------
>> > 2 files changed, 8 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
>> > index 9f157e7c51e7..db6d22b23bd8 100644
>> > --- a/arch/arm/kernel/entry-armv.S
>> > +++ b/arch/arm/kernel/entry-armv.S
>> > @@ -797,7 +797,10 @@ ENTRY(__switch_to)
>> > #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
>> > ldr r7, [r2, #TI_TASK]
>> > ldr r8, =__stack_chk_guard
>> > - ldr r7, [r7, #TSK_STACK_CANARY]
>> > + .if (TSK_STACK_CANARY > PAGE_MASK)
>>
>> Shouldn't this be ~PAGE_MASK?
>>
>> I think
>>
>> .if (TSK_STACK_CANARY & PAGE_MASK) != 0
>>
>> is better and clearer as well
>
> It's not really that much clearer - what has any of this got to do with
> the size of a page? Just because a definition appears to be numerically
> the same, it doesn't mean it should be used!
>
> The LDR instruction takes a maximum of a 12-bit constant. This 12-bit
> constant has nothing to do with the page size; it's been that way since
> the early ARMs that knew nothing about page tables.
>
> Please instead create a LDR_IMM12_MASK or similar definition for this.
>
Yes, good point.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ARM: fix randomized task_struct
2017-06-30 15:49 ` Ard Biesheuvel
2017-06-30 15:55 ` Russell King - ARM Linux
@ 2017-06-30 15:55 ` Arnd Bergmann
1 sibling, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2017-06-30 15:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Kees Cook, Kernel Hardening, Russell King, Will Deacon,
Linux ARM, Linux Kernel Mailing List
On Fri, Jun 30, 2017 at 5:49 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>> On 30 Jun 2017, at 15:34, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> With the new task struct randomization, we can run into a build
>> failure for certain random seeds:
>>
>> arch/arm/kernel/entry-armv.S: Assembler messages:
>> arch/arm/kernel/entry-armv.S:803: Error: bad immediate value for offset (4096)
>>
>> Only two constants in asm-offset.h are affected, and I'm changing
>> both of them here to work correctly in all configurations.
>>
>> One more macro has the problem, but is currently unused, so this
>> removes it instead of adding complexity.
>>
>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Fixes: c33d8b12fbbd ("task_struct: Allow randomized layout")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> arch/arm/kernel/entry-armv.S | 5 ++++-
>> arch/arm/mm/proc-macros.S | 10 ++++------
>> 2 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
>> index 9f157e7c51e7..db6d22b23bd8 100644
>> --- a/arch/arm/kernel/entry-armv.S
>> +++ b/arch/arm/kernel/entry-armv.S
>> @@ -797,7 +797,10 @@ ENTRY(__switch_to)
>> #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
>> ldr r7, [r2, #TI_TASK]
>> ldr r8, =__stack_chk_guard
>> - ldr r7, [r7, #TSK_STACK_CANARY]
>> + .if (TSK_STACK_CANARY > PAGE_MASK)
>
> Shouldn't this be ~PAGE_MASK?
>
> I think
>
> .if (TSK_STACK_CANARY & PAGE_MASK) != 0
>
> is better and clearer as well
Right, sorry about that. Russell also pointed out that PAGE_MASK
is not the best constant for this, as the MMU page size is independent
of the definition of the immediate arguments. I'll fix both and resend.
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-30 17:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-30 15:34 [PATCH] ARM: fix randomized task_struct Arnd Bergmann
2017-06-30 15:49 ` Ard Biesheuvel
2017-06-30 15:55 ` Russell King - ARM Linux
2017-06-30 17:34 ` Ard Biesheuvel
2017-06-30 15:55 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®