Re: [Xen-devel] [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Thanks for your careful review.

Jan Beulich writes ("Re: [PATCH 3/8] libelf: loop safety: Call 
elf_iter_ok[_counted] in every loop"):
> On 09.12.16 at 16:44, <ian.jackson@xxxxxxxxxxxxx> wrote:
> > +    for ( pos = 0;
> > +          elf_iter_ok_counted(elf, sizeof(feature)) &&
> > +              features[pos] != '\0';
> 
> The two sides of the && want to align with one another.

Sure, if you like it that way.

> > @@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct 
> > elf_binary *elf,
> >  
> >      h = parms->guest_info;
> >  #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
> > -    while ( STAR(h) )
> > +    while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
> > +            STAR(h) )
> 
> So you're using libelf determined sizes here rather than image
> properties.

I'm not sure what you mean.  Each iteration of this loop contains some
calls to elf_memset_unchecked.  The rules say:

 *  - 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).

And by elf_*_unchecked:

 * Remember to call elf_iter_ok_counted nearby.

So the calls to elf_memset_unchecked, to zero name and value, imply
that there must be a call to elf_iter_ok_counted.  The count parameter
should be the actual work done.

There are two loops inside this outer loop.  They are textually but
not logically nested.  By the rules each of these loops needs a call
to elf_iter_ok, but: since they iterate over characters for name and
value respectively, they clearly do no more work than the memsets.

> Perhaps that's not a big deal, but wouldn't it be more
> reasonable to simply account the whole section's size just once?

You mean to call elf_iter_ok_counted on the size of the note section ?

But that would be wrong, because in principle almost each character in
the note section could cause a memset of all of name and a memset of
all of value.

Doing the iteration checks at a distance, and based on input image
properties rather than actual code flow, requires the kind of subtle
and complicated analysis I found too fragile.

> > @@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
> >      uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
> >  
> >      count = elf_phdr_count(elf);
> > -    for ( i = 0; i < count; i++ )
> > +    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
> 
> At the example of this, but likely applicable elsewhere - what use is
> this check at this point in the series? Without peeking at later patches
> it is not really clear how adding this makes any difference.
> 
> Overall I'm not entirely opposed to the approach, but I find these
> extra checks rather unpleasant to have.

I think you may need to read the big comment in patch 8.  I introduce
that at the end of the series because it doesn't become true before
then.

If you like I can move it to the front of the series, and introduce it
with a "may not be true" caveat which is later removed.

Ian.

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