[PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Andrew Cooper]

A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
bytes in adjacent stringtable entries.

Instead, check for namesz exactly equal to 4, and use memcmp() with an
explicit size.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

Noticed while auditing Xen's use of strncmp() for the command line patch.
---
 xen/common/version.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/common/version.c b/xen/common/version.c
index 223cb52..1df7e78 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -97,17 +97,17 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
     if ( NT_GNU_BUILD_ID != n->type )
         return -ENODATA;
 
-    if ( n->namesz + n->descsz < n->namesz )
+    if ( n->namesz != 4 /* GNU\0 */)
         return -EINVAL;
 
-    if ( n->namesz < 4 /* GNU\0 */)
+    if ( n->namesz + n->descsz < n->namesz )
         return -EINVAL;
 
     if ( n->namesz + n->descsz > n_sz - sizeof(*n) )
         return -EINVAL;
 
     /* Sanity check, name should be "GNU" for ld-generated build-id. */
-    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
+    if ( memcmp(ELFNOTE_NAME(n), "GNU", 4) != 0 )
         return -ENODATA;
 
     if ( len )
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Roger Pau Monné]

On Mon, Dec 31, 2018 at 05:34:25PM +0000, Andrew Cooper wrote:
> A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
> bytes in adjacent stringtable entries.
> 
> Instead, check for namesz exactly equal to 4, and use memcmp() with an
> explicit size.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

This LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Albeit I wonder the usefulness of some of the checks performed by this
function.

I'm not sure the point of the 'n->namesz + n->descsz < n->namesz'
check, I assume this is an overflow check. And then 'n->namesz +
n->descsz > n_sz - sizeof(*n)' should rather be 'n->namesz + n->descsz
!= n_sz - sizeof(*n)' I think.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Wei Liu]

On Mon, Dec 31, 2018 at 05:34:25PM +0000, Andrew Cooper wrote:
> A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
> bytes in adjacent stringtable entries.
> 
> Instead, check for namesz exactly equal to 4, and use memcmp() with an
> explicit size.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> Noticed while auditing Xen's use of strncmp() for the command line patch.
> ---
>  xen/common/version.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/version.c b/xen/common/version.c
> index 223cb52..1df7e78 100644
> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -97,17 +97,17 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>      if ( NT_GNU_BUILD_ID != n->type )
>          return -ENODATA;
>  
> -    if ( n->namesz + n->descsz < n->namesz )
> +    if ( n->namesz != 4 /* GNU\0 */)
>          return -EINVAL;
>  
> -    if ( n->namesz < 4 /* GNU\0 */)
> +    if ( n->namesz + n->descsz < n->namesz )

The reordering of two predicates doesn't seem to serve any particular
purpose? You could've just changed "<" to "!=" for less code churn?

>          return -EINVAL;
>  
>      if ( n->namesz + n->descsz > n_sz - sizeof(*n) )
>          return -EINVAL;
>  
>      /* Sanity check, name should be "GNU" for ld-generated build-id. */
> -    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
> +    if ( memcmp(ELFNOTE_NAME(n), "GNU", 4) != 0 )

OOI what is the advantage of memcmp compared to strncmp?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Andrew Cooper]

On 02/01/2019 10:43, Wei Liu wrote:
> On Mon, Dec 31, 2018 at 05:34:25PM +0000, Andrew Cooper wrote:
>> A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
>> bytes in adjacent stringtable entries.
>>
>> Instead, check for namesz exactly equal to 4, and use memcmp() with an
>> explicit size.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> Noticed while auditing Xen's use of strncmp() for the command line patch.
>> ---
>>  xen/common/version.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/common/version.c b/xen/common/version.c
>> index 223cb52..1df7e78 100644
>> --- a/xen/common/version.c
>> +++ b/xen/common/version.c
>> @@ -97,17 +97,17 @@ int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>>      if ( NT_GNU_BUILD_ID != n->type )
>>          return -ENODATA;
>>  
>> -    if ( n->namesz + n->descsz < n->namesz )
>> +    if ( n->namesz != 4 /* GNU\0 */)
>>          return -EINVAL;
>>  
>> -    if ( n->namesz < 4 /* GNU\0 */)
>> +    if ( n->namesz + n->descsz < n->namesz )
> The reordering of two predicates doesn't seem to serve any particular
> purpose? You could've just changed "<" to "!=" for less code churn?

Logically, the != 4 check should be ahead of the truncation check, but
yes - patch did collapse it into something rather harder to read in this
case.

>
>>          return -EINVAL;
>>  
>>      if ( n->namesz + n->descsz > n_sz - sizeof(*n) )
>>          return -EINVAL;
>>  
>>      /* Sanity check, name should be "GNU" for ld-generated build-id. */
>> -    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
>> +    if ( memcmp(ELFNOTE_NAME(n), "GNU", 4) != 0 )
> OOI what is the advantage of memcmp compared to strncmp?

Erm.  I suppose in this exact case, personal preference.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Jan Beulich]

>>> On 02.01.19 at 11:43, <wei.liu2@citrix.com> wrote:
> On Mon, Dec 31, 2018 at 05:34:25PM +0000, Andrew Cooper wrote:
>>      /* Sanity check, name should be "GNU" for ld-generated build-id. */
>> -    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
>> +    if ( memcmp(ELFNOTE_NAME(n), "GNU", 4) != 0 )
> 
> OOI what is the advantage of memcmp compared to strncmp?

memcmp() generally is more performant than strncmp(), due to it not
needing to look for nul terminators.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Jan Beulich]

>>> On 31.12.18 at 18:34, <andrew.cooper3@citrix.com> wrote:
> A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
> bytes in adjacent stringtable entries.
> 
> Instead, check for namesz exactly equal to 4,

Is that a requirement spelled out anywhere? Till now I've been
under the impression that e.g. 8 bytes of name are fine as well,
as long as the first four of them are "GNU\0".

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Andrew Cooper]

On 07/01/2019 10:36, Jan Beulich wrote:
>>>> On 31.12.18 at 18:34, <andrew.cooper3@citrix.com> wrote:
>> A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
>> bytes in adjacent stringtable entries.
>>
>> Instead, check for namesz exactly equal to 4,
> Is that a requirement spelled out anywhere? Till now I've been
> under the impression that e.g. 8 bytes of name are fine as well,
> as long as the first four of them are "GNU\0".

No idea, but if this is true then we've got bigger problems with parsing
the notes.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/build-id: Fix xen_build_id_check() to be robust against malformed notes

[Jan Beulich]

>>> On 07.01.19 at 18:34, <andrew.cooper3@citrix.com> wrote:
> On 07/01/2019 10:36, Jan Beulich wrote:
>>>>> On 31.12.18 at 18:34, <andrew.cooper3@citrix.com> wrote:
>>> A NT_GNU_BUILD_ID with namesz longer than 4 will cause the strncmp() to use
>>> bytes in adjacent stringtable entries.
>>>
>>> Instead, check for namesz exactly equal to 4,
>> Is that a requirement spelled out anywhere? Till now I've been
>> under the impression that e.g. 8 bytes of name are fine as well,
>> as long as the first four of them are "GNU\0".
> 
> No idea, but if this is true then we've got bigger problems with parsing
> the notes.

Okay, I've gone and checked the spec
(http://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section)
and to me it is not entirely unambiguous but matches up better
with the behavior that you want to establish than the more
relaxed one I was suggesting. IOW
Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit I'd like to note that we're still not in line with the doc above
as far as padding is concerned. But there was a lengthy discussion
on the gABI mailing list not so long ago, because the spelled out
behavior also is not in line with what binutils does, nor with what
older gABI versions did mandate.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel