>>> On 08.12.16 at 15:46, <andrew.coop...@citrix.com> wrote:
> On 08/12/16 14:41, Jan Beulich wrote:
>>>>> On 08.12.16 at 15:18, <andrew.coop...@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel
