[PATCH v3] aarch64: enforce >=64K guard size

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] aarch64: enforce >=64K guard size

Szabolcs Nagy-2
v3:
- more comment in allocate_stack.
- define ARCH_MIN_GUARD_SIZE explicitly for all targets.
- rebase on top of master.
v2:
- only change guard size on aarch64
- don't report the inflated guard size

There are several compiler implementations that allow large stack
allocations to jump over the guard page at the end of the stack and
corrupt memory beyond that. See CVE-2017-1000364.

Compilers can emit code to probe the stack such that the guard page
cannot be skipped, but on aarch64 the probe interval is 64K instead
of the minimum supported page size (4K).

This patch enforces at least 64K guard on aarch64 unless the guard
is disabled by its size to 0.  For backward compatibility reasons
the increased guard is not reported, so it is only observable by
exhausting the address space or parsing /proc/self/maps on linux.

On other targets the patch has no effect.

The patch does not affect threads with user allocated stacks.

2018-01-09  Szabolcs Nagy  <[hidden email]>

        * nptl/allocatestack.c (allocate_stack): Use ARCH_MIN_GUARD_SIZE.
        * sysdeps/aarch64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/alpha/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/arm/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/hppa/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/i386/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/ia64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/m68k/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/microblaze/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/mips/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/nios2/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/powerpc/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/s390/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/sh/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/sparc/sparc32/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/sparc/sparc64/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/tile/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
        * sysdeps/x86_64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.

guardmin.diff (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Szabolcs Nagy-2
On 09/01/18 10:24, Szabolcs Nagy wrote:

> v3:
> - more comment in allocate_stack.
> - define ARCH_MIN_GUARD_SIZE explicitly for all targets.
> - rebase on top of master.
> v2:
> - only change guard size on aarch64
> - don't report the inflated guard size
>
> There are several compiler implementations that allow large stack
> allocations to jump over the guard page at the end of the stack and
> corrupt memory beyond that. See CVE-2017-1000364.
>
> Compilers can emit code to probe the stack such that the guard page
> cannot be skipped, but on aarch64 the probe interval is 64K instead
> of the minimum supported page size (4K).
>
> This patch enforces at least 64K guard on aarch64 unless the guard
> is disabled by its size to 0.  For backward compatibility reasons
> the increased guard is not reported, so it is only observable by
> exhausting the address space or parsing /proc/self/maps on linux.
>
> On other targets the patch has no effect.
>
> The patch does not affect threads with user allocated stacks.
>
> 2018-01-09  Szabolcs Nagy  <[hidden email]>
>
>         * nptl/allocatestack.c (allocate_stack): Use ARCH_MIN_GUARD_SIZE.
>         * sysdeps/aarch64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/alpha/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/arm/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/hppa/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/i386/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/ia64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/m68k/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/microblaze/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/mips/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/nios2/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/powerpc/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/s390/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/sh/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/sparc/sparc32/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/sparc/sparc64/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/tile/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>         * sysdeps/x86_64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>


meanwhile this passed a build-many-glibcs.py test
is this ok for 2.27 ?

$ grep '"FAIL"' build-state.json
      "compilers-m68k-linux-gnu-coldfire gcc-first build": "FAIL",
      "glibcs-hppa-linux-gnu check": "FAIL",
      "glibcs-m68k-linux-gnu-coldfire check-compilers": "FAIL",
      "glibcs-microblaze-linux-gnu check": "FAIL",
      "glibcs-microblazeel-linux-gnu check": "FAIL",

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Florian Weimer-5
On 01/10/2018 12:48 PM, Szabolcs Nagy wrote:
> meanwhile this passed a build-many-glibcs.py test
> is this ok for 2.27 ?

I don't think the GCC patch has been committed yet.

I find it baffling what's going on there.  Aren't the ARM maintainers
interested in this feature?

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Szabolcs Nagy-2
On 11/01/18 09:32, Florian Weimer wrote:
> On 01/10/2018 12:48 PM, Szabolcs Nagy wrote:
>> meanwhile this passed a build-many-glibcs.py test
>> is this ok for 2.27 ?
>
> I don't think the GCC patch has been committed yet.
>
> I find it baffling what's going on there.  Aren't the ARM maintainers interested in this feature?
>

i don't know all the details on the gcc side,

the original patch was identified to be both suboptimal
and incorrect in some cases, however nobody had the time
to fix it.

however there is a commitment to 64k probe interval by
default so even if gcc-8 only does 4k probing or no
probing by default, i would still want the larger guard
size in glibc (it is also safer with little cost).

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Szabolcs Nagy-2
In reply to this post by Szabolcs Nagy-2
On 10/01/18 11:48, Szabolcs Nagy wrote:

> On 09/01/18 10:24, Szabolcs Nagy wrote:
>> v3:
>> - more comment in allocate_stack.
>> - define ARCH_MIN_GUARD_SIZE explicitly for all targets.
>> - rebase on top of master.
>> v2:
>> - only change guard size on aarch64
>> - don't report the inflated guard size
>>
>> There are several compiler implementations that allow large stack
>> allocations to jump over the guard page at the end of the stack and
>> corrupt memory beyond that. See CVE-2017-1000364.
>>
>> Compilers can emit code to probe the stack such that the guard page
>> cannot be skipped, but on aarch64 the probe interval is 64K instead
>> of the minimum supported page size (4K).
>>
>> This patch enforces at least 64K guard on aarch64 unless the guard
>> is disabled by its size to 0.  For backward compatibility reasons
>> the increased guard is not reported, so it is only observable by
>> exhausting the address space or parsing /proc/self/maps on linux.
>>
>> On other targets the patch has no effect.
>>
>> The patch does not affect threads with user allocated stacks.
>>
>> 2018-01-09  Szabolcs Nagy  <[hidden email]>
>>
>>         * nptl/allocatestack.c (allocate_stack): Use ARCH_MIN_GUARD_SIZE.
>>         * sysdeps/aarch64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/alpha/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/arm/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/hppa/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/i386/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/ia64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/m68k/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/microblaze/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/mips/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/nios2/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/powerpc/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/s390/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/sh/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/sparc/sparc32/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/sparc/sparc64/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/tile/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>         * sysdeps/x86_64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.
>>
>
>
> meanwhile this passed a build-many-glibcs.py test
> is this ok for 2.27 ?
>

ping.

i'd like to have this patch independently
of what gcc is doing.

i only expect this to affect aarch64.
(and new targets that will have to add ARCH_MIN_GUARD_SIZE)

> $ grep '"FAIL"' build-state.json
>       "compilers-m68k-linux-gnu-coldfire gcc-first build": "FAIL",
>       "glibcs-hppa-linux-gnu check": "FAIL",
>       "glibcs-m68k-linux-gnu-coldfire check-compilers": "FAIL",
>       "glibcs-microblaze-linux-gnu check": "FAIL",
>       "glibcs-microblazeel-linux-gnu check": "FAIL",
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Florian Weimer-5
On 01/15/2018 05:59 PM, Szabolcs Nagy wrote:
> i'd like to have this patch independently of what gcc is doing.

This does not make sense to me.  The sole purpose of this patch is to
support what GCC is doing, and things still aren't settled on the GCC side.

The patch looks backportable to a release branch to me, so there's no
rush.  (I already backported the guard size accounting change.)

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Szabolcs Nagy-2
On 15/01/18 17:05, Florian Weimer wrote:
> On 01/15/2018 05:59 PM, Szabolcs Nagy wrote:
>> i'd like to have this patch independently of what gcc is doing.
>
> This does not make sense to me.  The sole purpose of this patch is to support what GCC is doing, and things
> still aren't settled on the GCC side.
>

i think still think that the patch is useful even without any probing.

i don't expect this to be settled in gcc-8.

> The patch looks backportable to a release branch to me, so there's no rush.  (I already backported the guard
> size accounting change.)
>

ok, then i can wait.

> Thanks,
> Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

James Greenhalgh-2
In reply to this post by Florian Weimer-5
On Thu, Jan 11, 2018 at 09:32:23AM +0000, Florian Weimer wrote:
> On 01/10/2018 12:48 PM, Szabolcs Nagy wrote:
> > meanwhile this passed a build-many-glibcs.py test
> > is this ok for 2.27 ?
>
> I don't think the GCC patch has been committed yet.
>
> I find it baffling what's going on there.  Aren't the ARM maintainers
> interested in this feature?

Last I saw in December, we were making some good progress towards a conclusion
over here, which will guide the GCC support. Here is what I proposed then:

https://sourceware.org/ml/libc-alpha/2017-12/msg00630.html

  Our proposal then, having spoken things through with the Arm engineers
  here, and taken in to consideration the opinions on this thread, is that
  we move to two "blessed" configurations of the GCC support for AArch64.

  One would assume 64k guard pages. This would work out-of-the-box on systems
  with a 64k page size, and would work with modifications to glibc (and other
  libraries as appropriate) on systems with a 4k (or other) page size. The
  performance impact will be low. If we take this approach, this will be the
  default configuration for GCC.

  The other would assume 4k guard pages. This would work everywhere, and
  as-good-as guarantee complete coverage. However, it would be inefficient
  on systems with a larger page size, or with a glibc upgraded to use
  64k guard-pages.

You replied:

https://sourceware.org/ml/libc-alpha/2017-12/msg00635.html

  So if this is some sort of consensus proposal, as opposed to actual
  technical requirements which favor Option 2 in some deployments, I think
  that's not a good idea, and we should go with Option 1 instead.

To which Szabolcs replied:

https://sourceware.org/ml/libc-alpha/2017-12/msg00662.html

  well glibc can pretend that only Option 1 is available,
  my latest patch assumes 64k probe interval:
  https://sourceware.org/ml/libc-alpha/2017-12/msg00451.html

  however Option 1 requires generic code to be changed
  for aarch64 only (in the libc and elsewhere) and we
  cannot easily do that on all (non-glibc) systems.

  it seems to me if there are systems where Option 1
  may not provide guaranteed trap on stack overflow
  then gcc should have Option 2 for those systems.

Which to me would imply that, as Szabolcs has said, this patch is the correct
thing to do regardless of what we do in GCC.

The GCC requirements from non-glibc targets remain unclear to me. Rich
said:

https://sourceware.org/ml/libc-alpha/2017-12/msg00682.html

  For what it's worth, I would prefer having the assumed minimum guard
  size be 4k for musl targets. Even if we do increase the default guard
  to 64k for 64-bit archs (seems likely), applications that manually set
  it lower for whatever reason should still be handled safely.

So, for me, that makes the GCC path clear - the compromise/consensus proposal
to have two modes is the only way to unify the glibc and musl requirements.
That goes against what Jeff would ideally like:

https://sourceware.org/ml/libc-alpha/2017-12/msg00700.html

  Ideally we set the guard size now and never ever decrease it.   I'd
  really like to remove the option to make it a compile-time configurable
  --param for GCC.  ie, it's baked into the port and never changes.

But seems to be the only sensible way to bridge the gap between what different
C librarians and users expect.

The GCC side conversation finished here just before I took a break for
Christmas https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01211.html I'll write
a reply to Jeff soon and try to pin down what needs to happen next with the
AArch64 GCC side. I do care about this feature, and would like it in GCC 8,
there are a fair amount of competing requirements on the patch for AArch64.

All that said, Szabolcs' patch over here looks correct, and makes a clear
statement about what we can expect as we progress the glibc support. We
still have a little time before the GCC 8 release, and the GCC patch will
require slight rework around the SVE patch anyway. If we have agreement over
here, we at least know what we're working towards.

Is there some reason I'm missing that Szabolc's patch would be incorrect,
regardless of what we do in GCC? If not, I'd suggest taking it.

Thanks,
James

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] aarch64: enforce >=64K guard size

Florian Weimer-5
On 01/16/2018 05:04 PM, James Greenhalgh wrote:

> The GCC requirements from non-glibc targets remain unclear to me. Rich
> said:
>
> https://sourceware.org/ml/libc-alpha/2017-12/msg00682.html
>
>    For what it's worth, I would prefer having the assumed minimum guard
>    size be 4k for musl targets. Even if we do increase the default guard
>    to 64k for 64-bit archs (seems likely), applications that manually set
>    it lower for whatever reason should still be handled safely.
>
> So, for me, that makes the GCC path clear - the compromise/consensus proposal
> to have two modes is the only way to unify the glibc and musl requirements.

Currently, musl and glibc requirements are completely aligned: Both
provide a 4 KiB guard region and thus require a 4 KiB probe interval and
a caller/callee probing regime compatible with that.

There is *nothing* in glibc which currently favors a 64 KiB guard
region.  4 KiB is strongly preferred because that's how the ABI started out.

Thanks,
Florian