Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware obtain FineIBT, the indirect functions
look like:
__cfi_foo:
endbr64
subl $hash, %r10d
jz 1f
ud2
nop
1:
foo:
endbr64
This is because clang currently does not supress ENDBR emission for
functions it provides a __cfi prologue symbol for.
Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR (if present) when
switching to FineIBT mode.
Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/kernel/alternative.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -940,6 +940,17 @@ static int cfi_rewrite_preamble(s32 *sta
return 0;
}
+static void cfi_rewrite_endbr(s32 *start, s32 *end)
+{
+ s32 *s;
+
+ for (s = start; s < end; s++) {
+ void *addr = (void *)s + *s;
+
+ poison_endbr(addr+16, false);
+ }
+}
+
/* .retpoline_sites */
static int cfi_rand_callers(s32 *start, s32 *end)
{
@@ -1034,14 +1045,19 @@ static void __apply_fineibt(s32 *start_r
return;
case CFI_FINEIBT:
+ /* place the FineIBT preamble at func()-16 */
ret = cfi_rewrite_preamble(start_cfi, end_cfi);
if (ret)
goto err;
+ /* rewrite the callers to target func()-16 */
ret = cfi_rewrite_callers(start_retpoline, end_retpoline);
if (ret)
goto err;
+ /* now that nobody targets func()+0, remove ENDBR there */
+ cfi_rewrite_endbr(start_cfi, end_cfi);
+
if (builtin)
pr_info("Using FineIBT CFI\n");
return;
On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> booting on IBT enabled hardware obtain FineIBT, the indirect functions
> look like:
>
> __cfi_foo:
> endbr64
> subl $hash, %r10d
> jz 1f
> ud2
> nop
> 1:
> foo:
> endbr64
>
> This is because clang currently does not supress ENDBR emission for
> functions it provides a __cfi prologue symbol for.
Should this be considered a bug in Clang?
>
> Having this second ENDBR however makes it possible to elide the CFI
> check. Therefore, we should poison this second ENDBR (if present) when
> switching to FineIBT mode.
>
> Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
> Reported-by: "Milburn, Alyssa" <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Looks like a good work-around.
Acked-by: Kees Cook <[email protected]>
--
Kees Cook
On Tue, Jun 20, 2023 at 2:55 PM Kees Cook <[email protected]> wrote:
>
> On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> > booting on IBT enabled hardware obtain FineIBT, the indirect functions
> > look like:
> >
> > __cfi_foo:
> > endbr64
> > subl $hash, %r10d
> > jz 1f
> > ud2
> > nop
> > 1:
> > foo:
> > endbr64
> >
> > This is because clang currently does not supress ENDBR emission for
> > functions it provides a __cfi prologue symbol for.
>
> Should this be considered a bug in Clang?
The endbr is needed with KCFI if we have FineIBT disabled for some
reason. There's some discussion here:
https://github.com/ClangBuiltLinux/linux/issues/1735
However, since the kernel handles FineIBT patching, it might be
cleaner to let it also poison the extra endbr.
Sami
On Tue, Jun 20, 2023 at 02:55:10PM -0700, Kees Cook wrote:
> On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> > booting on IBT enabled hardware obtain FineIBT, the indirect functions
> > look like:
> >
> > __cfi_foo:
> > endbr64
> > subl $hash, %r10d
> > jz 1f
> > ud2
> > nop
> > 1:
> > foo:
> > endbr64
> >
> > This is because clang currently does not supress ENDBR emission for
> > functions it provides a __cfi prologue symbol for.
>
> Should this be considered a bug in Clang?
No, I don't think so. I was going to say this is perhaps insufficiently
explored space, but upon more consideration I think this is actually
correct behaviour (and I need to write a better Changelog).
The issue is that the compiler generates code for kCFI+IBT, it doesn't
know about FineIBT *at*all*. Additionally, one can inhibit patching of
FineIBT by booting with 'cfi=kcfi' on IBT enabled hardware.
And in that case (kCFI+IBT), we'll do the caller hash check and still
jump to +0, so there really must be an ENDBR there.
Only if we were to dis-allow this combination could we say the ENDBR at
+0 becomes superfluous and should find means for the compiler not emit
it.
> > Having this second ENDBR however makes it possible to elide the CFI
> > check. Therefore, we should poison this second ENDBR (if present) when
> > switching to FineIBT mode.
> >
> > Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
> > Reported-by: "Milburn, Alyssa" <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> Looks like a good work-around.
>
> Acked-by: Kees Cook <[email protected]>
Thanks!
On Wed, Jun 21, 2023 at 10:18:57AM +0200, Peter Zijlstra wrote:
> (and I need to write a better Changelog).
Updated changelog...
---
Subject: x86/fineibt: Poison ENDBR at +0
From: Peter Zijlstra <[email protected]>
Date: Thu, 15 Jun 2023 21:35:48 +0200
Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware obtain FineIBT, the indirect functions
look like:
__cfi_foo:
endbr64
subl $hash, %r10d
jz 1f
ud2
nop
1:
foo:
endbr64
This is because the compiler generates code for kCFI+IBT. In that case
the caller does the hash check and will jump to +0, so there must be
an ENDBR there. The compiler doesn't know about FineIBT at all; also
it is possible to actually use kCFI+IBT when booting with 'cfi=kcfi'
on IBT enabled hardware.
Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR when switching to
FineIBT mode.
Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Sami Tolvanen <[email protected]>
Acked-by: Kees Cook <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/alternative.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1063,6 +1063,17 @@ static int cfi_rewrite_preamble(s32 *sta
return 0;
}
+static void cfi_rewrite_endbr(s32 *start, s32 *end)
+{
+ s32 *s;
+
+ for (s = start; s < end; s++) {
+ void *addr = (void *)s + *s;
+
+ poison_endbr(addr+16, false);
+ }
+}
+
/* .retpoline_sites */
static int cfi_rand_callers(s32 *start, s32 *end)
{
@@ -1157,14 +1168,19 @@ static void __apply_fineibt(s32 *start_r
return;
case CFI_FINEIBT:
+ /* place the FineIBT preamble at func()-16 */
ret = cfi_rewrite_preamble(start_cfi, end_cfi);
if (ret)
goto err;
+ /* rewrite the callers to target func()-16 */
ret = cfi_rewrite_callers(start_retpoline, end_retpoline);
if (ret)
goto err;
+ /* now that nobody targets func()+0, remove ENDBR there */
+ cfi_rewrite_endbr(start_cfi, end_cfi);
+
if (builtin)
pr_info("Using FineIBT CFI\n");
return;
On Tue, Jun 20, 2023 at 05:04:50PM -0700, Sami Tolvanen wrote:
> On Tue, Jun 20, 2023 at 2:55 PM Kees Cook <[email protected]> wrote:
> >
> > On Thu, Jun 15, 2023 at 09:35:48PM +0200, Peter Zijlstra wrote:
> > > Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
> > > booting on IBT enabled hardware obtain FineIBT, the indirect functions
> > > look like:
> > >
> > > __cfi_foo:
> > > endbr64
> > > subl $hash, %r10d
> > > jz 1f
> > > ud2
> > > nop
> > > 1:
> > > foo:
> > > endbr64
> > >
> > > This is because clang currently does not supress ENDBR emission for
> > > functions it provides a __cfi prologue symbol for.
> >
> > Should this be considered a bug in Clang?
>
> The endbr is needed with KCFI if we have FineIBT disabled for some
> reason. There's some discussion here:
>
> https://github.com/ClangBuiltLinux/linux/issues/1735
>
> However, since the kernel handles FineIBT patching, it might be
> cleaner to let it also poison the extra endbr.
That's what I get for replying before reading all replies. Anyway, we're
in agreement.
On Wed, Jun 21, 2023 at 10:48:02AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 10:18:57AM +0200, Peter Zijlstra wrote:
> > (and I need to write a better Changelog).
>
> Updated changelog...
LGTM! Thanks :)
--
Kees Cook
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 04505bbbbb15da950ea0239e328a76a3ad2376e0
Gitweb: https://git.kernel.org/tip/04505bbbbb15da950ea0239e328a76a3ad2376e0
Author: Peter Zijlstra <[email protected]>
AuthorDate: Thu, 15 Jun 2023 21:35:48 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Mon, 10 Jul 2023 09:52:25 +02:00
x86/fineibt: Poison ENDBR at +0
Alyssa noticed that when building the kernel with CFI_CLANG+IBT and
booting on IBT enabled hardware to obtain FineIBT, the indirect
functions look like:
__cfi_foo:
endbr64
subl $hash, %r10d
jz 1f
ud2
nop
1:
foo:
endbr64
This is because the compiler generates code for kCFI+IBT. In that case
the caller does the hash check and will jump to +0, so there must be
an ENDBR there. The compiler doesn't know about FineIBT at all; also
it is possible to actually use kCFI+IBT when booting with 'cfi=kcfi'
on IBT enabled hardware.
Having this second ENDBR however makes it possible to elide the CFI
check. Therefore, we should poison this second ENDBR when switching to
FineIBT mode.
Fixes: 931ab63664f0 ("x86/ibt: Implement FineIBT")
Reported-by: "Milburn, Alyssa" <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Sami Tolvanen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/alternative.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 04b25a2..d77aaab 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1068,6 +1068,17 @@ static int cfi_rewrite_preamble(s32 *start, s32 *end)
return 0;
}
+static void cfi_rewrite_endbr(s32 *start, s32 *end)
+{
+ s32 *s;
+
+ for (s = start; s < end; s++) {
+ void *addr = (void *)s + *s;
+
+ poison_endbr(addr+16, false);
+ }
+}
+
/* .retpoline_sites */
static int cfi_rand_callers(s32 *start, s32 *end)
{
@@ -1162,14 +1173,19 @@ static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
return;
case CFI_FINEIBT:
+ /* place the FineIBT preamble at func()-16 */
ret = cfi_rewrite_preamble(start_cfi, end_cfi);
if (ret)
goto err;
+ /* rewrite the callers to target func()-16 */
ret = cfi_rewrite_callers(start_retpoline, end_retpoline);
if (ret)
goto err;
+ /* now that nobody targets func()+0, remove ENDBR there */
+ cfi_rewrite_endbr(start_cfi, end_cfi);
+
if (builtin)
pr_info("Using FineIBT CFI\n");
return;