* [PATCH] x86/CET: Fix property note parser
@ 2018-07-28 4:49 H.J. Lu
2018-07-29 12:50 ` V2 " H.J. Lu
0 siblings, 1 reply; 2+ messages in thread
From: H.J. Lu @ 2018-07-28 4:49 UTC (permalink / raw)
To: Florian Weimer; +Cc: GNU C Library, Carlos O'Donell
[-- Attachment #1: Type: text/plain, Size: 3380 bytes --]
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.
[-- Attachment #2: 0001-x86-CET-Fix-property-note-parser.patch --]
[-- Type: text/x-patch, Size: 2057 bytes --]
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.
---
sysdeps/x86/dl-prop.h | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 35d3f16a23..9e4d51a71d 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;
+ 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;
+ }
break;
}
+
+ /* 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
--
2.17.1
^ permalink raw reply [flat|nested] 2+ messages in thread
* V2 [PATCH] x86/CET: Fix property note parser
2018-07-28 4:49 [PATCH] x86/CET: Fix property note parser H.J. Lu
@ 2018-07-29 12:50 ` H.J. Lu
0 siblings, 0 replies; 2+ messages in thread
From: H.J. Lu @ 2018-07-29 12:50 UTC (permalink / raw)
To: Florian Weimer, GNU C Library, Carlos O'Donell
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 --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
--
2.17.1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-07-29 12:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-28 4:49 [PATCH] x86/CET: Fix property note parser H.J. Lu
2018-07-29 12:50 ` V2 " H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).