QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH qemu] vfio/spapr: Fix page size calculation
@ 2020-03-24  6:39 Alexey Kardashevskiy
  2020-03-24 13:27 ` Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Alexey Kardashevskiy @ 2020-03-24  6:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexey Kardashevskiy, Alex Williamson, qemu-ppc, David Gibson

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-24  6:39 [PATCH qemu] vfio/spapr: Fix page size calculation Alexey Kardashevskiy
@ 2020-03-24 13:27 ` Philippe Mathieu-Daudé
  2020-03-24 14:30   ` Greg Kurz
  2020-03-24 14:29 ` Greg Kurz
  2020-03-26  0:11 ` David Gibson
  2 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-24 13:27 UTC (permalink / raw)
  To: Alexey Kardashevskiy, qemu-devel; +Cc: Alex Williamson, qemu-ppc, David Gibson



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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-24  6:39 [PATCH qemu] vfio/spapr: Fix page size calculation Alexey Kardashevskiy
  2020-03-24 13:27 ` Philippe Mathieu-Daudé
@ 2020-03-24 14:29 ` Greg Kurz
  2020-03-26  0:11 ` David Gibson
  2 siblings, 0 replies; 8+ messages in thread
From: Greg Kurz @ 2020-03-24 14:29 UTC (permalink / raw)
  To: Alexey Kardashevskiy; +Cc: Alex Williamson, qemu-ppc, qemu-devel, David Gibson

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-24 13:27 ` Philippe Mathieu-Daudé
@ 2020-03-24 14:30   ` Greg Kurz
  2020-03-24 14:47     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 8+ messages in thread
From: Greg Kurz @ 2020-03-24 14:30 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Alexey Kardashevskiy, Alex Williamson, qemu-ppc, qemu-devel,
	David Gibson

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-24 14:30   ` Greg Kurz
@ 2020-03-24 14:47     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-24 14:47 UTC (permalink / raw)
  To: Greg Kurz
  Cc: Alexey Kardashevskiy, Alex Williamson, qemu-ppc, qemu-devel,
	David Gibson

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-24  6:39 [PATCH qemu] vfio/spapr: Fix page size calculation Alexey Kardashevskiy
  2020-03-24 13:27 ` Philippe Mathieu-Daudé
  2020-03-24 14:29 ` Greg Kurz
@ 2020-03-26  0:11 ` David Gibson
  2020-03-26 11:21   ` Peter Maydell
  2 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2020-03-26  0:11 UTC (permalink / raw)
  To: Alexey Kardashevskiy; +Cc: Alex Williamson, qemu-ppc, qemu-devel


[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-26  0:11 ` David Gibson
@ 2020-03-26 11:21   ` Peter Maydell
  2020-03-26 11:28     ` David Gibson
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2020-03-26 11:21 UTC (permalink / raw)
  To: David Gibson
  Cc: Alexey Kardashevskiy, Alex Williamson, qemu-ppc, QEMU Developers

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH qemu] vfio/spapr: Fix page size calculation
  2020-03-26 11:21   ` Peter Maydell
@ 2020-03-26 11:28     ` David Gibson
  0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2020-03-26 11:28 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Alexey Kardashevskiy, Alex Williamson, qemu-ppc, QEMU Developers


[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24  6:39 [PATCH qemu] vfio/spapr: Fix page size calculation Alexey Kardashevskiy
2020-03-24 13:27 ` Philippe Mathieu-Daudé
2020-03-24 14:30   ` Greg Kurz
2020-03-24 14:47     ` Philippe Mathieu-Daudé
2020-03-24 14:29 ` Greg Kurz
2020-03-26  0:11 ` David Gibson
2020-03-26 11:21   ` Peter Maydell
2020-03-26 11:28     ` David Gibson

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git