[PATCH qemu] ppc/vof: Fix Coverity issues

[PATCH qemu] ppc/vof: Fix Coverity issues

[Alexey Kardashevskiy]

This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity.

This adds a comment about the return parameters number in the VOF hcall.
The reason for such counting is to keep the numbers look the same in
vof_client_handle() and the Linux (an OF client).

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

Will this make COverity happy? What is the canonical way of fixing these
uint32_t vs. int? Thanks,

---
 hw/ppc/vof.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index 81f65962156c..872f671babbe 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle)
 static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
                                     uint32_t buf, uint32_t len)
 {
-    uint32_t ret = -1;
+    int ret = -1;
     char tmp[VOF_MAX_PATH] = "";
 
     ret = phandle_to_path(fdt, phandle, tmp, sizeof(tmp));
@@ -529,13 +529,13 @@ static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
 
     trace_vof_package_to_path(phandle, tmp, ret);
 
-    return ret;
+    return (uint32_t) ret;
 }
 
 static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle,
                                      uint32_t buf, uint32_t len)
 {
-    uint32_t ret = -1;
+    int ret = -1;
     uint32_t phandle = vof_instance_to_package(vof, ihandle);
     char tmp[VOF_MAX_PATH] = "";
 
@@ -549,7 +549,7 @@ static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle,
     }
     trace_vof_instance_to_path(ihandle, phandle, tmp, ret);
 
-    return ret;
+    return (uint32_t) ret;
 }
 
 static uint32_t vof_write(Vof *vof, uint32_t ihandle, uint32_t buf,
@@ -965,11 +965,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void *fdt,
     }
 
     nret = be32_to_cpu(args_be.nret);
+    if (nret > ARRAY_SIZE(args_be.args) - nargs) {
+        return -EINVAL;
+    }
     ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret);
     if (!nret) {
         return 0;
     }
 
+    /* @nrets includes the value which this function returns */
     args_be.args[nargs] = cpu_to_be32(ret);
     for (i = 1; i < nret; ++i) {
         args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);
-- 
2.30.2

Re: [PATCH qemu] ppc/vof: Fix Coverity issues

[David Gibson]

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

On Tue, Jul 13, 2021 at 11:46:38PM +1000, Alexey Kardashevskiy wrote:
> This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity.
> 
> This adds a comment about the return parameters number in the VOF hcall.
> The reason for such counting is to keep the numbers look the same in
> vof_client_handle() and the Linux (an OF client).
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> Will this make COverity happy? What is the canonical way of fixing these
> uint32_t vs. int? Thanks,

It might make Coverity happy, but I think it's an ugly approach.

> 
> ---
>  hw/ppc/vof.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> index 81f65962156c..872f671babbe 100644
> --- a/hw/ppc/vof.c
> +++ b/hw/ppc/vof.c
> @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle)
>  static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
>                                      uint32_t buf, uint32_t len)
>  {
> -    uint32_t ret = -1;
> +    int ret = -1;

I don't think you want to try to use the same variable for the value
from phandle_to_path() and the return value from this function -
they're different types, with different encodings.  The inner value
should remain int (that's the libfdt convention).

The outer one is explicltly unsigned.  You're not really looking for
negative error values, but specifically for -1U == ~0U as the single
error value.  So re-introduce your PROM_ERROR valued, defined as ~0U,
so that it's clearly unsigned, and use that and unsigned logic for all
manipulation of the outer value.

>      char tmp[VOF_MAX_PATH] = "";
>  
>      ret = phandle_to_path(fdt, phandle, tmp, sizeof(tmp));
> @@ -529,13 +529,13 @@ static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
>  
>      trace_vof_package_to_path(phandle, tmp, ret);
>  
> -    return ret;
> +    return (uint32_t) ret;
>  }
>  
>  static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle,
>                                       uint32_t buf, uint32_t len)
>  {
> -    uint32_t ret = -1;
> +    int ret = -1;
>      uint32_t phandle = vof_instance_to_package(vof, ihandle);
>      char tmp[VOF_MAX_PATH] = "";
>  
> @@ -549,7 +549,7 @@ static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle,
>      }
>      trace_vof_instance_to_path(ihandle, phandle, tmp, ret);
>  
> -    return ret;
> +    return (uint32_t) ret;
>  }
>  
>  static uint32_t vof_write(Vof *vof, uint32_t ihandle, uint32_t buf,
> @@ -965,11 +965,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void *fdt,
>      }
>  
>      nret = be32_to_cpu(args_be.nret);
> +    if (nret > ARRAY_SIZE(args_be.args) - nargs) {
> +        return -EINVAL;
> +    }

That looks reasonable.

>      ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret);
>      if (!nret) {
>          return 0;
>      }
>  
> +    /* @nrets includes the value which this function returns */
>      args_be.args[nargs] = cpu_to_be32(ret);
>      for (i = 1; i < nret; ++i) {
>          args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);

-- 
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] ppc/vof: Fix Coverity issues

[Greg Kurz]

On Tue, 13 Jul 2021 23:46:38 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity.
> 
> This adds a comment about the return parameters number in the VOF hcall.
> The reason for such counting is to keep the numbers look the same in
> vof_client_handle() and the Linux (an OF client).
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> Will this make COverity happy? What is the canonical way of fixing these
> uint32_t vs. int? Thanks,
> 

You might want to mention the Coverity IDs fixed by this
patch in the changelog, e.g.

Fixes: CID xxxxxxx, yyyyyyy

> ---
>  hw/ppc/vof.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> index 81f65962156c..872f671babbe 100644
> --- a/hw/ppc/vof.c
> +++ b/hw/ppc/vof.c
> @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle)
>  static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
>                                      uint32_t buf, uint32_t len)
>  {
> -    uint32_t ret = -1;
> +    int ret = -1;
>      char tmp[VOF_MAX_PATH] = "";
>  
>      ret = phandle_to_path(fdt, phandle, tmp, sizeof(tmp));
> @@ -529,13 +529,13 @@ static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
>  
>      trace_vof_package_to_path(phandle, tmp, ret);
>  
> -    return ret;
> +    return (uint32_t) ret;
>  }
>  
>  static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle,
>                                       uint32_t buf, uint32_t len)
>  {
> -    uint32_t ret = -1;
> +    int ret = -1;
>      uint32_t phandle = vof_instance_to_package(vof, ihandle);
>      char tmp[VOF_MAX_PATH] = "";
>  
> @@ -549,7 +549,7 @@ static uint32_t vof_instance_to_path(void *fdt, Vof *vof, uint32_t ihandle,
>      }
>      trace_vof_instance_to_path(ihandle, phandle, tmp, ret);
>  
> -    return ret;
> +    return (uint32_t) ret;
>  }
>  
>  static uint32_t vof_write(Vof *vof, uint32_t ihandle, uint32_t buf,
> @@ -965,11 +965,15 @@ int vof_client_call(MachineState *ms, Vof *vof, void *fdt,
>      }
>  
>      nret = be32_to_cpu(args_be.nret);
> +    if (nret > ARRAY_SIZE(args_be.args) - nargs) {
> +        return -EINVAL;
> +    }
>      ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret);
>      if (!nret) {
>          return 0;
>      }
>  
> +    /* @nrets includes the value which this function returns */
>      args_be.args[nargs] = cpu_to_be32(ret);
>      for (i = 1; i < nret; ++i) {
>          args_be.args[nargs + i] = cpu_to_be32(rets[i - 1]);

Re: [PATCH qemu] ppc/vof: Fix Coverity issues

[Alexey Kardashevskiy]

On 7/19/21 13:57, David Gibson wrote:
> On Tue, Jul 13, 2021 at 11:46:38PM +1000, Alexey Kardashevskiy wrote:
>> This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity.
>>
>> This adds a comment about the return parameters number in the VOF hcall.
>> The reason for such counting is to keep the numbers look the same in
>> vof_client_handle() and the Linux (an OF client).
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>> Will this make COverity happy? What is the canonical way of fixing these
>> uint32_t vs. int? Thanks,
> 
> It might make Coverity happy, but I think it's an ugly approach.
> 
>>
>> ---
>>   hw/ppc/vof.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
>> index 81f65962156c..872f671babbe 100644
>> --- a/hw/ppc/vof.c
>> +++ b/hw/ppc/vof.c
>> @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle)
>>   static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
>>                                       uint32_t buf, uint32_t len)
>>   {
>> -    uint32_t ret = -1;
>> +    int ret = -1;
> 
> I don't think you want to try to use the same variable for the value
> from phandle_to_path() and the return value from this function -
> they're different types, with different encodings.  The inner value
> should remain int (that's the libfdt convention).
> 
> The outer one is explicltly unsigned.  You're not really looking for
> negative error values, but specifically for -1U == ~0U as the single
> error value.  So re-introduce your PROM_ERROR valued, defined as ~0U,
> so that it's clearly unsigned, and use that and unsigned logic for all
> manipulation of the outer value.


Fair enough. One question. Linux defines it as

#define PROM_ERROR (-1u)

Do you still vote for "~0U"?



-- 
Alexey

Re: [PATCH qemu] ppc/vof: Fix Coverity issues

[David Gibson]

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

On Mon, Jul 19, 2021 at 06:25:53PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 7/19/21 13:57, David Gibson wrote:
> > On Tue, Jul 13, 2021 at 11:46:38PM +1000, Alexey Kardashevskiy wrote:
> > > This fixes NEGATIVE_RETURNS, OVERRUN issues reported by the Coverity.
> > > 
> > > This adds a comment about the return parameters number in the VOF hcall.
> > > The reason for such counting is to keep the numbers look the same in
> > > vof_client_handle() and the Linux (an OF client).
> > > 
> > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > ---
> > > 
> > > Will this make COverity happy? What is the canonical way of fixing these
> > > uint32_t vs. int? Thanks,
> > 
> > It might make Coverity happy, but I think it's an ugly approach.
> > 
> > > 
> > > ---
> > >   hw/ppc/vof.c | 12 ++++++++----
> > >   1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > index 81f65962156c..872f671babbe 100644
> > > --- a/hw/ppc/vof.c
> > > +++ b/hw/ppc/vof.c
> > > @@ -517,7 +517,7 @@ static uint32_t vof_instance_to_package(Vof *vof, uint32_t ihandle)
> > >   static uint32_t vof_package_to_path(const void *fdt, uint32_t phandle,
> > >                                       uint32_t buf, uint32_t len)
> > >   {
> > > -    uint32_t ret = -1;
> > > +    int ret = -1;
> > 
> > I don't think you want to try to use the same variable for the value
> > from phandle_to_path() and the return value from this function -
> > they're different types, with different encodings.  The inner value
> > should remain int (that's the libfdt convention).
> > 
> > The outer one is explicltly unsigned.  You're not really looking for
> > negative error values, but specifically for -1U == ~0U as the single
> > error value.  So re-introduce your PROM_ERROR valued, defined as ~0U,
> > so that it's clearly unsigned, and use that and unsigned logic for all
> > manipulation of the outer value.
> 
> 
> Fair enough. One question. Linux defines it as
> 
> #define PROM_ERROR (-1u)
> 
> Do you still vote for "~0U"?

I don't really mind.  I think (-1U) might cause some more Coverity
confusion that ~0U, based on experience with Coverity scans of dtc &
libfdt.

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