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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.06.16 at 19:14, <anthony.perard@xxxxxxxxxx> wrote:
> On Fri, Jun 24, 2016 at 01:44:30AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 19:15, <anthony.perard@xxxxxxxxxx> 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


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