Re: [PATCH] x86/hvm: Clean up track_dirty_vram() calltree

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/07/2020 17:13, Jan Beulich wrote:
> On 22.07.2020 17:15, Andrew Cooper wrote:
>>  * Rename nr to nr_frames.  A plain 'nr' is confusing to follow in the the
>>    lower levels.
>>  * Use DIV_ROUND_UP() rather than opencoding it in several different ways
>>  * The hypercall input is capped at uint32_t, so there is no need for
>>    nr_frames to be unsigned long in the lower levels.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> I'd like to note though that ...
>
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -58,16 +58,16 @@
>>  
>>  int hap_track_dirty_vram(struct domain *d,
>>                           unsigned long begin_pfn,
>> -                         unsigned long nr,
>> +                         unsigned int nr_frames,
>>                           XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
>>  {
>>      long rc = 0;
>>      struct sh_dirty_vram *dirty_vram;
>>      uint8_t *dirty_bitmap = NULL;
>>  
>> -    if ( nr )
>> +    if ( nr_frames )
>>      {
>> -        int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
>> +        unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
> ... with the change from long to int this construct will now no
> longer be correct for the (admittedly absurd) case of a hypercall
> input in the range of [0xfffffff9,0xffffffff]. We now fully
> depend on this getting properly rejected at the top level hypercall
> handler (which limits to 1Gb worth of tracked space).

I don't see how this makes any difference at all.

Exactly the same would be true in the old code for an input in the range
[0xfffffffffffffff9,0xffffffffffffffff], where the aspect which protects
you is the fact that the hypercall ABI truncates to 32 bits.

If you want a non-overflowing DIV_ROUND_UP(), the appropriate expression
in (x / a) + !!(x % a), but I don't think its reasonable to use the type
of this variable as a credible defence-in-depth argument against the
audit logic making a mistake, or that it is worth worrying about audit
mistakes in the first place.  If there are any audit mistakes, so much
more can potentially go wrong than this corner case.

~Andrew