[PATCH] x86/CET: Fix property note parser

[H.J. Lu]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-x86-CET-Fix-property-note-parser.patch
Type: text/x-patch
Size: 2057 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20180728/8a140cb6/attachment.bin>