diff mbox

x86/CET: Don't parse beyond the note end

Message ID CAMe9rOoAQgbtMzftq6UOG_dMvUL3EtAu2Gk3bu_7=Rdt27F+qw@mail.gmail.com
State New, archived
Headers

Commit Message

H.J. Lu July 27, 2018, 6:22 p.m. UTC 
  On Fri, Jul 27, 2018 at 11:20 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 07/27/2018 07:56 PM, H.J. Lu wrote:
>>
>> Yes, I can reproduce it.  Let me take a look.
>
>
> Great.  Did you see the patch I posted?
>

Please this one instead.

Comments

Florian Weimer July 27, 2018, 6:26 p.m. UTC | #1 
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?

I suppose our position is that we do not care about corrupt binaries. 
Is that justification for writing the check this way?

Thanks,
Florian
H.J. Lu July 27, 2018, 6:47 p.m. UTC | #2 
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.
Carlos O'Donell July 27, 2018, 7:22 p.m. UTC | #3 
On 07/27/2018 02:22 PM, H.J. Lu wrote:
> On Fri, Jul 27, 2018 at 11:20 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 07/27/2018 07:56 PM, H.J. Lu wrote:
>>> Yes, I can reproduce it.  Let me take a look.
>>
>> Great.  Did you see the patch I posted?
>>
> Please this one instead.
> 
> 
> -- H.J.
> 
> 
> 0001-x86-CET-Don-t-parse-beyond-the-note-end.patch
> 
> 
> From 8de773a7f9225bb9e42eae1263719ca506670087 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 27 Jul 2018 11:17:04 -0700
> Subject: [PATCH] x86/CET: Don't parse beyond the note end
> 
> Simply check if "ptr < ptr_end" since "ptr" is always incremented by 8.
> 
> 	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Don't
> 	parse beyond the note end.

This is OK for 2.28, limited to x86, and so low impact to other machine
testing going on.

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

> ---
>  sysdeps/x86/dl-prop.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
> index d56e20a6dc..35d3f16a23 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 (1)
> +	  while (ptr < ptr_end)

OK. This only happens with a note that is large enough not to be
considered invalid, but not large enough to contain the data we
need. So when we read the start of the property array, we are
already out of bounds.

Is this an empty property array?

>  	    {
>  	      unsigned int type = *(unsigned int *) ptr;
>  	      unsigned int datasz = *(unsigned int *) (ptr + 4);
> -- 2.17.1

Cheers,
Carlos.
H.J. Lu July 27, 2018, 7:30 p.m. UTC | #4 
On Fri, Jul 27, 2018 at 12:22 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 07/27/2018 02:22 PM, H.J. Lu wrote:
>> On Fri, Jul 27, 2018 at 11:20 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 07/27/2018 07:56 PM, H.J. Lu wrote:
>>>> Yes, I can reproduce it.  Let me take a look.
>>>
>>> Great.  Did you see the patch I posted?
>>>
>> Please this one instead.
>>
>>
>> -- H.J.
>>
>>
>> 0001-x86-CET-Don-t-parse-beyond-the-note-end.patch
>>
>>
>> From 8de773a7f9225bb9e42eae1263719ca506670087 Mon Sep 17 00:00:00 2001
>> From: "H.J. Lu" <hjl.tools@gmail.com>
>> Date: Fri, 27 Jul 2018 11:17:04 -0700
>> Subject: [PATCH] x86/CET: Don't parse beyond the note end
>>
>> Simply check if "ptr < ptr_end" since "ptr" is always incremented by 8.
>>
>>       * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Don't
>>       parse beyond the note end.
>
> This is OK for 2.28, limited to x86, and so low impact to other machine
> testing going on.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
>> ---
>>  sysdeps/x86/dl-prop.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
>> index d56e20a6dc..35d3f16a23 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 (1)
>> +       while (ptr < ptr_end)
>
> OK. This only happens with a note that is large enough not to be
> considered invalid, but not large enough to contain the data we
> need. So when we read the start of the property array, we are
> already out of bounds.
>
> Is this an empty property array?

It has

Displaying notes found in: .note.gnu.property
  Owner                 Data size Description
  GNU                  0x00000020 NT_GNU_PROPERTY_TYPE_0
      Properties: x86 ISA used:
x86 ISA needed:

It has a property with all bits cleared.  I checked in a linker patch:

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=56ad703d56ffe5dc55d5e719a6ec41fd6cf9bfbe

to remove such property.  It should be fixed for binutils 2.31.
Florian Weimer July 27, 2018, 8:39 p.m. UTC | #5 
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.

Thanks,
Florian
diff mbox

Patch

From 8de773a7f9225bb9e42eae1263719ca506670087 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 27 Jul 2018 11:17:04 -0700
Subject: [PATCH] x86/CET: Don't parse beyond the note end

Simply check if "ptr < ptr_end" since "ptr" is always incremented by 8.

	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Don't
	parse beyond the note end.
---
 sysdeps/x86/dl-prop.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index d56e20a6dc..35d3f16a23 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 (1)
+	  while (ptr < ptr_end)
 	    {
 	      unsigned int type = *(unsigned int *) ptr;
 	      unsigned int datasz = *(unsigned int *) (ptr + 4);
-- 
2.17.1