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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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