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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Jul 30, 2018 at 11:48 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> On 30/07/2018 15:23, H.J. Lu wrote:
>> 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..465f4f66e8
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/x86/tst-cet-property-2.c
>
>
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  char buf[20];
>> +
>> +  if (scanf ("%20s", buf) != 1)
>> +      return EXIT_UNSUPPORTED;
>> +
>> +  if (strcmp (buf, "IBT") != 0)
>> +      return EXIT_UNSUPPORTED;
>
> Maybe use FAIL_UNSUPPORTED (...) ?

Good point.

>
>> +
>> +  if (signal (SIGSEGV, &sig_handler) == SIG_ERR)
>> +    {
>> +      perror ("installing SIGSEGV handler");
>> +      return EXIT_FAILURE;
>> +    }
>
> Maybe use TEST_VERIFY_EXIT (signal (SIGSEGV, &sig_handler) != SIG_ERR) ?

Good point.

>> +
>> +  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.

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



-- 
H.J.