Re: [PATCH v1] vfio: Fix CID 1458134 in vfio_register_ram_discard_listener()

From: Igor Mammedov <imammedo@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, Peter Xu <peterx@redhat.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	Auger Eric <eric.auger@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	teawater <teawaterz@linux.alibaba.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Marek Kedzierski <mkedzier@redhat.com>,
	Wei Yang <richard.weiyang@linux.alibaba.com>
Subject: Re: [PATCH v1] vfio: Fix CID 1458134 in vfio_register_ram_discard_listener()
Date: Mon, 12 Jul 2021 11:05:48 +0200	[thread overview]
Message-ID: <20210712110548.02f3bc18@redhat.com> (raw)
In-Reply-To: <20210712083135.15755-1-david@redhat.com>

On Mon, 12 Jul 2021 10:31:35 +0200
David Hildenbrand <david@redhat.com> wrote:

>   CID 1458134:  Integer handling issues  (BAD_SHIFT)
>     In expression "1 << ctz64(container->pgsizes)", left shifting by more
>     than 31 bits has undefined behavior.  The shift amount,
>     "ctz64(container->pgsizes)", is 64.
> 
> Commit 5e3b981c330c ("vfio: Support for RamDiscardManager in the !vIOMMU
> case") added an assertion that our granularity is at least as big as the
> page size.
> 
> Although unlikely, we could have a page size that does not fit into
> 32 bit. In that case, we'd try shifting by more than 31 bit.
> 
> Let's use 1ULL instead and make sure we're not shifting by more than 63
> bit by asserting that any bit in container->pgsizes is set.
> 
> Fixes: CID 1458134
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Auger Eric <eric.auger@redhat.com>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: teawater <teawaterz@linux.alibaba.com>
> Cc: Marek Kedzierski <mkedzier@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/vfio/common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 3f0d111360..8728d4d5c2 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -783,7 +783,8 @@ static void vfio_register_ram_discard_listener(VFIOContainer *container,
>                                                                  section->mr);
>  
>      g_assert(vrdl->granularity && is_power_of_2(vrdl->granularity));
> -    g_assert(vrdl->granularity >= 1 << ctz64(container->pgsizes));
> +    g_assert(container->pgsizes &&
> +             vrdl->granularity >= 1ULL << ctz64(container->pgsizes));
>  
>      ram_discard_listener_init(&vrdl->listener,
>                                vfio_ram_discard_notify_populate,