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

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

[Peter Maydell]

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

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

[Peter Maydell]

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

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

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

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

[Peter Maydell]

On Thu, 12 Aug 2021 at 17:09, 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:

I hadn't noticed those, because Coverity doesn't complain about them.
Both sifive_u.c and microchip_pfsoc.c would need slightly different
code, though, because they are setting up a string like "M,MS,MS,MS"
where the first element is different from the others.

This is (I think) because they have the same misconception about
strncat()'s length argument, but they have a counterbalancing bug
where they reduce the 'remaining bytes in buffer' argument by 2 each
time round the loop even though the length of the first element in
their comma separated string is only 1 byte -- so they are accidentally
turning the length value into what it ought to be.

So those other board files should definitely also be updated to
use g_strjoinv(), but I'm not sure that we can usefully share code.
(We could have a function that takes an argument for the string
for the first CPU and one for the other CPUs, which would work
for all the boards we have now, but that feels a bit contrived
and maybe some other boards in future would want to make different
entries in the list be different...)

-- PMM

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

[Peter Maydell]

On Thu, 12 Aug 2021 at 17:09, 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:

I hadn't noticed those, because Coverity doesn't complain about them.
Both sifive_u.c and microchip_pfsoc.c would need slightly different
code, though, because they are setting up a string like "M,MS,MS,MS"
where the first element is different from the others.

This is (I think) because they have the same misconception about
strncat()'s length argument, but they have a counterbalancing bug
where they reduce the 'remaining bytes in buffer' argument by 2 each
time round the loop even though the length of the first element in
their comma separated string is only 1 byte -- so they are accidentally
turning the length value into what it ought to be.

So those other board files should definitely also be updated to
use g_strjoinv(), but I'm not sure that we can usefully share code.
(We could have a function that takes an argument for the string
for the first CPU and one for the other CPUs, which would work
for all the boards we have now, but that feels a bit contrived
and maybe some other boards in future would want to make different
entries in the list be different...)

-- PMM

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

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

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

[Peter Maydell]

On Fri, 13 Aug 2021 at 01:57, Alistair Francis <alistair23@gmail.com> wrote:
>
> 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>

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

No, this isn't 6.1 material -- as I note in the commit message,
the current code isn't actually buggy, just a bit misleading.

> If you want I can fix the other boards?

That would be great, thanks!

-- PMM

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

[Peter Maydell]

On Fri, 13 Aug 2021 at 01:57, Alistair Francis <alistair23@gmail.com> wrote:
>
> 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>

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

No, this isn't 6.1 material -- as I note in the commit message,
the current code isn't actually buggy, just a bit misleading.

> If you want I can fix the other boards?

That would be great, thanks!

-- PMM