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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.16 at 16:44, <ian.jackson@xxxxxxxxxxxxx> 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


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