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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.12.16 at 16:47, <ian.jackson@xxxxxxxxxxxxx> wrote:
> Andrew Cooper writes ("Re: [PATCH] libelf: Fix div0 issues in 
> elf_{shdr,phdr}_count()"):
>> On 08/12/16 15:17, Jan Beulich wrote:
>> > 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.
> 
> There must be no "reading of raw fields".  You may use
> elf_access_unsigned if you really want to.

Oh, by "raw" I meant using elf_uval() rather than elf_phdr_count().

>> > 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()).
> 
> The point of this checking is not to avoid overflow, but just to
> ensure that the loops which rely on elf_*_count do not iterate "far
> too much".
> 
>> > And looking at elf_init() again I also realize that I blindly
>> > copied the range checks, calculation of which can overflow.
> 
> These possibly-faulty range checks are harmless because they all
> operate on unsigned values.

Why does operating on unsigned values make overflow harmless?

> Frankly I'm not sure what the point of a01b6d46 was ?

First of all I'd like to refer you to its description, but see below.
Are you suggesting that all those adjustments appear pointless
to you?

>  * Not using unsafe operations like division by input-controlled
>    values.

But using a hard coded but possibly wrong value instead isn't
really a solution.

> Having stared at the commit message of a01b6d46 and at the
> pre-a01b6d46 code, I still don't understand what motivated the
> changes.

Hmm, that's interesting. Are you then saying all the changes it
did and described are wrong? Pointless? I could do with some
clarification here, as otherwise it's not clear whether spending
time on producing an improved v2 is actually worth it.

Jan


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