[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 00/10] target/arm: Various v8.1M minor features

[Peter Maydell]

This patchseries implements various minor v8.1M new features,
notably the branch-future and low-overhead-loop extensions.

(None of this will get enabled until we have enough to implement
a CPU model which has v8.1M, which will be the Cortex-M55, but
as usual we can get stuff into the tree gradually.)

Patch 1 is a decodetree fix suggested by Richard that is
necessary to avoid wrong-decode of the changes to t32.decode
by later patches.

thanks
-- PMM

Peter Maydell (10):
  decodetree: Fix codegen for non-overlapping group inside overlapping
    group
  target/arm: Implement v8.1M NOCP handling
  target/arm: Implement v8.1M conditional-select insns
  target/arm: Make the t32 insn[25:23]=111 group non-overlapping
  target/arm: Don't allow BLX imm for M-profile
  target/arm: Implement v8.1M branch-future insns (as NOPs)
  target/arm: Implement v8.1M low-overhead-loop instructions
  target/arm: Fix has_vfp/has_neon ID reg squashing for M-profile
  target/arm: Implement FPSCR.LTPSIZE for M-profile LOB extension
  target/arm: Fix writing to FPSCR.FZ16 on M-profile

 target/arm/cpu.h               |   7 ++
 target/arm/m-nocp.decode       |  10 ++-
 target/arm/t32.decode          |  50 +++++++----
 target/arm/cpu.c               |  34 ++++---
 target/arm/translate.c         | 157 +++++++++++++++++++++++++++++++++
 target/arm/vfp_helper.c        |  30 +++++--
 scripts/decodetree.py          |   2 +-
 target/arm/translate-vfp.c.inc |  17 +++-
 8 files changed, 268 insertions(+), 39 deletions(-)

-- 
2.20.1

[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