[PATCH qemu] vfio/spapr: Fix page size calculation

[PATCH qemu] vfio/spapr: Fix page size calculation

[Alexey Kardashevskiy]

Coverity detected an issue (CID 1421903) with potential call of clz64(0)
which returns 64 which make it do "<<" with a negative number.

This checks the mask and avoids undefined behaviour.

In practice pgsizes and memory_region_iommu_get_min_page_size() always
have some common page sizes and even if they did not, the resulting page
size would be 0x8000.0000.0000.0000 (gcc 9.2) and
ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/vfio/spapr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 33692fc86fd6..2900bd19417a 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
 {
     int ret = 0;
     IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
-    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
+    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
     unsigned entries, bits_total, bits_per_level, max_levels;
     struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
     long rampagesize = qemu_minrampagesize();
@@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
     if (pagesize > rampagesize) {
         pagesize = rampagesize;
     }
-    pagesize = 1ULL << (63 - clz64(container->pgsizes &
-                                   (pagesize | (pagesize - 1))));
+    pgmask = container->pgsizes & (pagesize | (pagesize - 1));
+    pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
     if (!pagesize) {
         error_report("Host doesn't support page size 0x%"PRIx64
                      ", the supported mask is 0x%lx",
-- 
2.17.1

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[Philippe Mathieu-Daudé]

On 3/24/20 7:39 AM, Alexey Kardashevskiy wrote:
> Coverity detected an issue (CID 1421903) with potential call of clz64(0)
> which returns 64 which make it do "<<" with a negative number.
> 
> This checks the mask and avoids undefined behaviour.
> 
> In practice pgsizes and memory_region_iommu_get_min_page_size() always
> have some common page sizes and even if they did not, the resulting page
> size would be 0x8000.0000.0000.0000 (gcc 9.2) and
> ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hw/vfio/spapr.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 33692fc86fd6..2900bd19417a 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>   {
>       int ret = 0;
>       IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> -    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
> +    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
>       unsigned entries, bits_total, bits_per_level, max_levels;
>       struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
>       long rampagesize = qemu_minrampagesize();
> @@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
>       if (pagesize > rampagesize) {
>           pagesize = rampagesize;
>       }
> -    pagesize = 1ULL << (63 - clz64(container->pgsizes &
> -                                   (pagesize | (pagesize - 1))));
> +    pgmask = container->pgsizes & (pagesize | (pagesize - 1));

Is that ROUND_UP(container->pgsizes, pagesize)?

> +    pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
>       if (!pagesize) {
>           error_report("Host doesn't support page size 0x%"PRIx64
>                        ", the supported mask is 0x%lx",
> 

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[Greg Kurz]

On Tue, 24 Mar 2020 17:39:12 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> Coverity detected an issue (CID 1421903) with potential call of clz64(0)
> which returns 64 which make it do "<<" with a negative number.
> 
> This checks the mask and avoids undefined behaviour.
> 
> In practice pgsizes and memory_region_iommu_get_min_page_size() always
> have some common page sizes and even if they did not, the resulting page
> size would be 0x8000.0000.0000.0000 (gcc 9.2) and
> ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/vfio/spapr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 33692fc86fd6..2900bd19417a 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>  {
>      int ret = 0;
>      IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> -    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
> +    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
>      unsigned entries, bits_total, bits_per_level, max_levels;
>      struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
>      long rampagesize = qemu_minrampagesize();
> @@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
>      if (pagesize > rampagesize) {
>          pagesize = rampagesize;
>      }
> -    pagesize = 1ULL << (63 - clz64(container->pgsizes &
> -                                   (pagesize | (pagesize - 1))));
> +    pgmask = container->pgsizes & (pagesize | (pagesize - 1));
> +    pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
>      if (!pagesize) {
>          error_report("Host doesn't support page size 0x%"PRIx64
>                       ", the supported mask is 0x%lx",

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[Greg Kurz]

On Tue, 24 Mar 2020 14:27:35 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> 
> 
> On 3/24/20 7:39 AM, Alexey Kardashevskiy wrote:
> > Coverity detected an issue (CID 1421903) with potential call of clz64(0)
> > which returns 64 which make it do "<<" with a negative number.
> > 
> > This checks the mask and avoids undefined behaviour.
> > 
> > In practice pgsizes and memory_region_iommu_get_min_page_size() always
> > have some common page sizes and even if they did not, the resulting page
> > size would be 0x8000.0000.0000.0000 (gcc 9.2) and
> > ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> >   hw/vfio/spapr.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> > index 33692fc86fd6..2900bd19417a 100644
> > --- a/hw/vfio/spapr.c
> > +++ b/hw/vfio/spapr.c
> > @@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
> >   {
> >       int ret = 0;
> >       IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> > -    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
> > +    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
> >       unsigned entries, bits_total, bits_per_level, max_levels;
> >       struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
> >       long rampagesize = qemu_minrampagesize();
> > @@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
> >       if (pagesize > rampagesize) {
> >           pagesize = rampagesize;
> >       }
> > -    pagesize = 1ULL << (63 - clz64(container->pgsizes &
> > -                                   (pagesize | (pagesize - 1))));
> > +    pgmask = container->pgsizes & (pagesize | (pagesize - 1));
> 
> Is that ROUND_UP(container->pgsizes, pagesize)?
> 

This means we clip all page sizes greater than pagesize from
container->pgsizes... It doesn't look like ROUND_UP() semantics
to me.

> > +    pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
> >       if (!pagesize) {
> >           error_report("Host doesn't support page size 0x%"PRIx64
> >                        ", the supported mask is 0x%lx",
> > 
> 
> 

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[Philippe Mathieu-Daudé]

On 3/24/20 3:30 PM, Greg Kurz wrote:
> On Tue, 24 Mar 2020 14:27:35 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>>
>>
>> On 3/24/20 7:39 AM, Alexey Kardashevskiy wrote:
>>> Coverity detected an issue (CID 1421903) with potential call of clz64(0)
>>> which returns 64 which make it do "<<" with a negative number.
>>>
>>> This checks the mask and avoids undefined behaviour.
>>>
>>> In practice pgsizes and memory_region_iommu_get_min_page_size() always
>>> have some common page sizes and even if they did not, the resulting page
>>> size would be 0x8000.0000.0000.0000 (gcc 9.2) and
>>> ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>    hw/vfio/spapr.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>>> index 33692fc86fd6..2900bd19417a 100644
>>> --- a/hw/vfio/spapr.c
>>> +++ b/hw/vfio/spapr.c
>>> @@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>>>    {
>>>        int ret = 0;
>>>        IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>>> -    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
>>> +    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
>>>        unsigned entries, bits_total, bits_per_level, max_levels;
>>>        struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
>>>        long rampagesize = qemu_minrampagesize();
>>> @@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
>>>        if (pagesize > rampagesize) {
>>>            pagesize = rampagesize;
>>>        }
>>> -    pagesize = 1ULL << (63 - clz64(container->pgsizes &
>>> -                                   (pagesize | (pagesize - 1))));
>>> +    pgmask = container->pgsizes & (pagesize | (pagesize - 1));
>>
>> Is that ROUND_UP(container->pgsizes, pagesize)?
>>
> 
> This means we clip all page sizes greater than pagesize from
> container->pgsizes... It doesn't look like ROUND_UP() semantics
> to me.

Ah. Extracting it as a new function with meaningful name would help code 
review :)

> 
>>> +    pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
>>>        if (!pagesize) {
>>>            error_report("Host doesn't support page size 0x%"PRIx64
>>>                         ", the supported mask is 0x%lx",
>>>
>>
>>
> 

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

On Tue, Mar 24, 2020 at 05:39:12PM +1100, Alexey Kardashevskiy wrote:
> Coverity detected an issue (CID 1421903) with potential call of clz64(0)
> which returns 64 which make it do "<<" with a negative number.
> 
> This checks the mask and avoids undefined behaviour.
> 
> In practice pgsizes and memory_region_iommu_get_min_page_size() always
> have some common page sizes and even if they did not, the resulting page
> size would be 0x8000.0000.0000.0000 (gcc 9.2) and
> ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Applied to ppc-for-5.1.

> ---
>  hw/vfio/spapr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 33692fc86fd6..2900bd19417a 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>  {
>      int ret = 0;
>      IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> -    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
> +    uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
>      unsigned entries, bits_total, bits_per_level, max_levels;
>      struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
>      long rampagesize = qemu_minrampagesize();
> @@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
>      if (pagesize > rampagesize) {
>          pagesize = rampagesize;
>      }
> -    pagesize = 1ULL << (63 - clz64(container->pgsizes &
> -                                   (pagesize | (pagesize - 1))));
> +    pgmask = container->pgsizes & (pagesize | (pagesize - 1));
> +    pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
>      if (!pagesize) {
>          error_report("Host doesn't support page size 0x%"PRIx64
>                       ", the supported mask is 0x%lx",

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[Peter Maydell]

On Thu, 26 Mar 2020 at 00:39, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Tue, Mar 24, 2020 at 05:39:12PM +1100, Alexey Kardashevskiy wrote:
> > Coverity detected an issue (CID 1421903) with potential call of clz64(0)
> > which returns 64 which make it do "<<" with a negative number.
> >
> > This checks the mask and avoids undefined behaviour.
> >
> > In practice pgsizes and memory_region_iommu_get_min_page_size() always
> > have some common page sizes and even if they did not, the resulting page
> > size would be 0x8000.0000.0000.0000 (gcc 9.2) and
> > ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
> >
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> Applied to ppc-for-5.1.

As a coverity-issue-fix it would be nice to have this in
5.0 unless you think it's particularly risky.

thanks
-- PMM

Re: [PATCH qemu] vfio/spapr: Fix page size calculation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Thu, Mar 26, 2020 at 11:21:47AM +0000, Peter Maydell wrote:
> On Thu, 26 Mar 2020 at 00:39, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Tue, Mar 24, 2020 at 05:39:12PM +1100, Alexey Kardashevskiy wrote:
> > > Coverity detected an issue (CID 1421903) with potential call of clz64(0)
> > > which returns 64 which make it do "<<" with a negative number.
> > >
> > > This checks the mask and avoids undefined behaviour.
> > >
> > > In practice pgsizes and memory_region_iommu_get_min_page_size() always
> > > have some common page sizes and even if they did not, the resulting page
> > > size would be 0x8000.0000.0000.0000 (gcc 9.2) and
> > > ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.
> > >
> > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
> > Applied to ppc-for-5.1.
> 
> As a coverity-issue-fix it would be nice to have this in
> 5.0 unless you think it's particularly risky.

In fact, I realized that shortly after and moved it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]