[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |