[PATCH 09/12] aarch64: support BTI enabled binaries
Adhemerval Zanella
adhemerval.zanella@linaro.org
Mon May 11 18:38:04 GMT 2020
On 11/05/2020 08:04, Szabolcs Nagy wrote:
> The 05/07/2020 18:07, Adhemerval Zanella wrote:
>> On 30/04/2020 14:44, Szabolcs Nagy wrote:
>>> From 45c6bce5a691ecec9bba52785bd1f3a4cbc76fd4 Mon Sep 17 00:00:00 2001
>>> From: Sudakshina Das <sudi.das@arm.com>
>>> Date: Tue, 17 Mar 2020 15:54:12 +0000
>>> Subject: [PATCH 09/12] aarch64: support BTI enabled binaries
>>>
>>> Binaries can opt-in to using BTI via an ELF property marking.
>>> The dynamic linker has to then mprotect the executable segments
>>> with PROT_BTI. In case of static linked executables or in case
>>> of the dynamic linker itself, PROT_BTI protection is done by the
>>> operating system.
>>>
>>> On AArch64 glibc uses PT_GNU_PROPERTY instead of PT_NOTE to check
>>> the properties of a binary because PT_NOTE can be unreliable with
>>> old linkers.
>>>
>>> Co-authored-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>>> ---
>>> elf/dl-load.c | 2 +
>>> elf/rtld.c | 2 +
>>> sysdeps/aarch64/Makefile | 4 +
>>> sysdeps/aarch64/dl-bti.c | 54 ++++++
>>> sysdeps/aarch64/dl-prop.h | 170 ++++++++++++++++++
>>> sysdeps/aarch64/linkmap.h | 1 +
>>> sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h | 1 +
>>> sysdeps/unix/sysv/linux/aarch64/bits/mman.h | 31 ++++
>>> .../unix/sysv/linux/aarch64/cpu-features.c | 3 +
>>> .../unix/sysv/linux/aarch64/cpu-features.h | 1 +
>>> 10 files changed, 269 insertions(+)
>>> create mode 100644 sysdeps/aarch64/dl-bti.c
>>> create mode 100644 sysdeps/aarch64/dl-prop.h
>>> create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/mman.h
>>>
>>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>>> index a6b80f9395..0930250619 100644
>>> --- a/elf/dl-load.c
>>> +++ b/elf/dl-load.c
>>> @@ -1145,6 +1145,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>> l->l_relro_size = ph->p_memsz;
>>> break;
>>>
>>> + case PT_GNU_PROPERTY:
>>> + /* Fall through. PT_GNU_PROPERTY holds property notes. */
>>> case PT_NOTE:
>>> if (_dl_process_pt_note (l, ph, fd, fbp))
>>> {
>>
>> This will print the same error message for a failure in _dl_process_pt_note
>> ("cannot process note segment"). Wouldn't be better to use a more specific
>> error message, like "cannot process GNU property segment"?
>
> i was struggling to deal with this, i'm happy
> to create a new hook for pt_gnu_property
> (that's actually cleaner in aarch64, but x86
> will have to continue to look at PT_NOTE for
> the same).
>
> it requires more generic changes though and
> related code repetitions.
I think it might be worth, specially since the _dl_process_pt_note
stills used the __MAX_ALLOCA_CUTOFF (which I think we should replace
with a scratch_buffer).
>
>>> + do
>>> + {
>>> + unsigned int type = *(unsigned int *) ptr;
>>> + unsigned int datasz = *(unsigned int *) (ptr + 4);
>>> +
>>> + /* Property type must be in ascending order. */
>>> + if (type < last_type)
>>> + return;
>>> +
>>> + ptr += 8;
>>> + if ((ptr + datasz) > ptr_end)
>>> + return;
>>> +
>>> + last_type = type;
>>
>> The logic to parse the PT_GNU_PROPERTY is quite similar to the one
>> at sysdeps/x86/dl-prop.h to parse PT_NOTE. Would it be possible to
>> maybe try to consolidate the logic somewhere to avoid this code
>> duplication?
>
> yes it's similar but not the same.
>
> x86 tries to deal with multiple property notes
> which does not happen on aarch64.
>
> i can try to refactor the code and see if
> that works.
Maybe parametrize the logic of number of property notes?
>
>>> + if (ph->p_offset + size <= (size_t) fbp->len)
>>> + note = (const void *) (fbp->buf + ph->p_offset);
>>> + else
>>> + {
>>> + if (size < __MAX_ALLOCA_CUTOFF)
>>> + note = alloca (size);
>>> + else
>>> + note = note_malloced = malloc (size);
>>> + if (note == NULL)
>>> + return -1;
>>> + if (__pread64_nocancel (fd, (void *) note, size, ph->p_offset) != size)
>>> + {
>>> + if (note_malloced)
>>> + free (note_malloced);
>>> + return -1;
>>
>> I wonder if we should use a scratch_buffer here instead.
>
> this logic is copied from x86, on aarch64 local buffer
> should work with current linkers (since they will only
> add at most one gnu property note to PT_GNU_PROPERTY),
> but we don't know what happens in the future so the
> malloc fallback is probably required.
>
> i think ideally the segment is mmaped into memory and
> we can just use that, but i assumed the logic is there
> for a reason on x86.
Would mmap/unmap the segment a better strategy than use a small stack
allocation for most of cases with a malloc fallback (which I think
won't be used unless a ill formatted note)?
>
>>> +++ b/sysdeps/aarch64/linkmap.h
>>> @@ -20,4 +20,5 @@ struct link_map_machine
>>> {
>>> ElfW(Addr) plt; /* Address of .plt */
>>> void *tlsdesc_table; /* Address of TLS descriptor hash table. */
>>> + int bti_guarded; /* Branch Target Identification mechanism enabled. */
>>
>> Maybe bool here?
>
> ok.
>
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
>>> @@ -72,3 +72,4 @@
>>> #define HWCAP2_BF16 (1 << 14)
>>> #define HWCAP2_DGH (1 << 15)
>>> #define HWCAP2_RNG (1 << 16)
>>> +#define HWCAP2_BTI (1 << 17)
>>
>> This it not yet upstream on Linus tree (6e7f2eacf098), but follows
>> the arm64/for-next/bti branch (8ef8f360cf30be12).
>
> yes, now renamed to bti-user (because the kernel
> code can also use bti protection) it is scheduled
> for next linux, this patchset depends on that work,
> but they have to be tested together.
Ack, we just need to avoid another BZ#25971.
>
>>> +/* AArch64 specific definitions, should be in sync with
>>> + arch/arm64/include/uapi/asm/mman.h. */
>>> +
>>> +#define PROT_BTI 0x10
>>
>> Linux specific flags should be protected by __USE_MISC.
>
> in posix the PROT_ prefix is reserved for sys/mman.h
> so there is no namespace issue with this.
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
>
> see also PROT_SAO on powerpc.
>
MMAP_ as well, but we still protect Linux ones with __USE_MISC.
The conform/data/sys/mman.h-data does not trigger a namespace issue
for neither MMAP_ nor PROT_, so I am not sure which would be the
best policy here.
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
>>> @@ -64,6 +64,7 @@ struct cpu_features
>>> {
>>> uint64_t midr_el1;
>>> unsigned zva_size;
>>> + int bti;
>>
>> Maybe bool here?
>
> ok.
>
More information about the Libc-alpha
mailing list