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

Groups > linux.kernel > #1678236 > unrolled thread

Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Arnd Bergmann <arnd@arndb.de> - 2017-06-30 00:10 +0200
    Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Kees Cook <keescook@chromium.org> - 2017-06-30 01:00 +0200
      Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Kees Cook <keescook@chromium.org> - 2017-06-30 02:10 +0200
      Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Arnd Bergmann <arnd@arndb.de> - 2017-06-30 09:40 +0200
        Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-06-30 10:00 +0200
          Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Arnd Bergmann <arnd@arndb.de> - 2017-06-30 10:30 +0200
            Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Kees Cook <keescook@chromium.org> - 2017-06-30 16:50 +0200
              Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin Arnd Bergmann <arnd@arndb.de> - 2017-06-30 17:30 +0200

#1678236 — Re: [PATCH v2 04/20] gcc-plugins: Add the randstruct plugin

On Fri, May 26, 2017 at 10:17 PM, Kees Cook <keescook@chromium.org> wrote:
> This feature is ported from grsecurity. The implementation is almost
> entirely identical to the original code written by the PaX Team and
> Brad Spengler.

Hi Kees,

I noticed new build errors that bisected back to this patch, which has
now showed up
in linux-next again:


/git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
/git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
value for offset (4644)
/git/arm-soc/scripts/Makefile.build:403: recipe for target
'arch/arm/kernel/entry-armv.o' failed
make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
/git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
/git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
value for offset (5584)
/git/arm-soc/scripts/Makefile.build:403: recipe for target
'arch/arm/kernel/entry-armv.o' failed
make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
/git/arm-soc/arch/arm/mm/tlb-v4.S: Assembler messages:
/git/arm-soc/arch/arm/mm/tlb-v4.S:35: Error: bad immediate value for
offset (4928)
/git/arm-soc/scripts/Makefile.build:403: recipe for target
'arch/arm/mm/tlb-v4.o' failed
make[3]: *** [arch/arm/mm/tlb-v4.o] Error 1
/git/arm-soc/arch/arm/mm/tlb-v4wbi.S: Assembler messages:
/git/arm-soc/arch/arm/mm/tlb-v4wbi.S:34: Error: bad immediate value
for offset (4928)
/git/arm-soc/scripts/Makefile.build:403: recipe for target
'arch/arm/mm/tlb-v4wbi.o' failed

So far, that's the only thing that goes wrong for me though, and this
is probably
easy to fix.

        Arnd

[toc] | [next] | [standalone]

#1678264

On Thu, Jun 29, 2017 at 3:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, May 26, 2017 at 10:17 PM, Kees Cook <keescook@chromium.org> wrote:
> I noticed new build errors that bisected back to this patch, which has
> now showed up
> in linux-next again:

(FWIW this is randstruct not initify, and has been in -next for a
couple weeks now.)

> /git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
> /git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
> value for offset (4644)
> /git/arm-soc/scripts/Makefile.build:403: recipe for target
> 'arch/arm/kernel/entry-armv.o' failed
> make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
> /git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
> /git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
> value for offset (5584)

arch/arm/kernel/entry-armv.S:   ldr     r7, [r7, #TSK_STACK_CANARY]
arch/arm/kernel/asm-offsets.c:  DEFINE(TSK_STACK_CANARY,
offsetof(struct task_struct, stack_canary));

This would imply that stack_canary got randomized to an offset within
struct task_struct beyond the "ldr" immediate range (4096). Yay for
giant structs.

I'm surprised this didn't bisect to "task_struct: Allow randomized layout".

> /git/arm-soc/scripts/Makefile.build:403: recipe for target
> 'arch/arm/kernel/entry-armv.o' failed
> make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
> /git/arm-soc/arch/arm/mm/tlb-v4.S: Assembler messages:
> /git/arm-soc/arch/arm/mm/tlb-v4.S:35: Error: bad immediate value for
> offset (4928)

Similar:

        act_mm  r3                              @ get current->active_mm
...
        .macro  act_mm, rd
        ldr     \rd, [\rd, #TSK_ACTIVE_MM]
...
kernel/asm-offsets.c:  DEFINE(TSK_ACTIVE_MM,
offsetof(struct task_struct, active_mm));

> /git/arm-soc/scripts/Makefile.build:403: recipe for target
> 'arch/arm/mm/tlb-v4.o' failed
> make[3]: *** [arch/arm/mm/tlb-v4.o] Error 1
> /git/arm-soc/arch/arm/mm/tlb-v4wbi.S: Assembler messages:
> /git/arm-soc/arch/arm/mm/tlb-v4wbi.S:34: Error: bad immediate value
> for offset (4928)
> /git/arm-soc/scripts/Makefile.build:403: recipe for target
> 'arch/arm/mm/tlb-v4wbi.o' failed

Same as above.

> So far, that's the only thing that goes wrong for me though, and this
> is probably
> easy to fix.

Thanks for letting me know! These haven't shown up in my tests since I
haven't gotten "unlucky" in randomizing the task_struct, it seems.

I see a few possible solutions:

- ignore it and try your build again with a fresh tree and a new
randomization seed ;)
- remove "depends on !COMPILE_TEST" from
GCC_PLUGIN_RANDSTRUCT_PERFORMANCE, which will leave most stuff near
their original locations
- add a new annotation __randomize_cacheline which performs the same
logic as above, but only for the marked structure
- build new logic to keep certain fields (with some special marking)
within a given range of their original position
- rewrite the ARM code to handle larger immediates

The first obviously won't fly. The second just bypasses the problem
forcing it to be exposed by other people later. The third is likely
easiest to do now, but reduces the effectiveness of randomization for
architectures that don't have sensitive immediate values. The fourth
sounds not generally useful. The fifth may be unacceptable to arm
maintainers due to performance impacts.

Can you verify that reverting "task_struct: Allow randomized layout"
fixes a bugged build for you?

-Kees

-- 
Kees Cook
Pixel Security

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

#1678316

On Thu, Jun 29, 2017 at 3:53 PM, Kees Cook <keescook@chromium.org> wrote:
> I see a few possible solutions:

Or this ugly hack:

diff --git a/include/linux/sched.h b/include/linux/sched.h
index e2ad3531e7fe..5d131f9f1dac 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -749,6 +749,19 @@ struct task_struct {
        /* Namespaces: */
        struct nsproxy                  *nsproxy;

+#ifdef CONFIG_ARM
+       /*
+        * Since task_struct is gigantic, some asmoffset locations
+        * (e.g. TSK_STACK_CANARY) for a randomized field may exceed
+        * an architecture's instruction immediate values. As a
+        * work-around to avoid changing the performance characteristics
+        * of the assembly, split the randomization into two groups,
+        * keeping the "early" fields within range of the immediates.
+        */
+       randomized_struct_fields_end
+       randomized_struct_fields_start
+#endif
+
        /* Signal handlers: */
        struct signal_struct            *signal;
        struct sighand_struct           *sighand;


I suspect updating the ARM assembly (CONFIG-conditionally) to accept
>4095 offsets is probably the best solution.

-Kees

-- 
Kees Cook
Pixel Security

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

#1678565

On Fri, Jun 30, 2017 at 12:53 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Jun 29, 2017 at 3:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, May 26, 2017 at 10:17 PM, Kees Cook <keescook@chromium.org> wrote:
>> I noticed new build errors that bisected back to this patch, which has
>> now showed up
>> in linux-next again:
>
> (FWIW this is randstruct not initify, and has been in -next for a
> couple weeks now.)

I first saw it last week and only now got around to looking any deeper,
as I had assumed that one of my own patches caused it.

>> /git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
>> /git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
>> value for offset (4644)
>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>> 'arch/arm/kernel/entry-armv.o' failed
>> make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
>> /git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
>> /git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
>> value for offset (5584)
>
> arch/arm/kernel/entry-armv.S:   ldr     r7, [r7, #TSK_STACK_CANARY]
> arch/arm/kernel/asm-offsets.c:  DEFINE(TSK_STACK_CANARY,
> offsetof(struct task_struct, stack_canary));
>
> This would imply that stack_canary got randomized to an offset within
> struct task_struct beyond the "ldr" immediate range (4096). Yay for
> giant structs.
>
> I'm surprised this didn't bisect to "task_struct: Allow randomized layout".

The bisection was a bit tricky, it's very possible that this should have
been the one to report.

>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>> 'arch/arm/kernel/entry-armv.o' failed
>> make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
>> /git/arm-soc/arch/arm/mm/tlb-v4.S: Assembler messages:
>> /git/arm-soc/arch/arm/mm/tlb-v4.S:35: Error: bad immediate value for
>> offset (4928)
>
> Similar:
>
>         act_mm  r3                              @ get current->active_mm
> ...
>         .macro  act_mm, rd
>         ldr     \rd, [\rd, #TSK_ACTIVE_MM]
> ...
> kernel/asm-offsets.c:  DEFINE(TSK_ACTIVE_MM,
> offsetof(struct task_struct, active_mm));
>
>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>> 'arch/arm/mm/tlb-v4.o' failed
>> make[3]: *** [arch/arm/mm/tlb-v4.o] Error 1
>> /git/arm-soc/arch/arm/mm/tlb-v4wbi.S: Assembler messages:
>> /git/arm-soc/arch/arm/mm/tlb-v4wbi.S:34: Error: bad immediate value
>> for offset (4928)
>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>> 'arch/arm/mm/tlb-v4wbi.o' failed
>
> Same as above.
>
>> So far, that's the only thing that goes wrong for me though, and this
>> is probably
>> easy to fix.
>
> Thanks for letting me know! These haven't shown up in my tests since I
> haven't gotten "unlucky" in randomizing the task_struct, it seems.

I've only hit it a couple of times a few thousand builds.

> I see a few possible solutions:
>
> - ignore it and try your build again with a fresh tree and a new
> randomization seed ;)
> - remove "depends on !COMPILE_TEST" from
> GCC_PLUGIN_RANDSTRUCT_PERFORMANCE, which will leave most stuff near
> their original locations
> - add a new annotation __randomize_cacheline which performs the same
> logic as above, but only for the marked structure
> - build new logic to keep certain fields (with some special marking)
> within a given range of their original position
> - rewrite the ARM code to handle larger immediates
>
> The first obviously won't fly. The second just bypasses the problem
> forcing it to be exposed by other people later. The third is likely
> easiest to do now, but reduces the effectiveness of randomization for
> architectures that don't have sensitive immediate values. The fourth
> sounds not generally useful. The fifth may be unacceptable to arm
> maintainers due to performance impacts.

I was thinking of the fifth solution, but don't know exactly how to
do it. If performance is a concern, I guess we could have separate
implementations for randstruct and traditional builds.

I've added a few more people to Cc that may know exactly how to
do it right.

> Can you verify that reverting "task_struct: Allow randomized layout"
> fixes a bugged build for you?

Confirmed.

      Arnd

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

#1678577

On 30 June 2017 at 07:35, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 30, 2017 at 12:53 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Jun 29, 2017 at 3:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, May 26, 2017 at 10:17 PM, Kees Cook <keescook@chromium.org> wrote:
>>> I noticed new build errors that bisected back to this patch, which has
>>> now showed up
>>> in linux-next again:
>>
>> (FWIW this is randstruct not initify, and has been in -next for a
>> couple weeks now.)
>
> I first saw it last week and only now got around to looking any deeper,
> as I had assumed that one of my own patches caused it.
>
>>> /git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
>>> /git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
>>> value for offset (4644)
>>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>>> 'arch/arm/kernel/entry-armv.o' failed
>>> make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
>>> /git/arm-soc/arch/arm/kernel/entry-armv.S: Assembler messages:
>>> /git/arm-soc/arch/arm/kernel/entry-armv.S:800: Error: bad immediate
>>> value for offset (5584)
>>
>> arch/arm/kernel/entry-armv.S:   ldr     r7, [r7, #TSK_STACK_CANARY]
>> arch/arm/kernel/asm-offsets.c:  DEFINE(TSK_STACK_CANARY,
>> offsetof(struct task_struct, stack_canary));
>>
>> This would imply that stack_canary got randomized to an offset within
>> struct task_struct beyond the "ldr" immediate range (4096). Yay for
>> giant structs.
>>
>> I'm surprised this didn't bisect to "task_struct: Allow randomized layout".
>
> The bisection was a bit tricky, it's very possible that this should have
> been the one to report.
>
>>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>>> 'arch/arm/kernel/entry-armv.o' failed
>>> make[3]: *** [arch/arm/kernel/entry-armv.o] Error 1
>>> /git/arm-soc/arch/arm/mm/tlb-v4.S: Assembler messages:
>>> /git/arm-soc/arch/arm/mm/tlb-v4.S:35: Error: bad immediate value for
>>> offset (4928)
>>
>> Similar:
>>
>>         act_mm  r3                              @ get current->active_mm
>> ...
>>         .macro  act_mm, rd
>>         ldr     \rd, [\rd, #TSK_ACTIVE_MM]
>> ...
>> kernel/asm-offsets.c:  DEFINE(TSK_ACTIVE_MM,
>> offsetof(struct task_struct, active_mm));
>>
>>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>>> 'arch/arm/mm/tlb-v4.o' failed
>>> make[3]: *** [arch/arm/mm/tlb-v4.o] Error 1
>>> /git/arm-soc/arch/arm/mm/tlb-v4wbi.S: Assembler messages:
>>> /git/arm-soc/arch/arm/mm/tlb-v4wbi.S:34: Error: bad immediate value
>>> for offset (4928)
>>> /git/arm-soc/scripts/Makefile.build:403: recipe for target
>>> 'arch/arm/mm/tlb-v4wbi.o' failed
>>
>> Same as above.
>>
>>> So far, that's the only thing that goes wrong for me though, and this
>>> is probably
>>> easy to fix.
>>
>> Thanks for letting me know! These haven't shown up in my tests since I
>> haven't gotten "unlucky" in randomizing the task_struct, it seems.
>
> I've only hit it a couple of times a few thousand builds.
>
>> I see a few possible solutions:
>>
>> - ignore it and try your build again with a fresh tree and a new
>> randomization seed ;)
>> - remove "depends on !COMPILE_TEST" from
>> GCC_PLUGIN_RANDSTRUCT_PERFORMANCE, which will leave most stuff near
>> their original locations
>> - add a new annotation __randomize_cacheline which performs the same
>> logic as above, but only for the marked structure
>> - build new logic to keep certain fields (with some special marking)
>> within a given range of their original position
>> - rewrite the ARM code to handle larger immediates
>>
>> The first obviously won't fly. The second just bypasses the problem
>> forcing it to be exposed by other people later. The third is likely
>> easiest to do now, but reduces the effectiveness of randomization for
>> architectures that don't have sensitive immediate values. The fourth
>> sounds not generally useful. The fifth may be unacceptable to arm
>> maintainers due to performance impacts.
>
> I was thinking of the fifth solution, but don't know exactly how to
> do it. If performance is a concern, I guess we could have separate
> implementations for randstruct and traditional builds.
>

Does this not apply to *all* entries in asm-offsets? If so, I don't
see how it is tractable to fix this in the code, unless we add some
instrumentation to asm-offsets to whitelist some huge structs and
error out on new ones. Or perhaps there's really only a handful?

In any case, these particular examples are fairly straightforward,
since there is no need to preserve the register's value.

ldr     r7, [r7, #TSK_STACK_CANARY]

could be replaced with

.if TSK_STACK_CANARAY >= PAGE_SIZE
add r7, r7, #TSK_STACK_CANARY & PAGE_MASK
.endif
ldr r7, [r7, #TSK_STACK_CANARY & ~PAGE_MASK]

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

#1678594

On Fri, Jun 30, 2017 at 9:55 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 30 June 2017 at 07:35, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jun 30, 2017 at 12:53 AM, Kees Cook <keescook@chromium.org> wrote:
>>> The first obviously won't fly. The second just bypasses the problem
>>> forcing it to be exposed by other people later. The third is likely
>>> easiest to do now, but reduces the effectiveness of randomization for
>>> architectures that don't have sensitive immediate values. The fourth
>>> sounds not generally useful. The fifth may be unacceptable to arm
>>> maintainers due to performance impacts.
>>
>> I was thinking of the fifth solution, but don't know exactly how to
>> do it. If performance is a concern, I guess we could have separate
>> implementations for randstruct and traditional builds.
>>
>
> Does this not apply to *all* entries in asm-offsets? If so, I don't
> see how it is tractable to fix this in the code, unless we add some
> instrumentation to asm-offsets to whitelist some huge structs and
> error out on new ones. Or perhaps there's really only a handful?

I think the other structs are all small enough:

* thread_info is at most 720 bytes (including crunch+vfp3, which
  you wouldn't find in one combined kernel) and not randomized
  at the moment
* pt_regs is 72 bytes and I don't see how that would be randomized
* machine_desc would be a candidate for randomizing, but is only
  108 bytes
* proc_info_list is 52 bytes and not currently randomized
* vm_area_struct is randomized but only 96 bytes.
* task_struct is clearly large enough, but we only use TSK_ACTIVE_MM
  and TSK_STACK_CANARY, both can be fixed with your trick.

> In any case, these particular examples are fairly straightforward,
> since there is no need to preserve the register's value.
>
> ldr     r7, [r7, #TSK_STACK_CANARY]
>
> could be replaced with
>
> .if TSK_STACK_CANARAY >= PAGE_SIZE
> add r7, r7, #TSK_STACK_CANARY & PAGE_MASK
> .endif
> ldr r7, [r7, #TSK_STACK_CANARY & ~PAGE_MASK]

Nice!

      Arnd

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

#1678890

On Fri, Jun 30, 2017 at 1:27 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 30, 2017 at 9:55 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 30 June 2017 at 07:35, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Jun 30, 2017 at 12:53 AM, Kees Cook <keescook@chromium.org> wrote:
>>>> The first obviously won't fly. The second just bypasses the problem
>>>> forcing it to be exposed by other people later. The third is likely
>>>> easiest to do now, but reduces the effectiveness of randomization for
>>>> architectures that don't have sensitive immediate values. The fourth
>>>> sounds not generally useful. The fifth may be unacceptable to arm
>>>> maintainers due to performance impacts.
>>>
>>> I was thinking of the fifth solution, but don't know exactly how to
>>> do it. If performance is a concern, I guess we could have separate
>>> implementations for randstruct and traditional builds.
>>>
>>
>> Does this not apply to *all* entries in asm-offsets? If so, I don't
>> see how it is tractable to fix this in the code, unless we add some
>> instrumentation to asm-offsets to whitelist some huge structs and
>> error out on new ones. Or perhaps there's really only a handful?
>
> I think the other structs are all small enough:
>
> * thread_info is at most 720 bytes (including crunch+vfp3, which
>   you wouldn't find in one combined kernel) and not randomized
>   at the moment
> * pt_regs is 72 bytes and I don't see how that would be randomized
> * machine_desc would be a candidate for randomizing, but is only
>   108 bytes
> * proc_info_list is 52 bytes and not currently randomized
> * vm_area_struct is randomized but only 96 bytes.
> * task_struct is clearly large enough, but we only use TSK_ACTIVE_MM
>   and TSK_STACK_CANARY, both can be fixed with your trick.

Yup, that matches what I found. task_struct is the only truly giant struct.

>> In any case, these particular examples are fairly straightforward,
>> since there is no need to preserve the register's value.
>>
>> ldr     r7, [r7, #TSK_STACK_CANARY]
>>
>> could be replaced with
>>
>> .if TSK_STACK_CANARAY >= PAGE_SIZE
>> add r7, r7, #TSK_STACK_CANARY & PAGE_MASK
>> .endif
>> ldr r7, [r7, #TSK_STACK_CANARY & ~PAGE_MASK]
>
> Nice!

Oh, very cool. This'll make it only an asm change in the case where
it's required for randstruct. Perfect. I'll send a patch and carry it
in the randstruct tree.

(In looking at this, it seems tsk_mm is unused in mm/proc-macros.S, so
I'll remove that code unless someone sees something I don't.)

-Kees

-- 
Kees Cook
Pixel Security

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

#1678910

On Fri, Jun 30, 2017 at 4:41 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jun 30, 2017 at 1:27 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jun 30, 2017 at 9:55 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 30 June 2017 at 07:35, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Fri, Jun 30, 2017 at 12:53 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>> The first obviously won't fly. The second just bypasses the problem
>>>>> forcing it to be exposed by other people later. The third is likely
>>>>> easiest to do now, but reduces the effectiveness of randomization for
>>>>> architectures that don't have sensitive immediate values. The fourth
>>>>> sounds not generally useful. The fifth may be unacceptable to arm
>>>>> maintainers due to performance impacts.
>>>>
>>>> I was thinking of the fifth solution, but don't know exactly how to
>>>> do it. If performance is a concern, I guess we could have separate
>>>> implementations for randstruct and traditional builds.
>>>>
>>>
>>> Does this not apply to *all* entries in asm-offsets? If so, I don't
>>> see how it is tractable to fix this in the code, unless we add some
>>> instrumentation to asm-offsets to whitelist some huge structs and
>>> error out on new ones. Or perhaps there's really only a handful?
>>
>> I think the other structs are all small enough:
>>
>> * thread_info is at most 720 bytes (including crunch+vfp3, which
>>   you wouldn't find in one combined kernel) and not randomized
>>   at the moment
>> * pt_regs is 72 bytes and I don't see how that would be randomized
>> * machine_desc would be a candidate for randomizing, but is only
>>   108 bytes
>> * proc_info_list is 52 bytes and not currently randomized
>> * vm_area_struct is randomized but only 96 bytes.
>> * task_struct is clearly large enough, but we only use TSK_ACTIVE_MM
>>   and TSK_STACK_CANARY, both can be fixed with your trick.
>
> Yup, that matches what I found. task_struct is the only truly giant struct.
>
>>> In any case, these particular examples are fairly straightforward,
>>> since there is no need to preserve the register's value.
>>>
>>> ldr     r7, [r7, #TSK_STACK_CANARY]
>>>
>>> could be replaced with
>>>
>>> .if TSK_STACK_CANARAY >= PAGE_SIZE
>>> add r7, r7, #TSK_STACK_CANARY & PAGE_MASK
>>> .endif
>>> ldr r7, [r7, #TSK_STACK_CANARY & ~PAGE_MASK]
>>
>> Nice!
>
> Oh, very cool. This'll make it only an asm change in the case where
> it's required for randstruct. Perfect. I'll send a patch and carry it
> in the randstruct tree.
>
> (In looking at this, it seems tsk_mm is unused in mm/proc-macros.S, so
> I'll remove that code unless someone sees something I don't.)

Ah, I missed that. This is what I have committed locally
(sorry, doesn't apply because gmail). Note that I had to
compare against PAGE_MASK here rather than PAGE_SIZE,
which contains a 'ul' postfix that the assembler doesn't like.

I'll send another copy to the list separately later today,
once my randconfig builds complete (there have been some
other regressions in the last few days, this one seems fine now).

       Arnd

commit 3b6308404cd40edcff9f5e2aacd3cff9b67d660c
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Fri Jun 30 13:50:23 2017 +0200

    ARM: fix randomized task_struct

    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.

    Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    Fixes: c33d8b12fbbd ("task_struct: Allow randomized layout")
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

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..c7bd8fcf16a7 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -37,7 +37,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

 /*

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web