[PATCH for-5.1] hw/arm/armsse: Assert info->num_cpus is in-bounds in armsse_realize()

[PATCH for-5.1] hw/arm/armsse: Assert info->num_cpus is in-bounds in armsse_realize()

[Peter Maydell]

In armsse_realize() we have a loop over [0, info->num_cpus), which
indexes into various fixed-size arrays in the ARMSSE struct.  This
confuses Coverity, which warns that we might overrun those arrays
(CID 1430326, 1430337, 1430371, 1430414, 1430430).  This can't
actually happen, because the info struct is always one of the entries
in the armsse_variants[] array and num_cpus is either 1 or 2; we also
already assert in armsse_init() that num_cpus is not too large.
However, adding an assert to armsse_realize() like the one in
armsse_init() should help Coverity figure out that these code paths
aren't possible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/armsse.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 64fcab895f7..dcbff9bd8f4 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -452,6 +452,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    assert(info->num_cpus <= SSE_MAX_CPUS);
+
     /* max SRAM_ADDR_WIDTH: 24 - log2(SRAM_NUM_BANK) */
     assert(is_power_of_2(info->sram_banks));
     addr_width_max = 24 - ctz32(info->sram_banks);
-- 
2.20.1

Re: [PATCH for-5.1] hw/arm/armsse: Assert info->num_cpus is in-bounds in armsse_realize()

[Philippe Mathieu-Daudé]

On 7/13/20 4:37 PM, Peter Maydell wrote:
> In armsse_realize() we have a loop over [0, info->num_cpus), which
> indexes into various fixed-size arrays in the ARMSSE struct.  This
> confuses Coverity, which warns that we might overrun those arrays
> (CID 1430326, 1430337, 1430371, 1430414, 1430430).  This can't
> actually happen, because the info struct is always one of the entries
> in the armsse_variants[] array and num_cpus is either 1 or 2; we also
> already assert in armsse_init() that num_cpus is not too large.
> However, adding an assert to armsse_realize() like the one in
> armsse_init() should help Coverity figure out that these code paths
> aren't possible.

Similar to commit 1db889c71f ("hw/openrisc/openrisc_sim: Add assertion
to silence GCC warning").

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/armsse.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
> index 64fcab895f7..dcbff9bd8f4 100644
> --- a/hw/arm/armsse.c
> +++ b/hw/arm/armsse.c
> @@ -452,6 +452,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    assert(info->num_cpus <= SSE_MAX_CPUS);
> +
>      /* max SRAM_ADDR_WIDTH: 24 - log2(SRAM_NUM_BANK) */
>      assert(is_power_of_2(info->sram_banks));
>      addr_width_max = 24 - ctz32(info->sram_banks);
> 

Re: [PATCH for-5.1] hw/arm/armsse: Assert info->num_cpus is in-bounds in armsse_realize()

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200713143716.9881-1-peter.maydell@linaro.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-unit: tests/test-crypto-secret
  TEST    check-unit: tests/test-char
**
ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
ERROR test-char - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1204:char_serial_test: 'chr' should not be NULL
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 017
  TEST    iotest-qcow2: 018
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=a72d62fc12f242e5a79799d2742f6b37', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-s6rziv2y/src/docker-src.2020-07-13-11.15.06.18606:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=a72d62fc12f242e5a79799d2742f6b37
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-s6rziv2y/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    15m51.537s
user    0m9.092s


The full log is available at
http://patchew.org/logs/20200713143716.9881-1-peter.maydell@linaro.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

[PATCH for-5.1] hw/arm/armsse: Assert info->num_cpus is in-bounds in armsse_realize()

[Peter Maydell]

In armsse_realize() we have a loop over [0, info->num_cpus), which
indexes into various fixed-size arrays in the ARMSSE struct.  This
confuses Coverity, which warns that we might overrun those arrays
(CID 1430326, 1430337, 1430371, 1430414, 1430430).  This can't
actually happen, because the info struct is always one of the entries
in the armsse_variants[] array and num_cpus is either 1 or 2; we also
already assert in armsse_init() that num_cpus is not too large.
However, adding an assert to armsse_realize() like the one in
armsse_init() should help Coverity figure out that these code paths
aren't possible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/armsse.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 64fcab895f7..dcbff9bd8f4 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -452,6 +452,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    assert(info->num_cpus <= SSE_MAX_CPUS);
+
     /* max SRAM_ADDR_WIDTH: 24 - log2(SRAM_NUM_BANK) */
     assert(is_power_of_2(info->sram_banks));
     addr_width_max = 24 - ctz32(info->sram_banks);
-- 
2.20.1