[Qemu-devel] [PATCH] arm_gicv3: Add assert()s to tell Coverity that offsets are aligned

[Qemu-devel] [PATCH] arm_gicv3: Add assert()s to tell Coverity that offsets are aligned

[Peter Maydell]

Coverity complains that the GICR_IPRIORITYR case in gicv3_readl()
can overflow an array, because it doesn't know that the offsets
passed to that function must be word aligned. Add some assert()s
which hopefully tell Coverity that this isn't possible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I don't have any way to test this except getting it into master
and seeing if Coverity still complains, but if it does then
I'll happily just mark the error as a false positive...

 hw/intc/arm_gicv3_redist.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 2f60096..77e5cfa 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -420,6 +420,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data,
     MemTxResult r;
     int cpuidx;
 
+    assert((offset & (size - 1)) == 0);
+
     /* This region covers all the redistributor pages; there are
      * (for GICv3) two 64K pages per CPU. At the moment they are
      * all contiguous (ie in this one region), though we might later
@@ -468,6 +470,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
     MemTxResult r;
     int cpuidx;
 
+    assert((offset & (size - 1)) == 0);
+
     /* This region covers all the redistributor pages; there are
      * (for GICv3) two 64K pages per CPU. At the moment they are
      * all contiguous (ie in this one region), though we might later
-- 
1.9.1

Re: [Qemu-devel] [PATCH] arm_gicv3: Add assert()s to tell Coverity that offsets are aligned

[Shannon Zhao]

On 2016/7/12 2:22, Peter Maydell wrote:
> Coverity complains that the GICR_IPRIORITYR case in gicv3_readl()
> can overflow an array, because it doesn't know that the offsets
> passed to that function must be word aligned. Add some assert()s
> which hopefully tell Coverity that this isn't possible.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I don't have any way to test this except getting it into master
> and seeing if Coverity still complains, but if it does then
> I'll happily just mark the error as a false positive...
> 
Since the codes are correct, maybe it could ignore the original complain
at Coverity instead of adding the assert(). But anyway I'm fine with
this patch.

>  hw/intc/arm_gicv3_redist.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index 2f60096..77e5cfa 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -420,6 +420,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data,
>      MemTxResult r;
>      int cpuidx;
>  
> +    assert((offset & (size - 1)) == 0);
> +
>      /* This region covers all the redistributor pages; there are
>       * (for GICv3) two 64K pages per CPU. At the moment they are
>       * all contiguous (ie in this one region), though we might later
> @@ -468,6 +470,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
>      MemTxResult r;
>      int cpuidx;
>  
> +    assert((offset & (size - 1)) == 0);
> +
>      /* This region covers all the redistributor pages; there are
>       * (for GICv3) two 64K pages per CPU. At the moment they are
>       * all contiguous (ie in this one region), though we might later
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH] arm_gicv3: Add assert()s to tell Coverity that offsets are aligned

[Peter Maydell]

On 12 July 2016 at 02:33, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>
>
> On 2016/7/12 2:22, Peter Maydell wrote:
>> Coverity complains that the GICR_IPRIORITYR case in gicv3_readl()
>> can overflow an array, because it doesn't know that the offsets
>> passed to that function must be word aligned. Add some assert()s
>> which hopefully tell Coverity that this isn't possible.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> I don't have any way to test this except getting it into master
>> and seeing if Coverity still complains, but if it does then
>> I'll happily just mark the error as a false positive...
>>
> Since the codes are correct, maybe it could ignore the original complain
> at Coverity instead of adding the assert(). But anyway I'm fine with
> this patch.

Yeah, I was on the fence about just ignoring it. But
it's not really feasible for an analysis tool to
deduce that the unaligned case can't happen, so
a little assistance seemed in order.

thanks
-- PMM