| Message ID | 20160622171545.5304-10-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> 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
On Fri, Jun 24, 2016 at 01:44:30AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: > > + 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. I'm not sure how I could handle that, here. > Also, thinking about it again, the use of UINT_MAX for bounding > pointers is unfortunate: I think this would better be UINTPTR_MAX. Well, I do cast every addr to uint32_t, and I had to define UINT_MAX in the previous patch (hvmloader: Locate the BIOS blob)(I should probably add a comment about it in the patch description). I could try to add UINTPTR_MAX instead.
>>> On 24.06.16 at 19:14, <anthony.perard@citrix.com> wrote: > On Fri, Jun 24, 2016 at 01:44:30AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: >> > + 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. > > I'm not sure how I could handle that, here. Either determine the length of the string first, or have a special variant of strcmp() which returns false when the address would wrap without having reached the end of the string. But again - maybe all this goes too far, and we should instead opt for the simpler route (and easier to grok code) assuming that no item would ever cross the 4Gb boundary. I'm sure making this a requirement even for PVH (which might not actually have anything right below 4Gb, especially when there is no ACPI enabled, and hence no tables to be put anywhere) wouldn't pose a severe limitation to the party setting up the domain: After all such code has to be prepared for stuff to need placement below 4Gb (apart from ACPI tables this could also be PCI device MMIO BAR assignment ranges) anyway. >> Also, thinking about it again, the use of UINT_MAX for bounding >> pointers is unfortunate: I think this would better be UINTPTR_MAX. > > Well, I do cast every addr to uint32_t, and I had to define UINT_MAX in > the previous patch (hvmloader: Locate the BIOS blob)(I should probably > add a comment about it in the patch description). > > I could try to add UINTPTR_MAX instead. That would seem more natural, thanks. And in fact I question the use of uint32_t (instead of unsigned long or even better uintptr_t) for such casts and variable types. Jan
diff --git a/tools/firmware/hvmloader/tests.c b/tools/firmware/hvmloader/tests.c index fea3ad3..bf3aa01 100644 --- 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) + return check_overlap(start, size, + 4ul << 20, + (PT_START + 4 * PAGE_SIZE) - (4ul << 20)); + return false; +} + void perform_tests(void) { int i, passed, skipped; @@ -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)) ) + { + 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; + 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)) ) + { + printf("Skipping tests due to memory used by" + " module[%d]'s cmdline\n", i); + return; + } + } + if ( hvm_start_info->cmdline_paddr && + hvm_start_info->cmdline_paddr < UINT_MAX && + check_test_overlap(hvm_start_info->cmdline_paddr, + strlen((char*)(uint32_t)hvm_start_info->cmdline_paddr)) ) + { + printf("Skipping tests due to memory used by the hvm_start_info->cmdline\n"); + return; + } + passed = skipped = 0; for ( i = 0; tests[i].test; i++ ) {
As perform_tests() is going to clear memory past 4MB, we check that the memory can be use or we skip the tests. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Changes in V5: - also account for the pages table - fix coding style - also check modules cmdline and main cmdline and modlist_paddr - make use of check_overlap. Changes in v4: - move the check into the perform_test() function. - skip tests instead of using BUG. New in V3 --- tools/firmware/hvmloader/tests.c | 53 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)