| Message ID | 1481298289-13546-4-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote: > --- a/xen/common/libelf/libelf-dominfo.c > +++ b/xen/common/libelf/libelf-dominfo.c > @@ -43,10 +43,13 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf, > if ( features == NULL ) > return 0; > > - for ( pos = 0; features[pos] != '\0'; pos += len ) > + for ( pos = 0; > + elf_iter_ok_counted(elf, sizeof(feature)) && > + features[pos] != '\0'; The two sides of the && want to align with one another. > @@ -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. Perhaps that's not a big deal, but wouldn't it be more reasonable to simply account the whole section's size just once? > @@ -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. Jan
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@eu.citrix.com> 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.
>>> On 12.12.16 at 16:38, <ian.jackson@eu.citrix.com> 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
Jan Beulich writes ("Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop"):
> On 12.12.16 at 16:38, <ian.jackson@eu.citrix.com> wrote:
> > 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?
The purpose of the accounting is to prevent the image from causing us
to do lots of work. The work calculation should be based on the
actual algorithm, not on some hypothetical other algorithm that might
be more efficient.
Otherwise if our algorithm is inefficient in some surprising way, when
faced with certain unusual images, that would be a DOS vulnerability.
I think it is easier to write these checks, in terms of the actual
work done, than attempt to construct a proof that the algorithm always
only does a reasonable amount of work.
Ian.
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index 7f4a6a0..b139e32 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -43,10 +43,13 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf, if ( features == NULL ) return 0; - for ( pos = 0; features[pos] != '\0'; pos += len ) + for ( pos = 0; + elf_iter_ok_counted(elf, sizeof(feature)) && + features[pos] != '\0'; + pos += len ) { elf_memset_unchecked(feature, 0, sizeof(feature)); - for ( len = 0;; len++ ) + for ( len = 0;; len++ ) /* can't do more than sizeof(feature) */ { if ( len >= sizeof(feature)-1 ) break; @@ -60,7 +63,7 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf, feature[len] = features[pos + len]; } - for ( i = 0; i < elf_xen_features; i++ ) + for ( i = 0; elf_iter_ok(elf) && i < elf_xen_features; i++ ) { if ( !elf_xen_feature_names[i] ) continue; @@ -236,7 +239,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf, parms->elf_note_start = start; parms->elf_note_end = end; for ( note = ELF_MAKE_HANDLE(elf_note, parms->elf_note_start); - ELF_HANDLE_PTRVAL(note) < parms->elf_note_end; + elf_iter_ok(elf) && ELF_HANDLE_PTRVAL(note) < parms->elf_note_end; note = elf_note_next(elf, note) ) { #ifdef __XEN__ @@ -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) ) { elf_memset_unchecked(name, 0, sizeof(name)); elf_memset_unchecked(value, 0, sizeof(value)); - for ( len = 0;; len++, h++ ) + for ( len = 0;; len++, h++ ) /* covered by iter_ok_counted above */ { if ( len >= sizeof(name)-1 ) break; @@ -291,7 +295,7 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, if ( STAR(h) == '=' ) { h++; - for ( len = 0;; len++, h++ ) + for ( len = 0;; len++, h++ ) /* covered by iter_ok_counted */ { if ( len >= sizeof(value)-1 ) break; @@ -504,7 +508,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, /* Find and parse elf notes. */ count = elf_phdr_count(elf); - for ( i = 0; i < count; i++ ) + for ( i = 0; elf_iter_ok(elf) && i < count; i++ ) { phdr = elf_phdr_by_index(elf, i); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) ) @@ -537,7 +541,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, if ( xen_elfnotes == 0 ) { count = elf_shdr_count(elf); - for ( i = 1; i < count; i++ ) + for ( i = 1; elf_iter_ok(elf) && i < count; i++ ) { shdr = elf_shdr_by_index(elf, i); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 00479af..68c9021 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -85,7 +85,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t /* Find symbol table and symbol string table. */ count = elf_shdr_count(elf); - for ( i = 1; i < count; i++ ) + for ( i = 1; elf_iter_ok(elf) && i < count; i++ ) { shdr = elf_shdr_by_index(elf, i); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) @@ -425,7 +425,7 @@ do { \ * NB: this _must_ be done one by one, and taking the bitness into account, * so that the guest can treat this as an array of type Elf{32/64}_Shdr. */ - for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) + for ( i = 0; elf_iter_ok(elf) && i < ELF_BSDSYM_SECTIONS; i++ ) { rc = elf_load_image(elf, header_base + header_size + shdr_size * i, ELF_REALPTR2PTRVAL(&header.elf_header.section[i]), @@ -453,7 +453,7 @@ void elf_parse_binary(struct elf_binary *elf) unsigned i, count; count = elf_phdr_count(elf); - for ( i = 0; i < count; i++ ) + for ( i = 0; elf_iter_ok(elf) && i < count; i++ ) { phdr = elf_phdr_by_index(elf, i); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) ) @@ -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++ ) { phdr = elf_phdr_by_index(elf, i); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) ) diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index a9edb6a..56dab63 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n ELF_HANDLE_DECL(elf_shdr) shdr; const char *sname; - for ( i = 1; i < count; i++ ) + for ( i = 1; elf_iter_ok(elf) && i < count; i++ ) { shdr = elf_shdr_by_index(elf, i); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) @@ -214,7 +214,7 @@ const char *elf_strval(struct elf_binary *elf, elf_ptrval start) if ( !elf_access_unsigned(elf, start, length, 1) ) /* ok */ return ELF_UNSAFE_PTR(start); - if ( length >= ELF_MAX_STRING_LENGTH ) + if ( !elf_iter_ok(elf) || length >= ELF_MAX_STRING_LENGTH ) { elf_mark_broken(elf, "excessively long string"); return NULL; @@ -262,7 +262,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym uint64_t info, name; const char *sym_name; - for ( ; ptr < end; ptr += elf_size(elf, sym) ) + for ( ; elf_iter_ok(elf) && ptr < end; ptr += elf_size(elf, sym) ) { sym = ELF_MAKE_HANDLE(elf_sym, ptr); info = elf_uval(elf, sym, st_info);
In every `for' or `while' loop, either call elf_iter_ok, or explain why it's not necessary. This is part of comprehensive defence against out of control loops. Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> --- xen/common/libelf/libelf-dominfo.c | 22 +++++++++++++--------- xen/common/libelf/libelf-loader.c | 8 ++++---- xen/common/libelf/libelf-tools.c | 6 +++--- 3 files changed, 20 insertions(+), 16 deletions(-)