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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.16 at 12:54, <ian.jackson@xxxxxxxxxxxxx> wrote:
> Jan Beulich writes ("Re: [PATCH] libelf: Fix div0 issues in 
> elf_{shdr,phdr}_count()"):
>> On 08.12.16 at 18:28, <ian.jackson@xxxxxxxxxxxxx> wrote:
>> > Looking at the commit message I was concerned that the phdr and shdr
>> > counts might be excessive, and that libelf might get stuck in a loop
>> > with an unreasonably high iteration count.
>> 
>> Looking at that commit message, what I'm missing first of all is an
>> explanation of why long loops are a problem in the first place. Do
>> we need to be concerned of single threaded tool stacks? I think
>> such a problem would better be addressed at higher layers,
>> switching to multi threaded designs.
> 
> libxl is (essentially) singlethreaded.  It is very hard to write
> correct multithreaded programs and I would be adamantly opposed to
> anything that made libxl more thready inside.  Also, even with
> threading, a long-running domain setup would make it difficult to kill
> the domain, unless we have asynchronous termination of such a thread,
> which would be a nightmare to implement correctly.

But libxl is only a library - the question is whether xl (or whatever
containing process) may be sitting in on libelf invocation,
preventing the process(es) controlling another domain(s) from
making forward progress.

As to killing the domain - wouldn't killing the xl process doing the
creation followed by an "xl destroy" be sufficient? (As you know,
I don't know very much about how the tool stack works, but this
would seem a pretty natural pair of operations.)

>> > I now see that (a) is ineffective because the image can specify its
>> > own stride for the iteration (the header size).  If it specifies a
>> > stride of zero, the maximum count is unbounded.
>> 
>> Well, a stride of zero is invalid (as is any smaller than the base ELF
>> spec's entry structure size).
> 
> That something is invalid does not mean it cannot be specified.

And I didn't mean to imply that. What I mean to imply is that if
we guarded against too small (and hence invalid) entry size values
(which is one of the things the reverted patch does), we wouldn't
have this problem.

>> For this last point - "ELF validator" is certainly the goal. But I
>> think it is reasonable for a library to at least check the input it
>> makes use of. If I had meant to fully validate the ELF image,
>> I would e.g. have had to reject images with zero e_phnum but
>> non-zero other e_ph* fields. Yet we don't care about those
>> other fields when there are no program headers (which, as
>> pointed out on IRC, is useless anyway, as then there's nothing
>> to load; only e_shnum can sensibly be zero).
> 
> (I think you omitted "not", so you meant "is certainly not the goal.)

Indeed, thanks for noticing and deducing the right thing.

> IMO libelf should do enough tests to function correctly with correct
> input, and avoid being vulnerable to incorrect input.  Code to perform
> other tests introduces risk (of bugs, including security bugs such as
> the one introduced in a01b6d4).
> 
> Such code has almost zero benefit because 1. we do not expect
> ELF-generating tools to generate invalid ELFs so these checks' failure
> paths will very likely never be executed anywhere, and 2. anyone
> debugging such a hypothetical bad ELF will have a variety of tools
> available which will do a much better job of analysing it.

Well, in that case I'll have to teach myself to keep my hands off of
libelf/ as much as possible. But as you can see from the entry size
aspect further up, some extra checks may help (and might even
allow to weaken your "don't allow division by ..." criteria).

Jan


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