diff mbox series

[v26,26/30] ELF: Introduce arch_setup_elf_property()

Message ID 20210427204315.24153-27-yu-cheng.yu@intel.com (mailing list archive)
State New, archived
Headers show
Series Control-flow Enforcement: Shadow Stack | expand

Commit Message

Yu-cheng Yu April 27, 2021, 8:43 p.m. UTC 
An ELF file's .note.gnu.property indicates arch features supported by the
file.  These features are extracted by arch_parse_elf_property() and stored
in 'arch_elf_state'.

Introduce x86 feature definitions and arch_setup_elf_property(), which
enables such features.  The first use-case of this function is Shadow
Stack.

ARM64 is the other arch that has ARCH_USE_GNU_PROPERTY and arch_parse_elf_
property().  Add arch_setup_elf_property() for it.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Martin <Dave.Martin@arm.com>
---
v24:
- Change cet_setup_shstk() to shstk_setup() to reflect function name changes
  relating to the splitting of shadow stack and ibt.

 arch/arm64/include/asm/elf.h |  5 +++++
 arch/x86/Kconfig             |  2 ++
 arch/x86/include/asm/elf.h   | 13 +++++++++++++
 arch/x86/kernel/process_64.c | 32 ++++++++++++++++++++++++++++++++
 fs/binfmt_elf.c              |  4 ++++
 include/linux/elf.h          |  6 ++++++
 include/uapi/linux/elf.h     |  9 +++++++++
 7 files changed, 71 insertions(+)

Comments

Borislav Petkov May 19, 2021, 6:10 p.m. UTC | #1 
On Tue, Apr 27, 2021 at 01:43:11PM -0700, Yu-cheng Yu wrote:
> @@ -1951,6 +1951,8 @@ config X86_SHADOW_STACK
>  	depends on AS_WRUSS
>  	depends on ARCH_HAS_SHADOW_STACK
>  	select ARCH_USES_HIGH_VMA_FLAGS
> +	select ARCH_USE_GNU_PROPERTY
> +	select ARCH_BINFMT_ELF_STATE
		^^^^^^^^

What's that for? Isn't ARCH_USE_GNU_PROPERTY enough?

> +int arch_setup_elf_property(struct arch_elf_state *state)
> +{
> +	int r = 0;
> +
> +	if (!IS_ENABLED(CONFIG_X86_SHADOW_STACK))
> +		return r;
> +
> +	memset(&current->thread.cet, 0, sizeof(struct cet_status));
> +
> +	if (static_cpu_has(X86_FEATURE_SHSTK)) {

	cpu_feature_enabled

> +		if (state->gnu_property & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
> +			r = shstk_setup();
> +	}
> +
> +	return r;
> +}
> +#endif

...

> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 30f68b42eeb5..24ba55ba8278 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -455,4 +455,13 @@ typedef struct elf64_note {
>  /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
>  #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI	(1U << 0)
>  
> +/* .note.gnu.property types for x86: */
> +#define GNU_PROPERTY_X86_FEATURE_1_AND		0xc0000002

Why not 0xc0000001? ARM64 is 0xc0000000...
Yu-cheng Yu May 19, 2021, 10:14 p.m. UTC | #2 
On 5/19/2021 11:10 AM, Borislav Petkov wrote:
> On Tue, Apr 27, 2021 at 01:43:11PM -0700, Yu-cheng Yu wrote:
>> @@ -1951,6 +1951,8 @@ config X86_SHADOW_STACK
>>   	depends on AS_WRUSS
>>   	depends on ARCH_HAS_SHADOW_STACK
>>   	select ARCH_USES_HIGH_VMA_FLAGS
>> +	select ARCH_USE_GNU_PROPERTY
>> +	select ARCH_BINFMT_ELF_STATE
> 		^^^^^^^^
> 
> What's that for? Isn't ARCH_USE_GNU_PROPERTY enough?
> 

ARCH_USE_GNU_PROPERTY is for defining parsing functions, e.g.
	arch_parse_elf_property(),
	arch_setup_property().

ARCH_BINFMT_ELF_STATE is for defining "struct arch_elf_state".

However, those parsing functions take (struct arch_elf_state *) as an 
input.  It probably makes sense to have ARCH_USE_GNU_PROPERTY dependent 
on ARCH_BINFMT_ELF_STATE.  It would be ok as-is too.  ARM people might 
have other plans in mind.

[...]

> 
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index 30f68b42eeb5..24ba55ba8278 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -455,4 +455,13 @@ typedef struct elf64_note {
>>   /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
>>   #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI	(1U << 0)
>>   
>> +/* .note.gnu.property types for x86: */
>> +#define GNU_PROPERTY_X86_FEATURE_1_AND		0xc0000002
> 
> Why not 0xc0000001? ARM64 is 0xc0000000...
> 

I just looked at the ABI document.

ARM has GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000

X86 has:
	GNU_PROPERTY_X86_ISA_1_USED	0xc0000000
	GNU_PROPERTY_X86_ISA_1_NEEDED	0xc0000001
	GNU_PROPERTY_X86_FEATURE_1_AND	0xc0000002

Yu-cheng
Borislav Petkov May 20, 2021, 9:26 a.m. UTC | #3 
On Wed, May 19, 2021 at 03:14:58PM -0700, Yu, Yu-cheng wrote:
> However, those parsing functions take (struct arch_elf_state *) as an input.

Exactly.

> It probably makes sense to have ARCH_USE_GNU_PROPERTY dependent on
> ARCH_BINFMT_ELF_STATE.  It would be ok as-is too.  ARM people might have
> other plans in mind.

Well, let's look at ARM, ARM64 in particular. They have defined struct
arch_elf_state without the ifdeffery in

arch/arm64/include/asm/elf.h

and are using that struct in arch_parse_elf_property().

And they have selected ARCH_BINFMT_ELF_STATE just so that they disable
those dummy accessors in fs/binfmt_elf.c

And you're practically glueing together ARCH_BINFMT_ELF_STATE and
ARCH_USE_GNU_PROPERTY. However, all the functionality is for adding
the gnu property note so I think you should select both but only use
ARCH_USE_GNU_PROPERTY in all the ifdeffery in your patch to at least
have this as simple as possible.

> I just looked at the ABI document.

Which document is that? Link?

> ARM has GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000
> 
> X86 has:
> 	GNU_PROPERTY_X86_ISA_1_USED	0xc0000000
> 	GNU_PROPERTY_X86_ISA_1_NEEDED	0xc0000001
> 	GNU_PROPERTY_X86_FEATURE_1_AND	0xc0000002

Our defines should at least have a comment pointing to that document.

Thx.
Yu-cheng Yu May 20, 2021, 5:18 p.m. UTC | #4 
On 5/20/2021 2:26 AM, Borislav Petkov wrote:
> On Wed, May 19, 2021 at 03:14:58PM -0700, Yu, Yu-cheng wrote:
>> However, those parsing functions take (struct arch_elf_state *) as an input.
> 
> Exactly.
> 
>> It probably makes sense to have ARCH_USE_GNU_PROPERTY dependent on
>> ARCH_BINFMT_ELF_STATE.  It would be ok as-is too.  ARM people might have
>> other plans in mind.
> 
> Well, let's look at ARM, ARM64 in particular. They have defined struct
> arch_elf_state without the ifdeffery in
> 
> arch/arm64/include/asm/elf.h
> 
> and are using that struct in arch_parse_elf_property().
> 
> And they have selected ARCH_BINFMT_ELF_STATE just so that they disable
> those dummy accessors in fs/binfmt_elf.c
> 
> And you're practically glueing together ARCH_BINFMT_ELF_STATE and
> ARCH_USE_GNU_PROPERTY. However, all the functionality is for adding
> the gnu property note so I think you should select both but only use
> ARCH_USE_GNU_PROPERTY in all the ifdeffery in your patch to at least
> have this as simple as possible.
> 

ARM64 has ARCH_USE_GNU_PROPERTY and ARCH_BINFMT_ELF_STATE selected 
unconditionally.  We will do the same for X86_64 and remove the ifdeffery.

>> I just looked at the ABI document.
> 
> Which document is that? Link?
> 
>> ARM has GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000
>>
>> X86 has:
>> 	GNU_PROPERTY_X86_ISA_1_USED	0xc0000000
>> 	GNU_PROPERTY_X86_ISA_1_NEEDED	0xc0000001
>> 	GNU_PROPERTY_X86_FEATURE_1_AND	0xc0000002
> 
> Our defines should at least have a comment pointing to that document.
> 
> Thx.
> 

The latest pdf's are posted here.

https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/x86-64-psABI

Thanks,
Yu-cheng
Borislav Petkov May 20, 2021, 5:35 p.m. UTC | #5 
On Thu, May 20, 2021 at 10:18:10AM -0700, Yu, Yu-cheng wrote:
> The latest pdf's are posted here.
>
> https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/x86-64-psABI

Ah, that document.

Please make sure it is specified over those defines from which document
they're coming from.

Thx.
Matthew Wilcox May 20, 2021, 5:38 p.m. UTC | #6 
On Wed, May 19, 2021 at 03:14:58PM -0700, Yu, Yu-cheng wrote:
> > > +++ b/include/uapi/linux/elf.h
> > > @@ -455,4 +455,13 @@ typedef struct elf64_note {
> > >   /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
> > >   #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI	(1U << 0)
> > > +/* .note.gnu.property types for x86: */
> > > +#define GNU_PROPERTY_X86_FEATURE_1_AND		0xc0000002
> > 
> > Why not 0xc0000001? ARM64 is 0xc0000000...
> > 
> 
> I just looked at the ABI document.
> 
> ARM has GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000
> 
> X86 has:
> 	GNU_PROPERTY_X86_ISA_1_USED	0xc0000000
> 	GNU_PROPERTY_X86_ISA_1_NEEDED	0xc0000001
> 	GNU_PROPERTY_X86_FEATURE_1_AND	0xc0000002

Please add all three, not just the last one.
Yu-cheng Yu May 20, 2021, 5:51 p.m. UTC | #7 
On 5/20/2021 10:35 AM, Borislav Petkov wrote:
> On Thu, May 20, 2021 at 10:18:10AM -0700, Yu, Yu-cheng wrote:
>> The latest pdf's are posted here.
>>
>> https://gitlab.com/x86-psABIs/x86-64-ABI/-/wikis/x86-64-psABI
> 
> Ah, that document.
> 
> Please make sure it is specified over those defines from which document
> they're coming from.
> 

I will do that.
Yu-cheng Yu May 20, 2021, 5:52 p.m. UTC | #8 
On 5/20/2021 10:38 AM, Matthew Wilcox wrote:
> On Wed, May 19, 2021 at 03:14:58PM -0700, Yu, Yu-cheng wrote:
>>>> +++ b/include/uapi/linux/elf.h
>>>> @@ -455,4 +455,13 @@ typedef struct elf64_note {
>>>>    /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
>>>>    #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI	(1U << 0)
>>>> +/* .note.gnu.property types for x86: */
>>>> +#define GNU_PROPERTY_X86_FEATURE_1_AND		0xc0000002
>>>
>>> Why not 0xc0000001? ARM64 is 0xc0000000...
>>>
>>
>> I just looked at the ABI document.
>>
>> ARM has GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000
>>
>> X86 has:
>> 	GNU_PROPERTY_X86_ISA_1_USED	0xc0000000
>> 	GNU_PROPERTY_X86_ISA_1_NEEDED	0xc0000001
>> 	GNU_PROPERTY_X86_FEATURE_1_AND	0xc0000002
> 
> Please add all three, not just the last one.
> 

Ok!
Yu-cheng Yu May 20, 2021, 9:06 p.m. UTC | #9 
On 5/20/2021 10:52 AM, Yu, Yu-cheng wrote:
> On 5/20/2021 10:38 AM, Matthew Wilcox wrote:
>> On Wed, May 19, 2021 at 03:14:58PM -0700, Yu, Yu-cheng wrote:
>>>>> +++ b/include/uapi/linux/elf.h
>>>>> @@ -455,4 +455,13 @@ typedef struct elf64_note {
>>>>>    /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
>>>>>    #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI    (1U << 0)
>>>>> +/* .note.gnu.property types for x86: */
>>>>> +#define GNU_PROPERTY_X86_FEATURE_1_AND        0xc0000002
>>>>
>>>> Why not 0xc0000001? ARM64 is 0xc0000000...
>>>>
>>>
>>> I just looked at the ABI document.
>>>
>>> ARM has GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000
>>>
>>> X86 has:
>>>     GNU_PROPERTY_X86_ISA_1_USED    0xc0000000
>>>     GNU_PROPERTY_X86_ISA_1_NEEDED    0xc0000001
>>>     GNU_PROPERTY_X86_FEATURE_1_AND    0xc0000002
>>
>> Please add all three, not just the last one.
>>
> 
> Ok!

Just found out, I have been reading an older version.  Now 0xc0000000 
and 0xc0000001 have become reserved/not-used.  Maybe I will just put a 
note about that along with the link to the ABI document.

Yu-cheng
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 8d1c8dcb87fd..d37bc7915935 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -281,6 +281,11 @@  static inline int arch_parse_elf_property(u32 type, const void *data,
 	return 0;
 }
 
+static inline int arch_setup_elf_property(struct arch_elf_state *arch)
+{
+	return 0;
+}
+
 static inline int arch_elf_pt_proc(void *ehdr, void *phdr,
 				   struct file *f, bool is_interp,
 				   struct arch_elf_state *state)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 41283f82fd87..77d2e44995d7 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1951,6 +1951,8 @@  config X86_SHADOW_STACK
 	depends on AS_WRUSS
 	depends on ARCH_HAS_SHADOW_STACK
 	select ARCH_USES_HIGH_VMA_FLAGS
+	select ARCH_USE_GNU_PROPERTY
+	select ARCH_BINFMT_ELF_STATE
 	help
 	  Shadow Stack protection is a hardware feature that detects function
 	  return address corruption.  This helps mitigate ROP attacks.
diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 9224d40cdefe..6a131047be8a 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -390,6 +390,19 @@  extern int compat_arch_setup_additional_pages(struct linux_binprm *bprm,
 
 extern bool arch_syscall_is_vdso_sigreturn(struct pt_regs *regs);
 
+#ifdef CONFIG_ARCH_BINFMT_ELF_STATE
+struct arch_elf_state {
+	unsigned int gnu_property;
+};
+
+#define INIT_ARCH_ELF_STATE {	\
+	.gnu_property = 0,	\
+}
+
+#define arch_elf_pt_proc(ehdr, phdr, elf, interp, state) (0)
+#define arch_check_elf(ehdr, interp, interp_ehdr, state) (0)
+#endif
+
 /* Do not change the values. See get_align_mask() */
 enum align_flags {
 	ALIGN_VA_32	= BIT(0),
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index d08307df69ad..d71045b29475 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -835,3 +835,35 @@  unsigned long KSTK_ESP(struct task_struct *task)
 {
 	return task_pt_regs(task)->sp;
 }
+
+#ifdef CONFIG_ARCH_USE_GNU_PROPERTY
+int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
+			    bool compat, struct arch_elf_state *state)
+{
+	if (type != GNU_PROPERTY_X86_FEATURE_1_AND)
+		return 0;
+
+	if (datasz != sizeof(unsigned int))
+		return -ENOEXEC;
+
+	state->gnu_property = *(unsigned int *)data;
+	return 0;
+}
+
+int arch_setup_elf_property(struct arch_elf_state *state)
+{
+	int r = 0;
+
+	if (!IS_ENABLED(CONFIG_X86_SHADOW_STACK))
+		return r;
+
+	memset(&current->thread.cet, 0, sizeof(struct cet_status));
+
+	if (static_cpu_has(X86_FEATURE_SHSTK)) {
+		if (state->gnu_property & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
+			r = shstk_setup();
+	}
+
+	return r;
+}
+#endif
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b12ba98ae9f5..fa665eceba04 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1248,6 +1248,10 @@  static int load_elf_binary(struct linux_binprm *bprm)
 
 	set_binfmt(&elf_format);
 
+	retval = arch_setup_elf_property(&arch_state);
+	if (retval < 0)
+		goto out;
+
 #ifdef ARCH_HAS_SETUP_ADDITIONAL_PAGES
 	retval = ARCH_SETUP_ADDITIONAL_PAGES(bprm, elf_ex, !!interpreter);
 	if (retval < 0)
diff --git a/include/linux/elf.h b/include/linux/elf.h
index c9a46c4e183b..be04d15e937f 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -92,9 +92,15 @@  static inline int arch_parse_elf_property(u32 type, const void *data,
 {
 	return 0;
 }
+
+static inline int arch_setup_elf_property(struct arch_elf_state *arch)
+{
+	return 0;
+}
 #else
 extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
 				   bool compat, struct arch_elf_state *arch);
+extern int arch_setup_elf_property(struct arch_elf_state *arch);
 #endif
 
 #ifdef CONFIG_ARCH_HAVE_ELF_PROT
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 30f68b42eeb5..24ba55ba8278 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -455,4 +455,13 @@  typedef struct elf64_note {
 /* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
 #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI	(1U << 0)
 
+/* .note.gnu.property types for x86: */
+#define GNU_PROPERTY_X86_FEATURE_1_AND		0xc0000002
+
+/* Bits for GNU_PROPERTY_X86_FEATURE_1_AND */
+#define GNU_PROPERTY_X86_FEATURE_1_IBT		0x00000001
+#define GNU_PROPERTY_X86_FEATURE_1_SHSTK	0x00000002
+#define GNU_PROPERTY_X86_FEATURE_1_VALID (GNU_PROPERTY_X86_FEATURE_1_IBT | \
+					   GNU_PROPERTY_X86_FEATURE_1_SHSTK)
+
 #endif /* _UAPI_LINUX_ELF_H */