Re: [PATCH] hw/riscv/virt.c: Assemble plic_hart_config string with g_strjoinv()

[Alistair Francis]

On Fri, Aug 13, 2021 at 2:17 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 8/12/21 4:46 PM, Peter Maydell wrote:
> > In the riscv virt machine init function, We assemble a string
> > plic_hart_config which is a comma-separated list of N copies of the
> > VIRT_PLIC_HART_CONFIG string.  The code that does this has a
> > misunderstanding of the strncat() length argument.  If the source
> > string is too large strncat() will write a maximum of length+1 bytes
> > (length bytes from the source string plus a trailing NUL), but the
> > code here assumes that it will write only length bytes at most.
> >
> > This isn't an actual bug because the code has correctly precalculated
> > the amount of memory it needs to allocate so that it will never be
> > too small (i.e.  we could have used plain old strcat()), but it does
> > mean that the code looks like it has a guard against accidental
> > overrun when it doesn't.
> >
> > Rewrite the string handling here to use the glib g_strjoinv()
> > function, which means we don't need to do careful accountancy of
> > string lengths, and makes it clearer that what we're doing is
> > "create a comma-separated string".
> >
> > Fixes: Coverity 1460752
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/riscv/virt.c | 33 ++++++++++++++++++++-------------
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 4a3cd2599a5..26bc8d289ba 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -541,6 +541,24 @@ static FWCfgState *create_fw_cfg(const MachineState 
> > *mc)
> >      return fw_cfg;
> >  }
> >
> > +/*
> > + * Return the per-socket PLIC hart topology configuration string
> > + * (caller must free with g_free())
> > + */
> > +static char *plic_hart_config_string(int hart_count)
> > +{
> > +    g_autofree const char **vals = g_new(const char *, hart_count + 1);
> > +    int i;
> > +
> > +    for (i = 0; i < hart_count; i++) {
> > +        vals[i] = VIRT_PLIC_HART_CONFIG;
>
> Have you considered adding plic_hart_config_string() an extra
> 'const char *plic_config' argument (declaring it in a new
> include/hw/riscv/plic_hart.h)?
> We could use it in the other boards:
>
> hw/riscv/microchip_pfsoc.c:267:            strncat(plic_hart_config, ","
> MICROCHIP_PFSOC_PLIC_HART_CONFIG,
> hw/riscv/microchip_pfsoc.c:268:                    plic_hart_config_len);
> hw/riscv/microchip_pfsoc.c:270:            strncat(plic_hart_config,
> "M", plic_hart_config_len);
>
> hw/riscv/sifive_u.c:826:            strncat(plic_hart_config, ","
> SIFIVE_U_PLIC_HART_CONFIG,
> hw/riscv/sifive_u.c:827:                    plic_hart_config_len);
> hw/riscv/sifive_u.c:829:            strncat(plic_hart_config, "M",
> plic_hart_config_len);
>
> hw/riscv/virt.c:612:                strncat(plic_hart_config, ",",
> plic_hart_config_len);
> hw/riscv/virt.c:614:            strncat(plic_hart_config,
> VIRT_PLIC_HART_CONFIG,
> hw/riscv/virt.c:615:                plic_hart_config_len);
>
> include/hw/riscv/microchip_pfsoc.h:141:#define
> MICROCHIP_PFSOC_PLIC_HART_CONFIG        "MS"
> include/hw/riscv/shakti_c.h:63:#define SHAKTI_C_PLIC_HART_CONFIG "MS"
> include/hw/riscv/sifive_e.h:83:#define SIFIVE_E_PLIC_HART_CONFIG "M"
> include/hw/riscv/sifive_u.h:147:#define SIFIVE_U_PLIC_HART_CONFIG "MS"
> include/hw/riscv/virt.h:74:#define VIRT_PLIC_HART_CONFIG "MS"
>
> Obviously someone else could do that as bytetask, so meanwhile
> for Coverity 1460752:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for fixing this Peter. Would you like this in for 6.1?

If you want I can fix the other boards?

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair