Re: [Xen-devel] [PATCH 8/8] libelf: safety: Document safety principles in header file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.12.16 at 12:43, <ian.jackson@xxxxxxxxxxxxx> wrote:
> Jan Beulich writes ("Re: [PATCH 8/8] libelf: safety: Document safety 
> principles in header file"):
>> On 09.12.16 at 16:44, <ian.jackson@xxxxxxxxxxxxx> wrote:
>> > + *  - Stack local buffer variables containing information derived from
>> > + *    the image (including structs, or byte buffers) must be
>> > + *    completely zeroed, using elf_memset_unchecked (and an
>> > + *    accompanying elf_iter_ok_counted) on entry to the function (or
>> > + *    somewhere very obviously near there).
>> > + *
>> > + *        Rationale: This avoids uninitialised stack data being used
>> > + *        as input to any of the loader.
>> 
>> What distinguishes a "buffer variable" from a plain variable? I.e.
>> where is the boundary here as to which ones need zeroing?
> 
> The hazard is an undiscovered uninitialised variable.  "Undiscovered"
> meaning static analysis tools won't find it (eg Coverity will find
> many if not most normal uninitialised variables).
> 
> This is the biggest risk for variables which can be partially
> initialised: ie, structs and arrays.  It is more difficult to reason
> about variables which can be partially initialised.

I guess you meant "can't"? In any event the wording may want
some clarification then.

>> Also,
>> I don't think zeroing is needed when a variable gets fully written
>> over (like in a structure assignment). Do you perhaps want to
>> limit the above to "directly derived from the image"?
> 
> Structure assignment does not necessarily write over the whole
> variable.  It may leave the padding un-overwritten.

Oh, true.

>> > + *  - Do not add code which is not necessary for libelf to function
>> > + *    with correct input, or to avoid being vulnerable to incorrect
>> > + *    input.  Do not add additional functionally-unnecessary checks
>> > + *    for diagnosing problems with the image, or validating sanity of
>> > + *    the input ELF.
> ...
>> While I can understand your reasoning, I continue to think that
>> libelf in a ELF loader, and hence should check certain things. The
>> best example I currently know of are e_[ps]hentsize, which the
>> code uses without checking the lower valid bound.
> 
> You don't give a counterargument to my reasoning:
> 
>> > + *        Rationale: Redundant checks have 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, because as said - I can understand it, given the perspective
you take.

Jan


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