Re: V3 [PATCH] x86/CET: Fix property note parser [BZ #23467]

[Carlos O'Donell <carlos at redhat dot com>]

On 07/30/2018 03:34 PM, H.J. Lu wrote:
> On Mon, Jul 30, 2018 at 12:09 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>> On 30/07/2018 15:56, H.J. Lu wrote:
>>>>> +
>>>>> +  test (bar);
>>>>> +
>>>>> +  return EXIT_FAILURE;
>>>>> +}
>>>>> 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;
>>>>>
>>>> Should we care for overflow here (I guess not since we don't really
>>>> protected against ill-formed elf files in general)?
>>> We do protect against ill-formed notes.  When we get here, the whole
>>> note has been loaded into memory.   There won't be overflow.
>> Indeed, LGTM to me then.
> This is the updated patch I am going to check in today.

Please get final Reviewed-by from the release manager for any patches
which are materially different from those reviewed.

> From bed8cb21b6bd67bed148713259779a8a3e3f844d 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 [BZ #23467]
> 
> GNU_PROPERTY_X86_FEATURE_1_AND may not be the first property item.  We
> need to check each property item until we reach the end of the property
> or find GNU_PROPERTY_X86_FEATURE_1_AND.
> 
> This patch adds 2 tests.  The first test checks if IBT is enabled and
> the second test reads the output from the first test to check if IBT
> is is enabled.  The second second test fails if IBT isn't enabled
> properly.
> 
> 	[BZ #23467]
> 	* sysdeps/unix/sysv/linux/x86/Makefile (tests): Add
> 	tst-cet-property-1 and tst-cet-property-2 if CET is enabled.
> 	(CFLAGS-tst-cet-property-1.o): New.
> 	(ASFLAGS-tst-cet-property-dep-2.o): Likewise.
> 	($(objpfx)tst-cet-property-2): Likewise.
> 	($(objpfx)tst-cet-property-2.out): Likewise.
> 	* sysdeps/unix/sysv/linux/x86/tst-cet-property-1.c: New file.
> 	* sysdeps/unix/sysv/linux/x86/tst-cet-property-2.c: Likewise.
> 	* sysdeps/unix/sysv/linux/x86/tst-cet-property-dep-2.S: Likewise.
> 	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Parse
> 	each property item until GNU_PROPERTY_X86_FEATURE_1_AND is
> 	found.
> ---
>  sysdeps/unix/sysv/linux/x86/Makefile          | 15 +++++
>  .../unix/sysv/linux/x86/tst-cet-property-1.c  | 40 +++++++++++++
>  .../unix/sysv/linux/x86/tst-cet-property-2.c  | 58 ++++++++++++++++++
>  .../sysv/linux/x86/tst-cet-property-dep-2.S   | 60 +++++++++++++++++++
>  sysdeps/x86/dl-prop.h                         | 24 +++++---
>  5 files changed, 188 insertions(+), 9 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/x86/tst-cet-property-1.c
>  create mode 100644 sysdeps/unix/sysv/linux/x86/tst-cet-property-2.c
>  create mode 100644 sysdeps/unix/sysv/linux/x86/tst-cet-property-dep-2.S

OK for 2.28 with 5 comment additions.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/sysdeps/unix/sysv/linux/x86/Makefile b/sysdeps/unix/sysv/linux/x86/Makefile
> index a30fdb1dc1..7dc4e61756 100644
> --- a/sysdeps/unix/sysv/linux/x86/Makefile
> +++ b/sysdeps/unix/sysv/linux/x86/Makefile
> @@ -25,6 +25,21 @@ tests += tst-saved_mask-1
>  endif
>  
>  ifeq ($(enable-cet),yes)
> +ifeq ($(subdir),elf)
> +tests += tst-cet-property-1 tst-cet-property-2
> +
> +CFLAGS-tst-cet-property-1.o += -fcf-protection
> +ASFLAGS-tst-cet-property-dep-2.o += -fcf-protection

OK.

> +
> +$(objpfx)tst-cet-property-2: $(objpfx)tst-cet-property-dep-2.o
> +$(objpfx)tst-cet-property-2.out: $(objpfx)tst-cet-property-2 \
> +				 $(objpfx)tst-cet-property-1.out
> +	env $(run-program-env) $(test-via-rtld-prefix) \
> +	  $(objpfx)tst-cet-property-2 \
> +	  < $(objpfx)tst-cet-property-1.out > $@; \
> +	  $(evaluate-test)

OK.

> +endif
> +
>  ifeq ($(subdir),stdlib)
>  tests += tst-cet-setcontext-1
>  CFLAGS-tst-cet-setcontext-1.c += -mshstk
> diff --git a/sysdeps/unix/sysv/linux/x86/tst-cet-property-1.c b/sysdeps/unix/sysv/linux/x86/tst-cet-property-1.c
> new file mode 100644
> index 0000000000..0a1e155770
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/tst-cet-property-1.c
> @@ -0,0 +1,40 @@
> +/* Test CET property note parser.

OK.

> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <elf.h>
> +#include <tcb-offsets.h>
> +

(1)

Needs a comment explaining why this test runs first before the
second test and what it should output.

> +static int
> +do_test (void)
> +{
> +  unsigned int feature_1;
> +#ifdef __x86_64__
> +# define SEG_REG "fs"
> +#else
> +# define SEG_REG "gs"
> +#endif
> +  asm ("movl %%" SEG_REG ":%P1, %0"
> +       : "=r" (feature_1) : "i" (FEATURE_1_OFFSET));
> +  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT) != 0)
> +    printf ("IBT\n");
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/x86/tst-cet-property-2.c b/sysdeps/unix/sysv/linux/x86/tst-cet-property-2.c
> new file mode 100644
> index 0000000000..fe793e92b6
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/tst-cet-property-2.c
> @@ -0,0 +1,58 @@
> +/* Test CET property note parser.

(2)

The core test should reference bug # please.

> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <support/check.h>
> +
> +extern void bar (void);
> +
> +void
> +__attribute__ ((noclone, noinline))
> +test (void (*func_p) (void))
> +{
> +  func_p ();
> +}
> +
> +static void
> +sig_handler (int signo)
> +{
> +  exit (EXIT_SUCCESS);
> +}
> +

(3)

Needs a comment explaining what this is testing.

> +static int
> +do_test (void)
> +{
> +  char buf[20];
> +
> +  if (scanf ("%20s", buf) != 1)
> +    FAIL_UNSUPPORTED ("IBT not supported");

OK.

> +
> +  if (strcmp (buf, "IBT") != 0)
> +    FAIL_UNSUPPORTED ("IBT not supported");

OK.

> +
> +  TEST_VERIFY_EXIT (signal (SIGSEGV, &sig_handler) != SIG_ERR);
> +
> +  test (bar);
> +
> +  return EXIT_FAILURE;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/x86/tst-cet-property-dep-2.S b/sysdeps/unix/sysv/linux/x86/tst-cet-property-dep-2.S
> new file mode 100644
> index 0000000000..831a8b2e68
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/tst-cet-property-dep-2.S
> @@ -0,0 +1,60 @@
> +/* Test CET property note parser.
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <cet.h>
> +
> +	.text
> +	.p2align 4,,15
> +	.globl	bar
> +	.type	bar, @function
> +bar:
> +	.cfi_startproc
> +	ret
> +	.cfi_endproc
> +	.size	bar, .-bar
> +
> +#if __SIZEOF_PTRDIFF_T__  == 8
> +# define ALIGN 3
> +#elif __SIZEOF_PTRDIFF_T__  == 4
> +# define ALIGN 2
> +#endif
> +

(4)

Needs a comment explaining this construct and what we are testing.

> +	.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:
> +#if __SIZEOF_PTRDIFF_T__  == 8
> +	.long 0x800	
> +	.long 0x800
> +#else
> +	.long 0x08000800
> +#endif
> +4:
> +	.p2align ALIGN
> +5:
> +
> +	.section	.note.GNU-stack,"",@progbits
> 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

OK.

>  	    {
>  	      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)

OK.

>  		{
> -		  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;

OK.

> +		    }

(5)

This needs a comment explaining why we return when datasz != 4.

> +		  return;

>  		}
> +
> +	      /* Check the next property item.  */
> +	      ptr += ALIGN_UP (datasz, sizeof (ElfW(Addr)));

OK.

>  	    }
> +	  while ((ptr_end - ptr) >= 8);

OK.

>  	}
>  
>        /* NB: Note sections like .note.ABI-tag and .note.gnu.build-id are
> -- 2.17.1


-- 
Cheers,
Carlos.