[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: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 6 Jul 2021 14:03:39 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=ErCUMiYkR610eB/k1dZYvQnlEViIfNkWDjKB0iL0r74=; b=HTvD6rO3LxwToThQKUdki5IIcTQTx+o/OeNd9e30pms8/iW5mpc2em3mcIQd6J7r/t1LV/vM4/Ka8KCB0qhKjpm4iJ20GfRDf19CrZYr1dcGOgeGTbdVsPrYk/6P4aXVxEKq0NfxVKWIfyWpmB1wNahIXzOK9Jr6nqzwSRKdc0p+vTMR77FcHS6+kBeKiHMY8JRAbNqiTYZXsP3ivsDUbZfRPQRlBYPFQfUZweJbTsQW0ca+qh+9LduECDYvjUXpcgh3XoB+U6Q+gePecGLZtX49QyLhvczDvK1mrQARoRt26NTQSIqS2orOZk8KZn/lz+H+JQ3oezpzNbLOJTalUg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VVslLwld4osopHA2VA86Tt407dsabmBHxrpIl1VHNp95O7iohWhw3vAJ0NBTGmLj0lGszad0z0tVhiGBylWykl09jDBtzQs5lFTf0BQOqzLb9XVQOu2554wiRlkwjdmG8Blfa9H/Q185E5tqaYuQgGfrxnwOz7IBbkZ58Wt2yZTg5hXAQkcE1G7eezH6kZmi0bT62JiGrKJpemBWYHuBXLc/ZPvco0nSnL/POoxg0Q93FvjGhmbZAJzsoot+TlsbZcsB85jO0itTNZikSlNViX5IHtxIdNh+8woxyBFMPX9NxNkJTtfqNtvSwTb0kVCS48BExN/DoXc7ZkK6KfDB/g==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.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 12:03:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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 unaware of there being any spelled out guarantee
that the numbers reported back from the hypercall are accurate.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.