[PATCH] x86: Document -mno-cet-switch

[PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

When -fcf-protection=branch is used, the compiler will generate jump
tables where the indirect jump is prefixed with the NOTRACK prefix, so
it can jump to non-ENDBR targets. Yet, for NOTRACK prefixes to work, the
NOTRACK specific enable bit must be set, what renders the binary broken
on any environment where this is not the case. In fact, having NOTRACK
disabled was a design choice for the Linux kernel CET support.

Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
tables for switch statements.

gcc/

	PR target/104816
	* config/i386/i386.opt: Turn on -mcet-switch by default.
	* doc/invoke.texi: Document -mno-cet-switch.

gcc/testsuite/

	* gcc.target/i386/cet-switch-1.c: Add -mno-cet-switch.
	* gcc.target/i386/cet-switch-2.c: Remove -mcet-switch.
	* gcc.target/i386/cet-switch-3.c: Likewise.
---
 gcc/config/i386/i386.opt                     | 2 +-
 gcc/doc/invoke.texi                          | 9 ++++++++-
 gcc/testsuite/gcc.target/i386/cet-switch-1.c | 2 +-
 gcc/testsuite/gcc.target/i386/cet-switch-2.c | 2 +-
 gcc/testsuite/gcc.target/i386/cet-switch-3.c | 2 +-
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
index a6b0e28f238..96b4a433e44 100644
--- a/gcc/config/i386/i386.opt
+++ b/gcc/config/i386/i386.opt
@@ -1047,7 +1047,7 @@ Enable shadow stack built-in functions from Control-flow Enforcement
 Technology (CET).
 
 mcet-switch
-Target Undocumented Var(flag_cet_switch) Init(0)
+Target Var(flag_cet_switch) Init(1)
 Turn on CET instrumentation for switch statements that use a jump table and
 an indirect jump.
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 7a35d9613a4..8bb96c5938a 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -1420,7 +1420,8 @@ See RS/6000 and PowerPC Options.
 -msse4a  -m3dnow  -m3dnowa  -mpopcnt  -mabm  -mbmi  -mtbm  -mfma4  -mxop @gol
 -madx  -mlzcnt  -mbmi2  -mfxsr  -mxsave  -mxsaveopt  -mrtm  -mhle  -mlwp @gol
 -mmwaitx  -mclzero  -mpku  -mthreads  -mgfni  -mvaes  -mwaitpkg @gol
--mshstk -mmanual-endbr -mforce-indirect-call  -mavx512vbmi2 -mavx512bf16 -menqcmd @gol
+-mshstk -mmanual-endbr -mno-cet-switch -mforce-indirect-call @gol
+-mavx512vbmi2 -mavx512bf16 -menqcmd @gol
 -mvpclmulqdq  -mavx512bitalg  -mmovdiri  -mmovdir64b  -mavx512vpopcntdq @gol
 -mavx5124fmaps  -mavx512vnni  -mavx5124vnniw  -mprfchw  -mrdpid @gol
 -mrdseed  -msgx -mavx512vp2intersect -mserialize -mtsxldtrk@gol
@@ -32641,6 +32642,12 @@ function attribute. This is useful when used with the option
 @option{-fcf-protection=branch} to control ENDBR insertion at the
 function entry.
 
+@item -mno-cet-switch
+@opindex mno-cet-switch
+@opindex mcet-switch
+Turn off CET instrumentation for switch statements that use a jump table
+and an indirect jump.  The default is @option{mcet-switch}.
+
 @item -mcall-ms2sysv-xlogues
 @opindex mcall-ms2sysv-xlogues
 @opindex mno-call-ms2sysv-xlogues
diff --git a/gcc/testsuite/gcc.target/i386/cet-switch-1.c b/gcc/testsuite/gcc.target/i386/cet-switch-1.c
index afe5adc2f3d..4931c3ad1d2 100644
--- a/gcc/testsuite/gcc.target/i386/cet-switch-1.c
+++ b/gcc/testsuite/gcc.target/i386/cet-switch-1.c
@@ -1,6 +1,6 @@
 /* Verify that CET works.  */
 /* { dg-do compile } */
-/* { dg-options "-O -fcf-protection" } */
+/* { dg-options "-O -fcf-protection -mno-cet-switch" } */
 /* { dg-final { scan-assembler-times "endbr32" 1 { target ia32 } } } */
 /* { dg-final { scan-assembler-times "endbr64" 1 { target { ! ia32 } } } } */
 /* { dg-final { scan-assembler-times "notrack jmp\[ \t]+\[*]" 1 } } */
diff --git a/gcc/testsuite/gcc.target/i386/cet-switch-2.c b/gcc/testsuite/gcc.target/i386/cet-switch-2.c
index 69ddc6fd5b7..11578d1a30c 100644
--- a/gcc/testsuite/gcc.target/i386/cet-switch-2.c
+++ b/gcc/testsuite/gcc.target/i386/cet-switch-2.c
@@ -1,6 +1,6 @@
 /* Verify that CET works.  */
 /* { dg-do compile } */
-/* { dg-options "-O -fcf-protection -mcet-switch" } */
+/* { dg-options "-O -fcf-protection" } */
 /* { dg-final { scan-assembler-times "endbr32" 12 { target ia32 } } } */
 /* { dg-final { scan-assembler-times "endbr64" 12 { target { ! ia32 } } } } */
 /* { dg-final { scan-assembler-times "\[ \t]+jmp\[ \t]+\[*]" 1 } } */
diff --git a/gcc/testsuite/gcc.target/i386/cet-switch-3.c b/gcc/testsuite/gcc.target/i386/cet-switch-3.c
index 0d9ed4488dd..a4e2e4dfc16 100644
--- a/gcc/testsuite/gcc.target/i386/cet-switch-3.c
+++ b/gcc/testsuite/gcc.target/i386/cet-switch-3.c
@@ -1,6 +1,6 @@
 /* Verify that CET works.  */
 /* { dg-do compile } */
-/* { dg-options "-O -fcf-protection -mcet-switch" } */
+/* { dg-options "-O -fcf-protection" } */
 /* { dg-final { scan-assembler-times "endbr32" 12 { target ia32 } } } */
 /* { dg-final { scan-assembler-times "endbr64" 12 { target { ! ia32 } } } } */
 /* { dg-final { scan-assembler-times "\[ \t]+jmp\[ \t]+\[*]" 1 } } */
-- 
2.35.1

Re: [PATCH] x86: Document -mno-cet-switch

[Uros Bizjak]

On Tue, May 10, 2022 at 6:20 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> When -fcf-protection=branch is used, the compiler will generate jump
> tables where the indirect jump is prefixed with the NOTRACK prefix, so
> it can jump to non-ENDBR targets. Yet, for NOTRACK prefixes to work, the
> NOTRACK specific enable bit must be set, what renders the binary broken
> on any environment where this is not the case. In fact, having NOTRACK
> disabled was a design choice for the Linux kernel CET support.
>
> Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
> jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
> tables for switch statements.
>
> gcc/
>
>         PR target/104816
>         * config/i386/i386.opt: Turn on -mcet-switch by default.
>         * doc/invoke.texi: Document -mno-cet-switch.
>
> gcc/testsuite/
>
>         * gcc.target/i386/cet-switch-1.c: Add -mno-cet-switch.
>         * gcc.target/i386/cet-switch-2.c: Remove -mcet-switch.
>         * gcc.target/i386/cet-switch-3.c: Likewise.

LGTM.

Thanks,
Uros.

> ---
>  gcc/config/i386/i386.opt                     | 2 +-
>  gcc/doc/invoke.texi                          | 9 ++++++++-
>  gcc/testsuite/gcc.target/i386/cet-switch-1.c | 2 +-
>  gcc/testsuite/gcc.target/i386/cet-switch-2.c | 2 +-
>  gcc/testsuite/gcc.target/i386/cet-switch-3.c | 2 +-
>  5 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
> index a6b0e28f238..96b4a433e44 100644
> --- a/gcc/config/i386/i386.opt
> +++ b/gcc/config/i386/i386.opt
> @@ -1047,7 +1047,7 @@ Enable shadow stack built-in functions from Control-flow Enforcement
>  Technology (CET).
>
>  mcet-switch
> -Target Undocumented Var(flag_cet_switch) Init(0)
> +Target Var(flag_cet_switch) Init(1)
>  Turn on CET instrumentation for switch statements that use a jump table and
>  an indirect jump.
>
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 7a35d9613a4..8bb96c5938a 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1420,7 +1420,8 @@ See RS/6000 and PowerPC Options.
>  -msse4a  -m3dnow  -m3dnowa  -mpopcnt  -mabm  -mbmi  -mtbm  -mfma4  -mxop @gol
>  -madx  -mlzcnt  -mbmi2  -mfxsr  -mxsave  -mxsaveopt  -mrtm  -mhle  -mlwp @gol
>  -mmwaitx  -mclzero  -mpku  -mthreads  -mgfni  -mvaes  -mwaitpkg @gol
> --mshstk -mmanual-endbr -mforce-indirect-call  -mavx512vbmi2 -mavx512bf16 -menqcmd @gol
> +-mshstk -mmanual-endbr -mno-cet-switch -mforce-indirect-call @gol
> +-mavx512vbmi2 -mavx512bf16 -menqcmd @gol
>  -mvpclmulqdq  -mavx512bitalg  -mmovdiri  -mmovdir64b  -mavx512vpopcntdq @gol
>  -mavx5124fmaps  -mavx512vnni  -mavx5124vnniw  -mprfchw  -mrdpid @gol
>  -mrdseed  -msgx -mavx512vp2intersect -mserialize -mtsxldtrk@gol
> @@ -32641,6 +32642,12 @@ function attribute. This is useful when used with the option
>  @option{-fcf-protection=branch} to control ENDBR insertion at the
>  function entry.
>
> +@item -mno-cet-switch
> +@opindex mno-cet-switch
> +@opindex mcet-switch
> +Turn off CET instrumentation for switch statements that use a jump table
> +and an indirect jump.  The default is @option{mcet-switch}.
> +
>  @item -mcall-ms2sysv-xlogues
>  @opindex mcall-ms2sysv-xlogues
>  @opindex mno-call-ms2sysv-xlogues
> diff --git a/gcc/testsuite/gcc.target/i386/cet-switch-1.c b/gcc/testsuite/gcc.target/i386/cet-switch-1.c
> index afe5adc2f3d..4931c3ad1d2 100644
> --- a/gcc/testsuite/gcc.target/i386/cet-switch-1.c
> +++ b/gcc/testsuite/gcc.target/i386/cet-switch-1.c
> @@ -1,6 +1,6 @@
>  /* Verify that CET works.  */
>  /* { dg-do compile } */
> -/* { dg-options "-O -fcf-protection" } */
> +/* { dg-options "-O -fcf-protection -mno-cet-switch" } */
>  /* { dg-final { scan-assembler-times "endbr32" 1 { target ia32 } } } */
>  /* { dg-final { scan-assembler-times "endbr64" 1 { target { ! ia32 } } } } */
>  /* { dg-final { scan-assembler-times "notrack jmp\[ \t]+\[*]" 1 } } */
> diff --git a/gcc/testsuite/gcc.target/i386/cet-switch-2.c b/gcc/testsuite/gcc.target/i386/cet-switch-2.c
> index 69ddc6fd5b7..11578d1a30c 100644
> --- a/gcc/testsuite/gcc.target/i386/cet-switch-2.c
> +++ b/gcc/testsuite/gcc.target/i386/cet-switch-2.c
> @@ -1,6 +1,6 @@
>  /* Verify that CET works.  */
>  /* { dg-do compile } */
> -/* { dg-options "-O -fcf-protection -mcet-switch" } */
> +/* { dg-options "-O -fcf-protection" } */
>  /* { dg-final { scan-assembler-times "endbr32" 12 { target ia32 } } } */
>  /* { dg-final { scan-assembler-times "endbr64" 12 { target { ! ia32 } } } } */
>  /* { dg-final { scan-assembler-times "\[ \t]+jmp\[ \t]+\[*]" 1 } } */
> diff --git a/gcc/testsuite/gcc.target/i386/cet-switch-3.c b/gcc/testsuite/gcc.target/i386/cet-switch-3.c
> index 0d9ed4488dd..a4e2e4dfc16 100644
> --- a/gcc/testsuite/gcc.target/i386/cet-switch-3.c
> +++ b/gcc/testsuite/gcc.target/i386/cet-switch-3.c
> @@ -1,6 +1,6 @@
>  /* Verify that CET works.  */
>  /* { dg-do compile } */
> -/* { dg-options "-O -fcf-protection -mcet-switch" } */
> +/* { dg-options "-O -fcf-protection" } */
>  /* { dg-final { scan-assembler-times "endbr32" 12 { target ia32 } } } */
>  /* { dg-final { scan-assembler-times "endbr64" 12 { target { ! ia32 } } } } */
>  /* { dg-final { scan-assembler-times "\[ \t]+jmp\[ \t]+\[*]" 1 } } */
> --
> 2.35.1
>

Re: [PATCH] x86: Document -mno-cet-switch

[Florian Weimer]

* H. J. Lu via Gcc-patches:

> When -fcf-protection=branch is used, the compiler will generate jump
> tables where the indirect jump is prefixed with the NOTRACK prefix, so
> it can jump to non-ENDBR targets. Yet, for NOTRACK prefixes to work, the
> NOTRACK specific enable bit must be set, what renders the binary broken
> on any environment where this is not the case. In fact, having NOTRACK
> disabled was a design choice for the Linux kernel CET support.

Why isn't that a kernel bug?  It doesn't match what is in the current
glibc sources.

> Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
> jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
> tables for switch statements.

Of course, that is a slight regression in security hardening.

Quite frankly, I'm puzzled why the kernel decided to require these
additional ENDBR instructions.

Thanks,
Florian

Re: [PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

On Wed, May 11, 2022 at 1:12 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Gcc-patches:
>
> > When -fcf-protection=branch is used, the compiler will generate jump
> > tables where the indirect jump is prefixed with the NOTRACK prefix, so
> > it can jump to non-ENDBR targets. Yet, for NOTRACK prefixes to work, the
> > NOTRACK specific enable bit must be set, what renders the binary broken
> > on any environment where this is not the case. In fact, having NOTRACK
> > disabled was a design choice for the Linux kernel CET support.
>
> Why isn't that a kernel bug?  It doesn't match what is in the current
> glibc sources.

User space uses NOTRACK in the jump table in assembly codes.

> > Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
> > jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
> > tables for switch statements.
>
> Of course, that is a slight regression in security hardening.
>
> Quite frankly, I'm puzzled why the kernel decided to require these
> additional ENDBR instructions.

Kernel is using -mcet-switch today.   Should we document -mcet-switch
and keep it off by default instead?

-- 
H.J.

Re: [PATCH] x86: Document -mno-cet-switch

[Florian Weimer]

* H. J. Lu:

> On Wed, May 11, 2022 at 1:12 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu via Gcc-patches:
>>
>> > When -fcf-protection=branch is used, the compiler will generate jump
>> > tables where the indirect jump is prefixed with the NOTRACK prefix, so
>> > it can jump to non-ENDBR targets. Yet, for NOTRACK prefixes to work, the
>> > NOTRACK specific enable bit must be set, what renders the binary broken
>> > on any environment where this is not the case. In fact, having NOTRACK
>> > disabled was a design choice for the Linux kernel CET support.
>>
>> Why isn't that a kernel bug?  It doesn't match what is in the current
>> glibc sources.
>
> User space uses NOTRACK in the jump table in assembly codes.

And is expected to continue to use it?

>> > Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
>> > jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
>> > tables for switch statements.
>>
>> Of course, that is a slight regression in security hardening.
>>
>> Quite frankly, I'm puzzled why the kernel decided to require these
>> additional ENDBR instructions.
>
> Kernel is using -mcet-switch today.   Should we document -mcet-switch
> and keep it off by default instead?

Sorry, I'm not 100% certain of the mechanics/options involved.

I think the default should reflect userspace requirements, like with the
red zone and vector register usage for integer code.

Thanks,
Florian

Re: [PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

On Wed, May 11, 2022 at 8:58 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Wed, May 11, 2022 at 1:12 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu via Gcc-patches:
> >>
> >> > When -fcf-protection=branch is used, the compiler will generate jump
> >> > tables where the indirect jump is prefixed with the NOTRACK prefix, so
> >> > it can jump to non-ENDBR targets. Yet, for NOTRACK prefixes to work, the
> >> > NOTRACK specific enable bit must be set, what renders the binary broken
> >> > on any environment where this is not the case. In fact, having NOTRACK
> >> > disabled was a design choice for the Linux kernel CET support.
> >>
> >> Why isn't that a kernel bug?  It doesn't match what is in the current
> >> glibc sources.
> >
> > User space uses NOTRACK in the jump table in assembly codes.
>
> And is expected to continue to use it?

Yes, it should be allowed in user space.

> >> > Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
> >> > jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
> >> > tables for switch statements.
> >>
> >> Of course, that is a slight regression in security hardening.
> >>
> >> Quite frankly, I'm puzzled why the kernel decided to require these
> >> additional ENDBR instructions.
> >
> > Kernel is using -mcet-switch today.   Should we document -mcet-switch
> > and keep it off by default instead?
>
> Sorry, I'm not 100% certain of the mechanics/options involved.
>
> I think the default should reflect userspace requirements, like with the
> red zone and vector register usage for integer code.

The question is if the compiler should use NOTRACK by default for
the jump table.   It is independent of whether NOTRACK is enabled or
not.

Should I check in my patch asis?

Thanks.

-- 
H.J.

Re: [PATCH] x86: Document -mno-cet-switch

[Florian Weimer]

* H. J. Lu:

>> >> > Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
>> >> > jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
>> >> > tables for switch statements.
>> >>
>> >> Of course, that is a slight regression in security hardening.
>> >>
>> >> Quite frankly, I'm puzzled why the kernel decided to require these
>> >> additional ENDBR instructions.
>> >
>> > Kernel is using -mcet-switch today.   Should we document -mcet-switch
>> > and keep it off by default instead?
>>
>> Sorry, I'm not 100% certain of the mechanics/options involved.
>>
>> I think the default should reflect userspace requirements, like with the
>> red zone and vector register usage for integer code.
>
> The question is if the compiler should use NOTRACK by default for
> the jump table.   It is independent of whether NOTRACK is enabled or
> not.

NOTRACK avoids the need for ENDBR instructions, right?  That's a
hardening improvement, so it should be used by default.

Thanks,
Florian

Re: [PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

On Wed, May 11, 2022 at 11:22 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> >> > Generate jump tables with ENDBR and skip the NOTRACK prefix for indirect
> >> >> > jump.  Document -mno-cet-switch to turn off CET instrumentation on jump
> >> >> > tables for switch statements.
> >> >>
> >> >> Of course, that is a slight regression in security hardening.
> >> >>
> >> >> Quite frankly, I'm puzzled why the kernel decided to require these
> >> >> additional ENDBR instructions.
> >> >
> >> > Kernel is using -mcet-switch today.   Should we document -mcet-switch
> >> > and keep it off by default instead?
> >>
> >> Sorry, I'm not 100% certain of the mechanics/options involved.
> >>
> >> I think the default should reflect userspace requirements, like with the
> >> red zone and vector register usage for integer code.
> >
> > The question is if the compiler should use NOTRACK by default for
> > the jump table.   It is independent of whether NOTRACK is enabled or
> > not.
>
> NOTRACK avoids the need for ENDBR instructions, right?  That's a
> hardening improvement, so it should be used by default.

NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
GCC uses it in the jump table to avoid ENDBR.

-- 
H.J.

Re: [PATCH] x86: Document -mno-cet-switch

[Florian Weimer]

* H. J. Lu:

>> NOTRACK avoids the need for ENDBR instructions, right?  That's a
>> hardening improvement, so it should be used by default.
>
> NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
> GCC uses it in the jump table to avoid ENDBR.

Typical jump table code looks like this:

Dump of assembler code for function __cache_sysconf:
   0x00000000000f7a80 <+0>:	endbr64 
   0x00000000000f7a84 <+4>:	sub    $0xb9,%edi
   0x00000000000f7a8a <+10>:	cmp    $0xc,%edi
   0x00000000000f7a8d <+13>:	ja     0xf7b70 <__cache_sysconf+240>
   0x00000000000f7a93 <+19>:	lea    0xba926(%rip),%rdx        # 0x1b23c0
   0x00000000000f7a9a <+26>:	movslq (%rdx,%rdi,4),%rax
   0x00000000000f7a9e <+30>:	add    %rdx,%rax
   0x00000000000f7aa1 <+33>:	notrack jmp *%rax

There's no ENDBR instruction between range check, the address
computation, and the NOTRACK JMP, so it's not possible to redirect that
JMP to some other place.

I don't know if GCC systematically enforces this in its optimizers,
though.

Thanks,
Florian

Re: [PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

On Wed, May 11, 2022 at 11:45 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> NOTRACK avoids the need for ENDBR instructions, right?  That's a
> >> hardening improvement, so it should be used by default.
> >
> > NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
> > GCC uses it in the jump table to avoid ENDBR.
>
> Typical jump table code looks like this:
>
> Dump of assembler code for function __cache_sysconf:
>    0x00000000000f7a80 <+0>:     endbr64
>    0x00000000000f7a84 <+4>:     sub    $0xb9,%edi
>    0x00000000000f7a8a <+10>:    cmp    $0xc,%edi
>    0x00000000000f7a8d <+13>:    ja     0xf7b70 <__cache_sysconf+240>
>    0x00000000000f7a93 <+19>:    lea    0xba926(%rip),%rdx        # 0x1b23c0
>    0x00000000000f7a9a <+26>:    movslq (%rdx,%rdi,4),%rax
>    0x00000000000f7a9e <+30>:    add    %rdx,%rax
>    0x00000000000f7aa1 <+33>:    notrack jmp *%rax
>
> There's no ENDBR instruction between range check, the address
> computation, and the NOTRACK JMP, so it's not possible to redirect that
> JMP to some other place.

That is the assumption we made.   RAX will always point to the valid
address.

> I don't know if GCC systematically enforces this in its optimizers,
> though.
>
> Thanks,
> Florian
>


-- 
H.J.

Re: [PATCH] x86: Document -mno-cet-switch

[Florian Weimer]

* H. J. Lu:

> On Wed, May 11, 2022 at 11:45 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> >> NOTRACK avoids the need for ENDBR instructions, right?  That's a
>> >> hardening improvement, so it should be used by default.
>> >
>> > NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
>> > GCC uses it in the jump table to avoid ENDBR.
>>
>> Typical jump table code looks like this:
>>
>> Dump of assembler code for function __cache_sysconf:
>>    0x00000000000f7a80 <+0>:     endbr64
>>    0x00000000000f7a84 <+4>:     sub    $0xb9,%edi
>>    0x00000000000f7a8a <+10>:    cmp    $0xc,%edi
>>    0x00000000000f7a8d <+13>:    ja     0xf7b70 <__cache_sysconf+240>
>>    0x00000000000f7a93 <+19>:    lea    0xba926(%rip),%rdx        # 0x1b23c0
>>    0x00000000000f7a9a <+26>:    movslq (%rdx,%rdi,4),%rax
>>    0x00000000000f7a9e <+30>:    add    %rdx,%rax
>>    0x00000000000f7aa1 <+33>:    notrack jmp *%rax
>>
>> There's no ENDBR instruction between range check, the address
>> computation, and the NOTRACK JMP, so it's not possible to redirect that
>> JMP to some other place.
>
> That is the assumption we made.   RAX will always point to the valid
> address.

Which means that NOTRACK should not weaken anything here.  What am I
missing?

Thanks,
Florian

Re: [PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

On Wed, May 11, 2022 at 12:02 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Wed, May 11, 2022 at 11:45 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> >> NOTRACK avoids the need for ENDBR instructions, right?  That's a
> >> >> hardening improvement, so it should be used by default.
> >> >
> >> > NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
> >> > GCC uses it in the jump table to avoid ENDBR.
> >>
> >> Typical jump table code looks like this:
> >>
> >> Dump of assembler code for function __cache_sysconf:
> >>    0x00000000000f7a80 <+0>:     endbr64
> >>    0x00000000000f7a84 <+4>:     sub    $0xb9,%edi
> >>    0x00000000000f7a8a <+10>:    cmp    $0xc,%edi
> >>    0x00000000000f7a8d <+13>:    ja     0xf7b70 <__cache_sysconf+240>
> >>    0x00000000000f7a93 <+19>:    lea    0xba926(%rip),%rdx        # 0x1b23c0
> >>    0x00000000000f7a9a <+26>:    movslq (%rdx,%rdi,4),%rax
> >>    0x00000000000f7a9e <+30>:    add    %rdx,%rax
> >>    0x00000000000f7aa1 <+33>:    notrack jmp *%rax
> >>
> >> There's no ENDBR instruction between range check, the address
> >> computation, and the NOTRACK JMP, so it's not possible to redirect that
> >> JMP to some other place.
> >
> > That is the assumption we made.   RAX will always point to the valid
> > address.
>
> Which means that NOTRACK should not weaken anything here.  What am I
> missing?
>

I will send out the v2 patch to document -mcet-switch instead.

-- 
H.J.

Re: [PATCH] x86: Document -mno-cet-switch

[Richard Biener]

On Wed, May 11, 2022 at 9:03 PM Florian Weimer via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> * H. J. Lu:
>
> > On Wed, May 11, 2022 at 11:45 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> >> NOTRACK avoids the need for ENDBR instructions, right?  That's a
> >> >> hardening improvement, so it should be used by default.
> >> >
> >> > NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
> >> > GCC uses it in the jump table to avoid ENDBR.
> >>
> >> Typical jump table code looks like this:
> >>
> >> Dump of assembler code for function __cache_sysconf:
> >>    0x00000000000f7a80 <+0>:     endbr64
> >>    0x00000000000f7a84 <+4>:     sub    $0xb9,%edi
> >>    0x00000000000f7a8a <+10>:    cmp    $0xc,%edi
> >>    0x00000000000f7a8d <+13>:    ja     0xf7b70 <__cache_sysconf+240>
> >>    0x00000000000f7a93 <+19>:    lea    0xba926(%rip),%rdx        # 0x1b23c0
> >>    0x00000000000f7a9a <+26>:    movslq (%rdx,%rdi,4),%rax
> >>    0x00000000000f7a9e <+30>:    add    %rdx,%rax
> >>    0x00000000000f7aa1 <+33>:    notrack jmp *%rax
> >>
> >> There's no ENDBR instruction between range check, the address
> >> computation, and the NOTRACK JMP, so it's not possible to redirect that
> >> JMP to some other place.
> >
> > That is the assumption we made.   RAX will always point to the valid
> > address.
>
> Which means that NOTRACK should not weaken anything here.  What am I
> missing?

Probably nothing.  Still if the code computing the address to jump to
is exploitable
it might be useful.  Like considering

void foo (int *idx) { switch (*idx) { ...; default:
__builtin_unreachable (); } }

then GCC would elide the range check for the jump table lookup.

I think the main reason for the NOTRACK is code size and the requirement to
place extra ENDBR at each jump target (where any ENDBR is a possible
valid target to use as gadget from a tracked brach).

So NOTRACK branches are bad and ENDBR are both "bad"?

Richard.

>
> Thanks,
> Florian
>

Re: [PATCH] x86: Document -mno-cet-switch

[H.J. Lu]

On Thu, May 12, 2022 at 12:15 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, May 11, 2022 at 9:03 PM Florian Weimer via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > * H. J. Lu:
> >
> > > On Wed, May 11, 2022 at 11:45 AM Florian Weimer <fweimer@redhat.com> wrote:
> > >>
> > >> * H. J. Lu:
> > >>
> > >> >> NOTRACK avoids the need for ENDBR instructions, right?  That's a
> > >> >> hardening improvement, so it should be used by default.
> > >> >
> > >> > NOTRACK weakens IBT since it disables IBT on the indirect jump instruction.
> > >> > GCC uses it in the jump table to avoid ENDBR.
> > >>
> > >> Typical jump table code looks like this:
> > >>
> > >> Dump of assembler code for function __cache_sysconf:
> > >>    0x00000000000f7a80 <+0>:     endbr64
> > >>    0x00000000000f7a84 <+4>:     sub    $0xb9,%edi
> > >>    0x00000000000f7a8a <+10>:    cmp    $0xc,%edi
> > >>    0x00000000000f7a8d <+13>:    ja     0xf7b70 <__cache_sysconf+240>
> > >>    0x00000000000f7a93 <+19>:    lea    0xba926(%rip),%rdx        # 0x1b23c0
> > >>    0x00000000000f7a9a <+26>:    movslq (%rdx,%rdi,4),%rax
> > >>    0x00000000000f7a9e <+30>:    add    %rdx,%rax
> > >>    0x00000000000f7aa1 <+33>:    notrack jmp *%rax
> > >>
> > >> There's no ENDBR instruction between range check, the address
> > >> computation, and the NOTRACK JMP, so it's not possible to redirect that
> > >> JMP to some other place.
> > >
> > > That is the assumption we made.   RAX will always point to the valid
> > > address.
> >
> > Which means that NOTRACK should not weaken anything here.  What am I
> > missing?
>
> Probably nothing.  Still if the code computing the address to jump to
> is exploitable
> it might be useful.  Like considering
>
> void foo (int *idx) { switch (*idx) { ...; default:
> __builtin_unreachable (); } }
>
> then GCC would elide the range check for the jump table lookup.
>
> I think the main reason for the NOTRACK is code size and the requirement to
> place extra ENDBR at each jump target (where any ENDBR is a possible
> valid target to use as gadget from a tracked brach).
>
> So NOTRACK branches are bad and ENDBR are both "bad"?

No, CET won't help here.  __builtin_unreachable indicates truly unreachable
for any inputs.  If not,  __builtin_trap should be used instead.

> Richard.
>
> >
> > Thanks,
> > Florian
> >



-- 
H.J.