diff mbox

V2 [PATCH] x86/CET: Fix property note parser

Message ID 20180729125009.GA4825@gmail.com
State New, archived
Headers show

Commit Message

H.J. Lu July 29, 2018, 12:50 p.m. UTC
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(-)
diff mbox

Patch

diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 35d3f16a23..d9e0770e29 100644
--- a/sysdeps/x86/dl-prop.h
+++ b/sysdeps/x86/dl-prop.h
@@ -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