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

[David Gibson]

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

signature.asc
Description: PGP signature