[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/riscv/virt.c: Assemble plic_hart_config string with g_str
From: |
Alistair Francis |
Subject: |
Re: [PATCH] hw/riscv/virt.c: Assemble plic_hart_config string with g_strjoinv() |
Date: |
Fri, 13 Aug 2021 10:57:12 +1000 |
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