V2 [PATCH] x86/CET: Fix property note parser
Commit Message
On Fri, Jul 27, 2018 at 09:49:53PM -0700, H.J. Lu wrote:
> On Fri, Jul 27, 2018 at 1:39 PM, Florian Weimer <fweimer@redhat.com> wrote:
> > On 07/27/2018 08:47 PM, H.J. Lu wrote:
> >>
> >> On Fri, Jul 27, 2018 at 11:26 AM, Florian Weimer <fweimer@redhat.com>
> >> wrote:
> >>>
> >>> On 07/27/2018 08:22 PM, H.J. Lu wrote:
> >>>>
> >>>>
> >>>> - while (1)
> >>>> + while (ptr < ptr_end)
> >>>> {
> >>>> unsigned int type = *(unsigned int *) ptr;
> >>>> unsigned int datasz = *(unsigned int *) (ptr + 4);
> >>>
> >>>
> >>>
> >>> You need 1 byte, but 8 bytes. Why is checking for at least 1 byte
> >>> sufficient here?
> >>>
> >>
> >> There is:
> >>
> >> /* Check for invalid property. */
> >> if (note->n_descsz < 8
> >> || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
> >> break;
> >>
> >> before that. n_descsz should be correct.
> >
> >
> > I do not have a strong opinion regarding this matter. For correctly
> > generated notes, your patch should be fine.
> >
>
> There is a real bug in the note parser. It doesn't check each item. This
> test should fail on CET SDV since IBT is on and endbr64 is missing
> in foo. But it passed:
>
> [hjl@gnu-cet-1 bad-property-1]$ cat y.S
> #include <cet.h>
>
> .text
> .globl main
> .type main, @function
> main:
> .cfi_startproc
> endbr64
> subq $8, %rsp
> .cfi_def_cfa_offset 16
> movq $foo, %rdi
> call *%rdi
> xorl %eax, %eax
> addq $8, %rsp
> .cfi_def_cfa_offset 8
> ret
> .cfi_endproc
> .size main, .-main
> .p2align 4,,15
> .type foo, @function
> foo:
> .cfi_startproc
> ret
> .cfi_endproc
> .size foo, .-foo
>
> #if __SIZEOF_PTRDIFF_T__ == 8
> # define ALIGN 3
> #elif __SIZEOF_PTRDIFF_T__ == 4
> # define ALIGN 2
> #endif
>
> .section ".note.gnu.property", "a"
> .p2align ALIGN
>
> .long 1f - 0f /* name length */
> .long 5f - 2f /* data length */
> .long 5 /* note type */
> 0: .asciz "GNU" /* vendor name */
> 1:
> .p2align ALIGN
> 2:
> .long 1 /* pr_type. */
> .long 4f - 3f /* pr_datasz. */
> 3:
> .long 0x800
> .long 0x800
> 4:
> .p2align ALIGN
> 5:
>
> .section .note.GNU-stack,"",@progbits
> [hjl@gnu-cet-1 bad-property-1]$ make y
> gcc -fcf-protection -c -o y.o y.S
> gcc -fcf-protection -g -o y y.o
> [hjl@gnu-cet-1 build-x86_64-linux]$
> ../../glibc-cet-O3/build-x86_64-linux/elf/ld.so --library-path .
> ~/bugs/libc/bad-property-1/y
> [hjl@gnu-cet-1 build-x86_64-linux]$
>
> This patch fixes it by properly checking each item and stops searching
> when a GNU_PROPERTY_X86_FEATURE_1_AND property is found
> regardless if its size is correct or not. Linker won't generate bad
> GNU_PROPERTY_X86_FEATURE_1_AND. But people may do crazy
> things.
>
> Starting program:
> /export/build/gnu/tools-build/glibc-cet/build-x86_64-linux/elf/ld.so
> --library-path . ~/bugs/libc/bad-property-1/y
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000401130 in ?? ()
> (gdb) disass 0x0000000000401130,+30
> Dump of assembler code from 0x401130 to 0x40114e:
> => 0x0000000000401130: retq
> 0x0000000000401131: nopw %cs:0x0(%rax,%rax,1)
> 0x000000000040113b: nopl 0x0(%rax,%rax,1)
> 0x0000000000401140: endbr64
> 0x0000000000401144: push %r15
> 0x0000000000401146: mov %rdx,%r15
> 0x0000000000401149: push %r14
> 0x000000000040114b: mov %rsi,%r14
> End of assembler dump.
> (gdb)
>
> OK for master?
>
> Thanks.
>
> --
> H.J.
> From 75052a7f08a4261eb7c56885b56970ca96301d36 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 27 Jul 2018 20:34:55 -0700
> Subject: [PATCH] x86/CET: Fix property note parser
>
> GNU_PROPERTY_X86_FEATURE_1_AND may not be the first property item. We
> need to properly check each property item until we reach the end of the
> property or find GNU_PROPERTY_X86_FEATURE_1_AND.
>
> * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Parse
> each property item.
The updated patch. We should return immediately when we find
GNU_PROPERTY_X86_FEATURE_1_AND. There is no need to search further.
OK for master?
H.J.
---
GNU_PROPERTY_X86_FEATURE_1_AND may not be the first property item. We
need to properly check each property item until we reach the end of the
property or find GNU_PROPERTY_X86_FEATURE_1_AND.
* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Parse
each property item until GNU_PROPERTY_X86_FEATURE_1_AND is
found.
---
sysdeps/x86/dl-prop.h | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
@@ -73,7 +73,7 @@ _dl_process_cet_property_note (struct link_map *l,
unsigned char *ptr = (unsigned char *) (note + 1) + 4;
unsigned char *ptr_end = ptr + note->n_descsz;
- while (ptr < ptr_end)
+ do
{
unsigned int type = *(unsigned int *) ptr;
unsigned int datasz = *(unsigned int *) (ptr + 4);
@@ -82,17 +82,23 @@ _dl_process_cet_property_note (struct link_map *l,
if ((ptr + datasz) > ptr_end)
break;
- if (type == GNU_PROPERTY_X86_FEATURE_1_AND
- && datasz == 4)
+ if (type == GNU_PROPERTY_X86_FEATURE_1_AND)
{
- unsigned int feature_1 = *(unsigned int *) ptr;
- if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
- l->l_cet |= lc_ibt;
- if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
- l->l_cet |= lc_shstk;
- break;
+ if (datasz == 4)
+ {
+ unsigned int feature_1 = *(unsigned int *) ptr;
+ if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
+ l->l_cet |= lc_ibt;
+ if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
+ l->l_cet |= lc_shstk;
+ }
+ return;
}
+
+ /* Check the next property item. */
+ ptr += ALIGN_UP (datasz, sizeof (ElfW(Addr)));
}
+ while ((ptr_end - ptr) >= 8);
}
/* NB: Note sections like .note.ABI-tag and .note.gnu.build-id are