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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.12.16 at 16:38, <ian.jackson@xxxxxxxxxxxxx> wrote:
>> > @@ -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.

Hmm, if the rules say that, I'll then have to question the rules:
Shouldn't accounting be based on what the workload the image
causes us, instead of our own overhead?

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

Well, as per above, different ways you and I think this should work
then, as it seems. I can see where you're coming from, but I'm not
convinced this is the right model when taking a client (image)
perspective.

>> > @@ -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.

Yeah, I still need to make it there (and maybe I should have looked
at that one first).

Jan


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