vfio: Fix CID 1458134 in vfio_register_ram_discard_listener()

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

Posted by David Hildenbrand 4 years, 7 months ago

  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>
---
 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,
-- 
2.31.1

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

Posted by Igor Mammedov 4 years, 7 months ago

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,

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

Posted by Pankaj Gupta 4 years, 7 months ago

>   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>
> ---
>  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,

> Fixes: d5015b801340 ("softmmu/memory: Pass ram_flags to
> qemu_ram_alloc_from_fd()")
>
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/remote/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/remote/memory.c b/hw/remote/memory.c
> index 472ed2a272..6e21ab1a45 100644
> --- a/hw/remote/memory.c
> +++ b/hw/remote/memory.c
> @@ -46,7 +46,7 @@ void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
>          subregion = g_new(MemoryRegion, 1);
>          memory_region_init_ram_from_fd(subregion, NULL,
>                                         name, sysmem_info->sizes[region],
> -                                       true, msg->fds[region],
> +                                       RAM_SHARED, msg->fds[region],
>                                         sysmem_info->offsets[region],
>                                         errp);
>

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>