[edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note

Ard Biesheuvel posted 17 patches 1 year, 5 months ago
There is a newer version of this series
[edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Ard Biesheuvel 1 year, 5 months ago
Implement a CPP macro that can be called from .S files to emit the .note
section carrying the annotation that informs the linker that the object
file is compatible with BTI control flow integrity checks.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
index abe2571245c665f3..11814f1ffaef698a 100644
--- a/MdePkg/Include/AArch64/ProcessorBind.h
+++ b/MdePkg/Include/AArch64/ProcessorBind.h
@@ -186,6 +186,37 @@ typedef INT64 INTN;
 #define GCC_ASM_IMPORT(func__)  \
          .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
 
+#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
+#define AARCH64_BTI(__type)                                        \
+    .ifnc         __type,                                         ;\
+    bti           __type                                          ;\
+    .endif                                                        ;\
+    .ifndef       .Lgnu_bti_notesize                              ;\
+    .pushsection  .note.gnu.property, "a"                         ;\
+    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
+    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
+    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
+    .align        3                                               ;\
+    .long         .Lnamesize                                      ;\
+    .long         .Lgnu_bti_notesize                              ;\
+    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
+0:  .asciz        "GNU"                                           ;\
+    .set          .Lnamesize, . - 0b                              ;\
+    .align        3                                               ;\
+1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
+    .long         .Lvalsize                                       ;\
+2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
+    .set          .Lvalsize, . - 2b                               ;\
+    .align        3                                               ;\
+    .set          .Lgnu_bti_notesize, . - 1b                      ;\
+    .popsection                                                   ;\
+    .endif
+#endif
+
+#endif
+
+#ifndef AARCH64_BTI
+#define AARCH64_BTI(__type)
 #endif
 
 /**
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101925): https://edk2.groups.io/g/devel/message/101925
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Pedro Falcato 1 year, 5 months ago
On Mon, Mar 27, 2023 at 12:01 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Implement a CPP macro that can be called from .S files to emit the .note
> section carrying the annotation that informs the linker that the object
> file is compatible with BTI control flow integrity checks.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index abe2571245c665f3..11814f1ffaef698a 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -186,6 +186,37 @@ typedef INT64 INTN;
>  #define GCC_ASM_IMPORT(func__)  \
>           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>
> +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> +#define AARCH64_BTI(__type)                                        \
> +    .ifnc         __type,                                         ;\
> +    bti           __type                                          ;\
> +    .endif                                                        ;\
> +    .ifndef       .Lgnu_bti_notesize                              ;\
> +    .pushsection  .note.gnu.property, "a"                         ;\
> +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> +    .align        3                                               ;\
> +    .long         .Lnamesize                                      ;\
> +    .long         .Lgnu_bti_notesize                              ;\
> +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> +0:  .asciz        "GNU"                                           ;\
> +    .set          .Lnamesize, . - 0b                              ;\
> +    .align        3                                               ;\
> +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> +    .long         .Lvalsize                                       ;\
> +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> +    .set          .Lvalsize, . - 2b                               ;\
> +    .align        3                                               ;\
> +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> +    .popsection                                                   ;\
> +    .endif
> +#endif
> +
> +#endif
> +
> +#ifndef AARCH64_BTI
> +#define AARCH64_BTI(__type)
>  #endif
>
>  /**
> --
> 2.39.2

Patch-set wide comment: is there any chance we could take this
opportunity to introduce a global ASM_FUNC (or a more Linux-named
ENTRY(FuncName))?
It seems to be that the current way is a bit error prone and you end
up repeating yourself quite a bit with:

ASM_PFX(Foo):
AARCH64_BTI(c)
<code>

having a:
ASM_FUNC(Foo)
<code>

that does proper PFX and BTI expansion internally seems better to me.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101959): https://edk2.groups.io/g/devel/message/101959
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Leif Lindholm 1 year, 5 months ago
On Mon, Mar 27, 2023 at 15:12:29 +0100, Pedro Falcato wrote:
> On Mon, Mar 27, 2023 at 12:01 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Implement a CPP macro that can be called from .S files to emit the .note
> > section carrying the annotation that informs the linker that the object
> > file is compatible with BTI control flow integrity checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index abe2571245c665f3..11814f1ffaef698a 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> >  #define GCC_ASM_IMPORT(func__)  \
> >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> >
> > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > +#define AARCH64_BTI(__type)                                        \
> > +    .ifnc         __type,                                         ;\
> > +    bti           __type                                          ;\
> > +    .endif                                                        ;\
> > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > +    .pushsection  .note.gnu.property, "a"                         ;\
> > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > +    .align        3                                               ;\
> > +    .long         .Lnamesize                                      ;\
> > +    .long         .Lgnu_bti_notesize                              ;\
> > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > +0:  .asciz        "GNU"                                           ;\
> > +    .set          .Lnamesize, . - 0b                              ;\
> > +    .align        3                                               ;\
> > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > +    .long         .Lvalsize                                       ;\
> > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > +    .set          .Lvalsize, . - 2b                               ;\
> > +    .align        3                                               ;\
> > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > +    .popsection                                                   ;\
> > +    .endif
> > +#endif
> > +
> > +#endif
> > +
> > +#ifndef AARCH64_BTI
> > +#define AARCH64_BTI(__type)
> >  #endif
> >
> >  /**
> > --
> > 2.39.2
> 
> Patch-set wide comment: is there any chance we could take this
> opportunity to introduce a global ASM_FUNC (or a more Linux-named
> ENTRY(FuncName))?
> It seems to be that the current way is a bit error prone and you end
> up repeating yourself quite a bit with:
> 
> ASM_PFX(Foo):
> AARCH64_BTI(c)
> <code>
> 
> having a:
> ASM_FUNC(Foo)
> <code>
> 
> that does proper PFX and BTI expansion internally seems better to me.

I was thinking while looking at this patch that ASM_FUNC could
probably do with moving over to this file from AsmMacroIoLibV8.h.
I didn't take the thought far enough to consider including the BTI
bits in that, but I guess that could make sense.

/
    Leif

> 
> -- 
> Pedro
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101960): https://edk2.groups.io/g/devel/message/101960
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Ard Biesheuvel 1 year, 5 months ago
On Mon, 27 Mar 2023 at 16:24, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 15:12:29 +0100, Pedro Falcato wrote:
> > On Mon, Mar 27, 2023 at 12:01 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > Implement a CPP macro that can be called from .S files to emit the .note
> > > section carrying the annotation that informs the linker that the object
> > > file is compatible with BTI control flow integrity checks.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index abe2571245c665f3..11814f1ffaef698a 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> > >  #define GCC_ASM_IMPORT(func__)  \
> > >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> > >
> > > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > > +#define AARCH64_BTI(__type)                                        \
> > > +    .ifnc         __type,                                         ;\
> > > +    bti           __type                                          ;\
> > > +    .endif                                                        ;\
> > > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > > +    .pushsection  .note.gnu.property, "a"                         ;\
> > > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > > +    .align        3                                               ;\
> > > +    .long         .Lnamesize                                      ;\
> > > +    .long         .Lgnu_bti_notesize                              ;\
> > > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > > +0:  .asciz        "GNU"                                           ;\
> > > +    .set          .Lnamesize, . - 0b                              ;\
> > > +    .align        3                                               ;\
> > > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > > +    .long         .Lvalsize                                       ;\
> > > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > > +    .set          .Lvalsize, . - 2b                               ;\
> > > +    .align        3                                               ;\
> > > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > > +    .popsection                                                   ;\
> > > +    .endif
> > > +#endif
> > > +
> > > +#endif
> > > +
> > > +#ifndef AARCH64_BTI
> > > +#define AARCH64_BTI(__type)
> > >  #endif
> > >
> > >  /**
> > > --
> > > 2.39.2
> >
> > Patch-set wide comment: is there any chance we could take this
> > opportunity to introduce a global ASM_FUNC (or a more Linux-named
> > ENTRY(FuncName))?
> > It seems to be that the current way is a bit error prone and you end
> > up repeating yourself quite a bit with:
> >
> > ASM_PFX(Foo):
> > AARCH64_BTI(c)
> > <code>
> >
> > having a:
> > ASM_FUNC(Foo)
> > <code>
> >
> > that does proper PFX and BTI expansion internally seems better to me.
>
> I was thinking while looking at this patch that ASM_FUNC could
> probably do with moving over to this file from AsmMacroIoLibV8.h.
> I didn't take the thought far enough to consider including the BTI
> bits in that, but I guess that could make sense.
>

Yeah, but I'd prefer it if we could do that globally, and not just for
AArch64 as part of this series.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102152): https://edk2.groups.io/g/devel/message/102152
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Leif Lindholm 1 year, 5 months ago
On Thu, Mar 30, 2023 at 09:28:45 +0200, Ard Biesheuvel wrote:
> > > Patch-set wide comment: is there any chance we could take this
> > > opportunity to introduce a global ASM_FUNC (or a more Linux-named
> > > ENTRY(FuncName))?
> > > It seems to be that the current way is a bit error prone and you end
> > > up repeating yourself quite a bit with:
> > >
> > > ASM_PFX(Foo):
> > > AARCH64_BTI(c)
> > > <code>
> > >
> > > having a:
> > > ASM_FUNC(Foo)
> > > <code>
> > >
> > > that does proper PFX and BTI expansion internally seems better to me.
> >
> > I was thinking while looking at this patch that ASM_FUNC could
> > probably do with moving over to this file from AsmMacroIoLibV8.h.
> > I didn't take the thought far enough to consider including the BTI
> > bits in that, but I guess that could make sense.
> 
> Yeah, but I'd prefer it if we could do that globally, and not just for
> AArch64 as part of this series.

Yeah, no issue with that.
This set adds primitives that will be useful for that in future.

/
    Leif


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102173): https://edk2.groups.io/g/devel/message/102173
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Leif Lindholm 1 year, 5 months ago
On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> Implement a CPP macro that can be called from .S files to emit the .note
> section carrying the annotation that informs the linker that the object
> file is compatible with BTI control flow integrity checks.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index abe2571245c665f3..11814f1ffaef698a 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -186,6 +186,37 @@ typedef INT64 INTN;
>  #define GCC_ASM_IMPORT(func__)  \
>           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>  
> +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> +#define AARCH64_BTI(__type)                                        \
> +    .ifnc         __type,                                         ;\
> +    bti           __type                                          ;\
> +    .endif                                                        ;\

This didn't jump out at me until looking at the consumer side.
This overlays two different sets of functionality depending on whether
an option is given to the macro or not, which feels semantically
suboptimal to me (i.e. it makes my head hurt).

Could we split this into two macros - one that inserts the instruction
and one that inserts the note, and expand the latter in the former?

/
    Leif

> +    .ifndef       .Lgnu_bti_notesize                              ;\
> +    .pushsection  .note.gnu.property, "a"                         ;\
> +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> +    .align        3                                               ;\
> +    .long         .Lnamesize                                      ;\
> +    .long         .Lgnu_bti_notesize                              ;\
> +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> +0:  .asciz        "GNU"                                           ;\
> +    .set          .Lnamesize, . - 0b                              ;\
> +    .align        3                                               ;\
> +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> +    .long         .Lvalsize                                       ;\
> +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> +    .set          .Lvalsize, . - 2b                               ;\
> +    .align        3                                               ;\
> +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> +    .popsection                                                   ;\
> +    .endif
> +#endif
> +
> +#endif
> +
> +#ifndef AARCH64_BTI
> +#define AARCH64_BTI(__type)
>  #endif
>  
>  /**
> -- 
> 2.39.2
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101948): https://edk2.groups.io/g/devel/message/101948
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Ard Biesheuvel 1 year, 5 months ago
On Mon, 27 Mar 2023 at 14:45, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > Implement a CPP macro that can be called from .S files to emit the .note
> > section carrying the annotation that informs the linker that the object
> > file is compatible with BTI control flow integrity checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index abe2571245c665f3..11814f1ffaef698a 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> >  #define GCC_ASM_IMPORT(func__)  \
> >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> >
> > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > +#define AARCH64_BTI(__type)                                        \
> > +    .ifnc         __type,                                         ;\
> > +    bti           __type                                          ;\
> > +    .endif                                                        ;\
>
> This didn't jump out at me until looking at the consumer side.
> This overlays two different sets of functionality depending on whether
> an option is given to the macro or not, which feels semantically
> suboptimal to me (i.e. it makes my head hurt).
>
> Could we split this into two macros - one that inserts the instruction
> and one that inserts the note, and expand the latter in the former?
>

Yeah, fair point.

So AARCH64_BTI_NOTE() to emit the note, and AARCH64_BTI() to emit the
instruction as well as the note.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101949): https://edk2.groups.io/g/devel/message/101949
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Leif Lindholm 1 year, 5 months ago
On Mon, Mar 27, 2023 at 14:46:49 +0200, Ard Biesheuvel wrote:
> On Mon, 27 Mar 2023 at 14:45, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > > Implement a CPP macro that can be called from .S files to emit the .note
> > > section carrying the annotation that informs the linker that the object
> > > file is compatible with BTI control flow integrity checks.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index abe2571245c665f3..11814f1ffaef698a 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> > >  #define GCC_ASM_IMPORT(func__)  \
> > >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> > >
> > > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > > +#define AARCH64_BTI(__type)                                        \
> > > +    .ifnc         __type,                                         ;\
> > > +    bti           __type                                          ;\
> > > +    .endif                                                        ;\
> >
> > This didn't jump out at me until looking at the consumer side.
> > This overlays two different sets of functionality depending on whether
> > an option is given to the macro or not, which feels semantically
> > suboptimal to me (i.e. it makes my head hurt).
> >
> > Could we split this into two macros - one that inserts the instruction
> > and one that inserts the note, and expand the latter in the former?
> >
> 
> Yeah, fair point.
> 
> So AARCH64_BTI_NOTE() to emit the note, and AARCH64_BTI() to emit the
> instruction as well as the note.

Yeah, exacty.

> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101950): https://edk2.groups.io/g/devel/message/101950
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Leif Lindholm 1 year, 5 months ago
On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> Implement a CPP macro that can be called from .S files to emit the .note
> section carrying the annotation that informs the linker that the object
> file is compatible with BTI control flow integrity checks.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index abe2571245c665f3..11814f1ffaef698a 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -186,6 +186,37 @@ typedef INT64 INTN;
>  #define GCC_ASM_IMPORT(func__)  \
>           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>  
> +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> +#define AARCH64_BTI(__type)                                        \
> +    .ifnc         __type,                                         ;\
> +    bti           __type                                          ;\
> +    .endif                                                        ;\
> +    .ifndef       .Lgnu_bti_notesize                              ;\
> +    .pushsection  .note.gnu.property, "a"                         ;\
> +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> +    .align        3                                               ;\
> +    .long         .Lnamesize                                      ;\
> +    .long         .Lgnu_bti_notesize                              ;\
> +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> +0:  .asciz        "GNU"                                           ;\
> +    .set          .Lnamesize, . - 0b                              ;\
> +    .align        3                                               ;\
> +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> +    .long         .Lvalsize                                       ;\
> +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> +    .set          .Lvalsize, . - 2b                               ;\
> +    .align        3                                               ;\
> +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> +    .popsection                                                   ;\
> +    .endif
> +#endif
> +
> +#endif

Is that second endif superflouous?

/
    Leif

> +
> +#ifndef AARCH64_BTI
> +#define AARCH64_BTI(__type)
>  #endif
>  
>  /**
> -- 
> 2.39.2
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101945): https://edk2.groups.io/g/devel/message/101945
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Ard Biesheuvel 1 year, 5 months ago
On Mon, 27 Mar 2023 at 13:53, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > Implement a CPP macro that can be called from .S files to emit the .note
> > section carrying the annotation that informs the linker that the object
> > file is compatible with BTI control flow integrity checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index abe2571245c665f3..11814f1ffaef698a 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> >  #define GCC_ASM_IMPORT(func__)  \
> >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> >
> > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > +#define AARCH64_BTI(__type)                                        \
> > +    .ifnc         __type,                                         ;\
> > +    bti           __type                                          ;\
> > +    .endif                                                        ;\
> > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > +    .pushsection  .note.gnu.property, "a"                         ;\
> > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > +    .align        3                                               ;\
> > +    .long         .Lnamesize                                      ;\
> > +    .long         .Lgnu_bti_notesize                              ;\
> > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > +0:  .asciz        "GNU"                                           ;\
> > +    .set          .Lnamesize, . - 0b                              ;\
> > +    .align        3                                               ;\
> > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > +    .long         .Lvalsize                                       ;\
> > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > +    .set          .Lvalsize, . - 2b                               ;\
> > +    .align        3                                               ;\
> > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > +    .popsection                                                   ;\
> > +    .endif
> > +#endif
> > +
> > +#endif
>
> Is that second endif superflouous?
>

No the diff just turned out a bit odd.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101946): https://edk2.groups.io/g/devel/message/101946
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 01/17] MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note
Posted by Leif Lindholm 1 year, 5 months ago
On Mon, Mar 27, 2023 at 14:15:09 +0200, Ard Biesheuvel wrote:
> On Mon, 27 Mar 2023 at 13:53, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 13:00:56 +0200, Ard Biesheuvel wrote:
> > > Implement a CPP macro that can be called from .S files to emit the .note
> > > section carrying the annotation that informs the linker that the object
> > > file is compatible with BTI control flow integrity checks.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 31 ++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index abe2571245c665f3..11814f1ffaef698a 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -186,6 +186,37 @@ typedef INT64 INTN;
> > >  #define GCC_ASM_IMPORT(func__)  \
> > >           .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> > >
> > > +#if defined(__ARM_FEATURE_BTI_DEFAULT) && __ARM_FEATURE_BTI_DEFAULT == 1
> > > +#define AARCH64_BTI(__type)                                        \
> > > +    .ifnc         __type,                                         ;\
> > > +    bti           __type                                          ;\
> > > +    .endif                                                        ;\
> > > +    .ifndef       .Lgnu_bti_notesize                              ;\
> > > +    .pushsection  .note.gnu.property, "a"                         ;\
> > > +    .set          NT_GNU_PROPERTY_TYPE_0, 0x5                     ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_AND, 0xc0000000  ;\
> > > +    .set          GNU_PROPERTY_AARCH64_FEATURE_1_BTI, 0x1         ;\
> > > +    .align        3                                               ;\
> > > +    .long         .Lnamesize                                      ;\
> > > +    .long         .Lgnu_bti_notesize                              ;\
> > > +    .long         NT_GNU_PROPERTY_TYPE_0                          ;\
> > > +0:  .asciz        "GNU"                                           ;\
> > > +    .set          .Lnamesize, . - 0b                              ;\
> > > +    .align        3                                               ;\
> > > +1:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_AND              ;\
> > > +    .long         .Lvalsize                                       ;\
> > > +2:  .long         GNU_PROPERTY_AARCH64_FEATURE_1_BTI              ;\
> > > +    .set          .Lvalsize, . - 2b                               ;\
> > > +    .align        3                                               ;\
> > > +    .set          .Lgnu_bti_notesize, . - 1b                      ;\
> > > +    .popsection                                                   ;\
> > > +    .endif
> > > +#endif
> > > +
> > > +#endif
> >
> > Is that second endif superflouous?
> >
> 
> No the diff just turned out a bit odd.

Oh, I see - it's because you add this inside the if __GNUC__ || __clang__
and add the #ifndef AARCH64_BTI outside it. Which is necessary. Moving
on.

/
    Leif




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101952): https://edk2.groups.io/g/devel/message/101952
Mute This Topic: https://groups.io/mt/97879282/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-