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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/12/16 15:17, Jan Beulich wrote:
>>>> 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.

Feel free.

I have to admit that I find it odd that the values aren't sanitised
once, then copied out into local good state.  The repeated use of
elf_uval() risks a lot of zeroes because of its range check case.

~Andrew

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