From patchwork Sun Jul 29 12:50:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 28672 Received: (qmail 114984 invoked by alias); 29 Jul 2018 12:50:15 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 114973 invoked by uid 89); 29 Jul 2018 12:50:14 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.8 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-pl0-f42.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ja2ubpzNs7P6ykBdNNwUOpQIm6cFSpikUZWMgnrOvIE=; b=pvj3kXMde6G6dWht5W8Puvmp66EjYUeyeqNMgUnDq50K61iGYpliA97GoAiJOnfl2k dSyT6IMZv6Z6AAy6RWosWt+TJEcr8TvztiaW3k85iwCc8z7EVzDm3N/DOEhwFn9pB6k6 OIshCWDiQL353nbJYck9zX39ur8Ndyug+mu1iwlJ/SBP/8fy4MT/IPxfQYN9WK/1PCM4 WztJuiCOxvwXjuqzbK5lRgCkEmU2Jr5KgRKr+eaz9aqbdocorsF3XRisb53qngWuykIm ov8hn7C3PqYHLopPIRLqd7bxspZQGHScO2tL0kNbEAgPPCyEux+2xMnizExMCBiZJB9L rOjg== Return-Path: Date: Sun, 29 Jul 2018 05:50:09 -0700 From: "H.J. Lu" To: Florian Weimer , GNU C Library , Carlos O'Donell Subject: V2 [PATCH] x86/CET: Fix property note parser Message-ID: <20180729125009.GA4825@gmail.com> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.0 (2018-05-17) 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 wrote: > > On 07/27/2018 08:47 PM, H.J. Lu wrote: > >> > >> On Fri, Jul 27, 2018 at 11:26 AM, Florian Weimer > >> 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 > > .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" > 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