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.mayd...@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é <f4...@amsat.org> > + } > + vals[i] = NULL; > + > + /* g_strjoinv() obliges us to cast away const here */ > + return g_strjoinv(",", (char **)vals); > +} > + > static void virt_machine_init(MachineState *machine) > { > const MemMapEntry *memmap = virt_memmap; > @@ -549,13 +567,12 @@ static void virt_machine_init(MachineState *machine) > MemoryRegion *main_mem = g_new(MemoryRegion, 1); > MemoryRegion *mask_rom = g_new(MemoryRegion, 1); > char *plic_hart_config, *soc_name; > - size_t plic_hart_config_len; > target_ulong start_addr = memmap[VIRT_DRAM].base; > target_ulong firmware_end_addr, kernel_start_addr; > uint32_t fdt_load_addr; > uint64_t kernel_entry; > DeviceState *mmio_plic, *virtio_plic, *pcie_plic; > - int i, j, base_hartid, hart_count; > + int i, base_hartid, hart_count; > > /* Check socket count limit */ > if (VIRT_SOCKETS_MAX < riscv_socket_count(machine)) { > @@ -604,17 +621,7 @@ static void virt_machine_init(MachineState *machine) > SIFIVE_CLINT_TIMEBASE_FREQ, true); > > /* 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); > - } > + plic_hart_config = plic_hart_config_string(hart_count); > > /* Per-socket PLIC */ > s->plic[i] = sifive_plic_create( >