Re: [PATCH 2/2] tools/migration: Fix potential overflow in send_checkpoint_dirty_pfn_list()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/07/2021 13:03, Jan Beulich wrote:
> On 06.07.2021 13:23, Andrew Cooper wrote:
>> 'count * sizeof(*pfns)' can in principle overflow, but is implausible in
>> practice as the time between checkpoints is typically sub-second.
>> Nevertheless, simplify the code and remove the risk.
>>
>> There is no need to loop over the bitmap to calculate count.  The number of
>> set bits is returned in xc_shadow_op_stats_t which is already collected (and
>> ignored).
>>
>> Bounds check the count against what will fit in REC_LENGTH_MAX.  At the time
>> of writing, this allows up to 0xffffff pfns.
> Well, okay, this then means that an overflow in the reporting of
> dirty_count doesn't matter for now, because the limit is lower
> anyway.
>
>>  Rearrange the pfns loop to check
>> for errors both ways, not simply that there were more pfns than expected.
> Hmm, "both ways" to me would mean ...
>
>> @@ -459,24 +462,20 @@ static int send_checkpoint_dirty_pfn_list(struct xc_sr_context *ctx)
>>          goto err;
>>      }
>>  
>> -    for ( i = 0, written = 0; i < ctx->restore.p2m_size; ++i )
>> +    for ( i = 0, written = 0; count && i < ctx->restore.p2m_size; ++i, --count )
>>      {
>>          if ( !test_bit(i, dirty_bitmap) )
>>              continue;
>>  
>> -        if ( written > count )
>> -        {
>> -            ERROR("Dirty pfn list exceed");
>> -            goto err;
>> -        }
>> -
>>          pfns[written++] = i;
>>      }
>>  
>> -    rec.length = count * sizeof(*pfns);
>> -
>> -    iov[1].iov_base = pfns;
>> -    iov[1].iov_len = rec.length;
>> +    if ( written != stats.dirty_count )
>> +    {
>> +        ERROR("Mismatch between dirty bitmap bits (%u), and dirty_count (%u)",
>> +              written, stats.dirty_count);
>> +        goto err;
>> +    }
> ... you then also check that there are no further bit set in the
> bitmap. As said elsewhere, I'm not convinced using statistics as
> a basis for actual operation (rather than just reporting) is
> appropriate.

I'm not interested in inference based on the name of the structure.

>  I'm unaware of there being any spelled out guarantee
> that the numbers reported back from the hypercall are accurate.

The live loop uses this information already for this purpose.  If it is
wrong, we've got bigger problems that this.

~Andrew