Re: [PULL 22/33] spapr: Implement Open Firmware client interface

[Peter Maydell]

On Fri, 9 Jul 2021 at 06:17, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> The PAPR platform describes an OS environment that's presented by
> a combination of a hypervisor and firmware. The features it specifies
> require collaboration between the firmware and the hypervisor.

Another Coverity issue unrelated to the others: CID 1458132:

> +int vof_client_call(MachineState *ms, Vof *vof, void *fdt,
> +                    target_ulong args_real)
> +{
> +    struct prom_args args_be;
> +    uint32_t args[ARRAY_SIZE(args_be.args)];
> +    uint32_t rets[ARRAY_SIZE(args_be.args)] = { 0 }, ret;
> +    char service[64];
> +    unsigned nargs, nret, i;
> +
> +    if (VOF_MEM_READ(args_real, &args_be, sizeof(args_be)) != MEMTX_OK) {
> +        return -EINVAL;
> +    }
> +    nargs = be32_to_cpu(args_be.nargs);
> +    if (nargs >= ARRAY_SIZE(args_be.args)) {

Our only bounds check against overflowing the args arrays is here,
on 'nargs'...

> +        return -EINVAL;
> +    }
> +
> +    if (VOF_MEM_READ(be32_to_cpu(args_be.service), service, sizeof(service)) 
> !=
> +        MEMTX_OK) {
> +        return -EINVAL;
> +    }
> +    if (strnlen(service, sizeof(service)) == sizeof(service)) {
> +        /* Too long service name */
> +        return -EINVAL;
> +    }
> +
> +    for (i = 0; i < nargs; ++i) {
> +        args[i] = be32_to_cpu(args_be.args[i]);
> +    }
> +
> +    nret = be32_to_cpu(args_be.nret);
> +    ret = vof_client_handle(ms, fdt, vof, service, args, nargs, rets, nret);
> +    if (!nret) {
> +        return 0;
> +    }
> +
> +    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]);
> +    }

...but in the code we fill the args_be array with "nargs + 1 + nret - 1"
values.

Side note: is the code really intentionally copying only nret-1 values
from rets[], or is the loop condition supposed to be "<= nret" ?

> +
> +    if (VOF_MEM_WRITE(args_real + offsetof(struct prom_args, args[nargs]),
> +                      args_be.args + nargs, sizeof(args_be.args[0]) * nret) 
> !=
> +        MEMTX_OK) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}

thanks
-- PMM