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

Message ID 20180729125009.GA4825@gmail.com
State New
Headers show
Series
  • V2 [PATCH] x86/CET: Fix property note parser
Related show

Commit Message

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

-- 
2.17.1

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