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

[Peter Maydell]

On Mon, 9 Aug 2021 at 10:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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:

Since I'd mostly written the code here anyway, I turned it
into an actual patch:
20210812144647.10516-1-peter.maydell@linaro.org/">https://patchew.org/QEMU/20210812144647.10516-1-peter.maydell@linaro.org/

-- PMM