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

[Philippe Mathieu-Daudé]

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>

> +    }
> +    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(
>