Re: [PULL 05/18] hw/riscv: virt: Allow creating multiple NUMA sockets

[Peter Maydell]

On Tue, 25 Aug 2020 at 20:03, Alistair Francis <alistair.francis@wdc.com> wrote:
>
> From: Anup Patel <anup.patel@wdc.com>
>
> We extend RISC-V virt machine to allow creating a multi-socket
> machine. Each RISC-V virt machine socket is a NUMA node having
> a set of HARTs, a memory instance, a CLINT instance, and a PLIC
> instance. Other devices are shared between all sockets. We also
> update the generated device tree accordingly.

Hi; Coverity (CID 1460752) points out that this code has
a misunderstanding of the length argument to strncat().
(I think this patch is just doing code-movement of this block of code,
but it seemed like the easiest place to send an email about the issue.)

> +        /* Per-socket PLIC hart topology configuration string */
> +        plic_hart_config_len =
> +            (strlen(VIRT_PLIC_HART_CONFIG) + 1) * hart_count;
> +        plic_hart_config = g_malloc0(plic_hart_config_len);
> +        for (j = 0; j < hart_count; j++) {
> +            if (j != 0) {
> +                strncat(plic_hart_config, ",", plic_hart_config_len);
> +            }
> +            strncat(plic_hart_config, VIRT_PLIC_HART_CONFIG,
> +                plic_hart_config_len);
> +            plic_hart_config_len -= (strlen(VIRT_PLIC_HART_CONFIG) + 1);
> +        }

The length argument to strncat() is here being used as if it were
"do not write more than length bytes", but strncat() will write
length+1 bytes in the "source too long" case (length characters
from the source string plus the trailing NUL). This isn't actually
an issue here because we carefully precalculate the allocation length
to be exactly correct, but it means that the code looks like it has
a guard against accidental miscalculation and overrun but it doesn't.

It might be preferable to write this to use glib string methods
rather than raw strlen/strncat, for example:

    const char **vals;
    const char *hart_config = VIRT_PLIC_HART_CONFIG;
    int i;

    vals = g_new(char *, hart_count + 1);
    for (i = 0; i < hart_count; i++) {
         vals[i] = (gchar *)hart_config;
    }
    vals[i] = NULL;
    /* g_strjoinv() obliges us to discard 'const' here :-( */
    plic_hart_config = g_strjoinv(",", (char **)vals);

Untested, but same structure as used in ppc_compat_add_property()
and qemu_rbd_mon_host(). Relieves us of the obligation to
carefully calculate string lengths and makes it clearer that
we're constructing a comma-separated string.

thanks
-- PMM