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

[David Gibson]

On Tue, Jul 20, 2021 at 03:07:26PM +1000, Alexey Kardashevskiy wrote:
> Coverity reported issues which are caused by mixing of signed return codes
> from DTC and unsigned return codes of the client interface.
> 
> This introduces PROM_ERROR and makes distinction between the error types.
> 
> This fixes NEGATIVE_RETURNS, OVERRUN issues reported by 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).
> 
> vmc->client_architecture_support() returns target_ulong and we want to
> propagate this to the client (for example H_MULTI_THREADS_ACTIVE).
> The VOF path to do_client_architecture_support() needs chopping off
> the top 32bit but SLOF's H_CAS does not; and either way the return values
> are either 0 or 32bit negative error code. For now this chops
> the top 32bits.
> 
> This makes "claim" fail if the allocated address is above 4GB as
> the client interface is 32bit. This still allows claiming memory above
> 4GB as potentially initrd can be put there and the client can read
> the address from the FDT's "available" property.
> 
> Fixes: CID 1458139, 1458138, 1458137, 1458133, 1458132
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Well, I don't know for sure if it will fix every one, but it certainly
looks like an improvement.  So, applied to ppc-for-6.1.

Couple of minor notes:
[snip]
> @@ -461,18 +458,18 @@ trace_exit:
>  uint32_t vof_client_open_store(void *fdt, Vof *vof, const char *nodename,
>                                 const char *prop, const char *path)
>  {
> -    int node = fdt_path_offset(fdt, nodename);
> -    int inst, offset;
> +    int offset, node = fdt_path_offset(fdt, nodename);
> +    uint32_t inst;
>  
>      offset = fdt_path_offset(fdt, path);
>      if (offset < 0) {
>          trace_vof_error_unknown_path(path);
> -        return offset;
> +        return PROM_ERROR;

Note that this case was a real bug, not just Coverity being pedantic.

>      }
>  
>      inst = vof_do_open(fdt, vof, offset, path);
>  
> -    return fdt_setprop_cell(fdt, node, prop, inst);
> +    return fdt_setprop_cell(fdt, node, prop, inst) >= 0 ? 0 : PROM_ERROR;
>  }
>  
>  static uint32_t vof_open(void *fdt, Vof *vof, uint32_t pathaddr)
> @@ -481,13 +478,13 @@ static uint32_t vof_open(void *fdt, Vof *vof, uint32_t 
> pathaddr)
>      int offset;
>  
>      if (readstr(pathaddr, path, sizeof(path))) {
> -        return -1;
> +        return PROM_ERROR;
>      }
>  
>      offset = path_offset(fdt, path);
>      if (offset < 0) {
>          trace_vof_error_unknown_path(path);
> -        return offset;
> +        return PROM_ERROR;

As is this one.

[snip]
> @@ -826,7 +824,7 @@ trace_exit:
>  static uint32_t vof_call_interpret(uint32_t cmdaddr, uint32_t param1,
>                                     uint32_t param2, uint32_t *ret2)
>  {
> -    uint32_t ret = -1;
> +    uint32_t ret = PROM_ERROR;
>      char cmd[VOF_MAX_FORTHCODE] = "";
>  
>      /* No interpret implemented so just call a trace */
> @@ -895,13 +893,20 @@ static uint32_t vof_client_handle(MachineState *ms, 
> void *fdt, Vof *vof,
>      } else if (cmpserv("write", 3, 1)) {
>          ret = vof_write(vof, args[0], args[1], args[2]);
>      } else if (cmpserv("claim", 3, 1)) {
> -        ret = vof_claim(vof, args[0], args[1], args[2]);
> -        if (ret != -1) {
> +        uint64_t ret64 = vof_claim(vof, args[0], args[1], args[2]);
> +
> +        if (ret64 < 0x100000000UL) {

This assumes that 2^32 fits in an unsigned long; I'm not certainly if
that's true for all supported qemu host arches.  ULL would be safer
here.

>              vof_dt_memory_available(fdt, vof->claimed, vof->claimed_base);
> +            ret = (uint32_t)ret64;
> +        } else {
> +            if (ret64 != -1) {
> +                vof_release(vof, ret, args[1]);
> +            }
> +            ret = PROM_ERROR;
>          }
>      } else if (cmpserv("release", 2, 0)) {
>          ret = vof_release(vof, args[0], args[1]);
> -        if (ret != -1) {
> +        if (ret != PROM_ERROR) {
>              vof_dt_memory_available(fdt, vof->claimed, vof->claimed_base);
>          }
>      } else if (cmpserv("call-method", 0, 0)) {
> @@ -965,11 +970,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]);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 6e90a0107247..da6e74b80dc1 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -88,8 +88,8 @@ vof_getproplen(uint32_t ph, const char *prop, uint32_t ret) 
> "ph=0x%x \"%s\" => l
>  vof_setprop(uint32_t ph, const char *prop, const char *val, uint32_t vallen, 
> uint32_t ret) "ph=0x%x \"%s\" [%s] len=%d => ret=%d"
>  vof_open(const char *path, uint32_t ph, uint32_t ih) "%s ph=0x%x => ih=0x%x"
>  vof_interpret(const char *cmd, uint32_t param1, uint32_t param2, uint32_t 
> ret, uint32_t ret2) "[%s] 0x%x 0x%x => 0x%x 0x%x"
> -vof_package_to_path(uint32_t ph, const char *tmp, uint32_t ret) "ph=0x%x => 
> %s len=%d"
> -vof_instance_to_path(uint32_t ih, uint32_t ph, const char *tmp, uint32_t 
> ret) "ih=0x%x ph=0x%x => %s len=%d"
> +vof_package_to_path(uint32_t ph, const char *tmp, int ret) "ph=0x%x => %s 
> len=%d"
> +vof_instance_to_path(uint32_t ih, uint32_t ph, const char *tmp, int ret) 
> "ih=0x%x ph=0x%x => %s len=%d"
>  vof_instance_to_package(uint32_t ih, uint32_t ph) "ih=0x%x => ph=0x%x"
>  vof_write(uint32_t ih, unsigned cb, const char *msg) "ih=0x%x [%u] \"%s\""
>  vof_avail(uint64_t start, uint64_t end, uint64_t size) 
> "0x%"PRIx64"..0x%"PRIx64" size=0x%"PRIx64

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