Re: [Xen-devel] [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.06.16 at 19:15, <anthony.perard@xxxxxxxxxx> wrote:
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include "util.h"
> +#include "config.h"
>  
>  #define TEST_FAIL 0
>  #define TEST_PASS 1
> @@ -189,6 +190,15 @@ static int shadow_gs_test(void)
>      return (ebx == 2) ? TEST_PASS : TEST_FAIL;
>  }
>  
> +static bool check_test_overlap(uint64_t start, uint64_t size)
> +{
> +    if (start)

Missing blanks.

> +        return check_overlap(start, size,
> +                             4ul << 20,
> +                             (PT_START + 4 * PAGE_SIZE) - (4ul << 20));

Can the 4ul << 20 and the upper bound be made into descriptive
#define-s please, also to be used wherever (if anywhere) those
boundary are currently of relevance (but at least side by side with
the respective comment at the top of the file)?

> @@ -210,6 +220,49 @@ void perform_tests(void)
>          return;
>      }
>  
> +    /* Check that tests does not use memory where modules are stored */
> +    if ( check_test_overlap((uint32_t)hvm_start_info, 
> sizeof(hvm_start_info)) )

sizeof(*hvm_start_info) perhaps?

> +    {
> +        printf("Skipping tests due to memory used by hvm_start_info\n");
> +        return;
> +    }
> +    if ( check_test_overlap(hvm_start_info->modlist_paddr,
> +                            hvm_start_info->nr_modules *
> +                              sizeof(struct hvm_modlist_entry)) )
> +    {
> +        printf("Skipping tests due to memory used by"
> +               " hvm_start_info->modlist\n");
> +        return;
> +    }
> +    for ( i = 0; i < hvm_start_info->nr_modules; i++ )
> +    {
> +        const struct hvm_modlist_entry *modlist =
> +            (struct hvm_modlist_entry 
> *)(uint32_t)hvm_start_info->modlist_paddr;

To cut done on the length of such lines, perhaps better to use void *
in casts like this?

> +        uint64_t cmdline_paddr = modlist[i].cmdline_paddr;
> +
> +        if ( check_test_overlap(modlist[i].paddr, modlist[i].size) )
> +        {
> +            printf("Skipping tests due to memory used by module[%d]\n", i);
> +            return;
> +        }
> +        if ( cmdline_paddr && cmdline_paddr < UINT_MAX &&
> +             check_test_overlap(cmdline_paddr,
> +                                strlen((char*)(uint32_t)cmdline_paddr)) )

As said on the previous patch - cmdline_paddr being below 4Gb
doesn't necessarily mean the string not crossing that boundary.
Depending on the resolution for that other patch this may need
adjustment.

Also, thinking about it again, the use of UINT_MAX for bounding
pointers is unfortunate: I think this would better be UINTPTR_MAX.

Jan


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