[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |