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. > > 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. Frankly I'm not sure what the point of a01b6d46 was ? > > I think it would be better to do those checks such that > > overflow results in an error return. The design principle behind my fixes to XSA-55 is to write the bulk of libelf in a subset of C which is always safe. This means: * Always using unsigned integers (so no signed integer overflow). * Doing all pointer dereferences into the ELF block using an access function which does a bounds check. (And this also means representing all pointers as unsigned offsets, so that we avoid calculating basilisk pointer values.) * Explicitly limiting the iteration count of every loop where necessayr (to avoid DOS attacks from bad images). * Not using unsafe operations like division by input-controlled values. Sticking to these rules is a lot easier than writing explicit checks. This is because these explicit checks are very easy to omit, or get wrong. The pointers are all encapsulated in a special type which prevents uncontrolled dereference. Admittedly the rule about iteration count is not so easy to remember, and I had failed to anticipate that someone would introduce division. But both of those kind of bugs are denial of service, rather than privilege escalation. Having stared at the commit message of a01b6d46 and at the pre-a01b6d46 code, I still don't understand what motivated the changes. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel