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.

Hi; the latest version of Coverity has pointed out another bug
in this commit (CID 1487241):

> +static uint32_t vof_setprop(MachineState *ms, void *fdt, Vof *vof,
> +                            uint32_t nodeph, uint32_t pname,
> +                            uint32_t valaddr, uint32_t vallen)
> +{
> +    char propname[OF_PROPNAME_LEN_MAX + 1];

We don't initialize this array...

> +    uint32_t ret = -1;
> +    int offset;
> +    char trval[64] = "";
> +    char nodepath[VOF_MAX_PATH] = "";
> +    Object *vmo = object_dynamic_cast(OBJECT(ms), TYPE_VOF_MACHINE_IF);
> +    g_autofree char *val = NULL;
> +
> +    if (vallen > VOF_MAX_SETPROPLEN) {
> +        goto trace_exit;

...and this error-exit check happens before the readstr() that
fills in the propname[] array...

> +    }
> +    if (readstr(pname, propname, sizeof(propname))) {
> +        goto trace_exit;

...and if the readstr() fails then propname[] isn't set either...

> +    }

> +trace_exit:
> +    trace_vof_setprop(nodeph, propname, trval, vallen, ret);

...but on the 'trace_exit' error path we still try to trace
propname, which will use the %s format string on it, printing
a potentially arbitrary amount of garbage.

We should either initialize the propname array or else
not trace it for these error paths.

thanks
-- PMM