[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] tools/migration: Fix potential overflow in send_checkpoint_dirty_pfn_list()
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
- Date: Tue, 6 Jul 2021 14:34:45 +0100
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=s+V3zJu17mw4cxKdyPeOkHXZe+pHkyctt/iMfwee90I=; b=jayufHs9Wp7GeR+4WxDN6CiFSubUPpdD5tweJxphQJLVDUQKL/UJy/8P2uJcY9az0FVUmgpsC24Z6YGIAmeE6kLj3LUb/lq7ly5wqkjLQWptLgqEjPyl32okJdOz+ssEeRrpJCK+35ZLm+8HT6fCvq30aNJpbIYWqhSh9LpPrwB/YQjseuvrAtMwDwpv78RP9daipxRzaG5iBcFjm41yxvHJNpzD9FXM+889ruE0SZ4lM+J55pTWGueYg/QHvjiD4JIsGHHSTpJcZ1+d4rzQeCWhVTP7wEl/YeB0K6oY5PshJPZbY3lLUPbvoZZaDM4lklUBsHEdoTmhc8C9IAelWg==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fDx3V0OQVDuZ50smOxfa5R5kJaQ67FEJNCW6aEqhb5YsC04RkrFbx7f9qVIq+3tCQs/Q8sm/sECD5re6+FIZIi2OtpJ29F3bmB+uxIIp5eG44164OG4lWxJd/FzC8iV1n0aSG67KL4YLi3VgcgiNenrx3UkEeXAY/dNhluDAXsYuOJgrajaTpVYwaPL/PfXLI2P/nDoT6z16YV0+JR/aTlhOuHySbBqDsVx+Sp78EpxRZHdiqzDpHMPk1wIKxmQAeypl5j/7pSZGBOjDajAksnBX0mqS7rdDwqym3DrINaeA/Wh8MyN3EAT4JNTb1LcAiWsAoPlcd3O1EzeoN0eZew==
- Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
- Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Olaf Hering <olaf@xxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
- Delivery-date: Tue, 06 Jul 2021 13:35:25 +0000
- Ironport-hdrordr: A9a23:64bqpKHRnhoyQ1+HpLqFHZHXdLJyesId70hD6qkvc3Nom52j+/ xGws536faVslcssHFJo6HkBEDyewKiyXcT2/hsAV7CZniahILMFu9fBOTZskXd8kHFh4lgPO JbAtJD4b7LfChHZKTBkXCF+r8bqbHtmsDY5pat854ud3APV0gJ1XYJNu/xKDwReOApP+taKH PR3Ls9m9L2Ek5nEPhTS0N1EtTrlpnurtbLcBQGDxko5E2nii6p0qfzF1y90g0FWz1C7L8++S yd+jaJqZmLgrWe8FvxxmXT55NZlJ/IzcZCPtWFjowwJi/3ggilSYx9U/mpvSwzosuo9FE2+e O87ysIDoBW0Tf8b2u1qRzi103LyzA18ULvzleenD/KvdH5bChSMbsAuatpNj/ir2YwttB116 xGm0iDsYBMMB/GlCPho/DVShBRkFauq3ZKq59Ts5Vma/pdVFZtl/1bwKsMe61wWB4SqbpXXt WGNfusp8q/KjihHjfkVgAF+q3eYpwxdi32CnTq9PbllQS+p0oJu3fw8vZv10voxKhNPqWs2N 60RZiAtIs+BfP+PpgNTtvof6OMexrwqFT3QTuvHWg=
- Ironport-sdr: i1rbDAP62lEWqbKwv5cCm2jtkPfQvq/GyrkwSmcYBY0NAtqzUMoqS2HCDd+Hu0jnyJoJV0l7S4 x3LmqMaAzK33O1Wc3xeVMOsRs1YZ+B29743w1lripUKiA5chjZlYnY4mUoUlDHKHMVMT9jqEIb oW9Ka/Zwwnk0QblMFq3ecX2yXP5VqklxR2cXkhFVyLS9wTCdC/Rlx4MvzLmwIov8+1+WXBLjz3 L4UZP+M/0wktknibWcgKCn9iUQdEaj1DASdIUhJ/taHQD7T0JQ7/MOvfbHWi1XrJCy0bDNRMv+ 0TI=
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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
|