Re: [Xen-devel] [PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.12.16 at 15:46, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/12/16 14:41, Jan Beulich wrote:
>>>>> On 08.12.16 at 15:18, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> elf_uval() can return zero either because the field itself is zero, or 
> because
>>> the access is out of bounds.
>>>
>>> c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 
> issues
>>> as e_{ph,sh}entsize are not checked for sanity before being used to divide
>>> elf->size.
>>>
>>> Spotted by Coverity.
>> And wrongly so, imo.
>>
>>> --- a/xen/common/libelf/libelf-tools.c
>>> +++ b/xen/common/libelf/libelf-tools.c
>>> @@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, 
>>> uint64_t 
> addr)
>>>  unsigned elf_shdr_count(struct elf_binary *elf)
>>>  {
>>>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
>>>      uint64_t max;
>>>  
>>>      if ( !count )
>>>          return 0;
>>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
>>> +    if ( !entsize )
>>> +    {
>>> +        elf_mark_broken(elf, "e_shentsize is zero");
>>> +        return 0;
>>> +    }
>> This as well as ...
>>
>>> @@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
>>>  unsigned elf_phdr_count(struct elf_binary *elf)
>>>  {
>>>      unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
>>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
>>>      uint64_t max;
>>>  
>>>      if ( !count )
>>>          return 0;
>>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
>>> +    if ( !entsize )
>>> +    {
>>> +        elf_mark_broken(elf, "e_phentsize is zero");
>>> +        return 0;
>>> +    }
>> ... this would end up being dead code, due to the checks the same
>> patch you refer to introduced in elf_init().
> 
> Are you sure?  elf_init() currently looks like this:
> 
>     /* Sanity check phdr if present. */
>     count = elf_phdr_count(elf);
>     if ( count )
>     {
>         if ( elf_uval(elf, elf->ehdr, e_phentsize) <
>              elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
>         {
>             elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
>                     elf_uval(elf, elf->ehdr, e_phentsize));
>             return -1;
>         }
> 
> There is no check of e_phentsize before it is used to divide with.

Oh, I see - elf_phdr_count() itself relies on the check that's
about to be done. But I think the correct thing then would be to
not use elf_phdr_count() here, but read the raw field instead.
You patch basically adds a weaker check there which is then
immediately being followed by the stronger check above.

And looking at it from that angle it's then questionable why
elf_{p,s}hdr_count() do any checking at all - this should all be
done once in elf_init(). IOW I did the adjustment in the wrong
way, and I really should have made elf_shdr_count() match
elf_phdr_count() (moving the checks into elf_init()).

And looking at elf_init() again I also realize that I blindly
copied the range checks, calculation of which can overflow.
I think it would be better to do those checks such that
overflow results in an error return.

I wonder if it wouldn't be better to revert that change, for
me to produce a better v2.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel