Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event

[Philippe Mathieu-Daudé]

On 05/16/2018 03:03 PM, Eric Auger wrote:
> Coverity complains about use of uninitialized Evt struct.
> The EVT_SET_TYPE and similar setters use deposit32() on fields
> in the struct, so they read the uninitialized existing values.
> In cases where we don't set all the fields in the event struct
> we'll end up leaking random uninitialized data from QEMU's
> stack into the guest.
> 
> Initializing the struct with "Evt evt = {};" ought to satisfy
> Coverity and fix the data leak.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/arm/smmuv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index b3026de..42dc521 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -143,7 +143,7 @@ static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
>  
>  void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>  {
> -    Evt evt;
> +    Evt evt = {};
>      MemTxResult r;
>  
>      if (!smmuv3_eventq_enabled(s)) {
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

[Philippe Mathieu-Daudé]

Hi Eric,

On 05/16/2018 03:03 PM, Eric Auger wrote:
> Coverity points out that this can overflow if n > 31,
> because it's only doing 32-bit arithmetic. Let's use 1ULL instead
> of 1. Also the formulae used to compute n can be replaced by
> the level_shift() macro.

This level_shift() replacement doesn't seems that obvious to me, can you
split it in another patch?

> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmu-common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 01c7be8..3c5f724 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
>  static inline hwaddr get_block_pte_address(uint64_t pte, int level,
>                                             int granule_sz, uint64_t *bsz)
>  {
> -    int n = (granule_sz - 3) * (4 - level) + 3;
> +    int n = level_shift(level, granule_sz);

Shouldn't this be level_shift(level + 1, granule_sz)?
Using level_shift() you replaced the trailing 3 by granule_sz. This
means the previous code was only correct for the granule_sz==3 case.

   level_shift(level + 1, granule_sz)
== (granule_sz - 3) * (3 - (level + 1)) + granule_sz;
== (granule_sz - 3) * (4 - level) + granule_sz;
!= (granule_sz - 3) * (4 - level) + 3;

>  
> -    *bsz = 1 << n;
> +    *bsz = 1ULL << n;

For the coverity fix (patch splitted):
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      return PTE_ADDRESS(pte, n);
>  }
>  

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

[Peter Maydell]

On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Eric,
>
> On 05/16/2018 03:03 PM, Eric Auger wrote:
>> Coverity points out that this can overflow if n > 31,
>> because it's only doing 32-bit arithmetic. Let's use 1ULL instead
>> of 1. Also the formulae used to compute n can be replaced by
>> the level_shift() macro.
>
> This level_shift() replacement doesn't seems that obvious to me, can you
> split it in another patch?
>
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmu-common.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>> index 01c7be8..3c5f724 100644
>> --- a/hw/arm/smmu-common.c
>> +++ b/hw/arm/smmu-common.c
>> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
>>  static inline hwaddr get_block_pte_address(uint64_t pte, int level,
>>                                             int granule_sz, uint64_t *bsz)
>>  {
>> -    int n = (granule_sz - 3) * (4 - level) + 3;
>> +    int n = level_shift(level, granule_sz);
>
> Shouldn't this be level_shift(level + 1, granule_sz)?

No. The two expressions are equivalent, they're
just arranged differently:

   level_shift(lvl, gsz)
      == gsz + (3 - lvl) * (gsz - 3)
      == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3)
      == gsz - gsz + (4 - lvl) * (gsz - 3) + 3
      == (gsz - 3) * (4 - lvl) + 3

thanks
-- PMM

[Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues

[Eric Auger]

This series includes 2 patches that fix Coverity issues respectively
in smmuv3 and smmu-common code.

Eric Auger (2):
  hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event
  hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

 hw/arm/smmu-common.c | 4 ++--
 hw/arm/smmuv3.c      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event

[Eric Auger]

Coverity complains about use of uninitialized Evt struct.
The EVT_SET_TYPE and similar setters use deposit32() on fields
in the struct, so they read the uninitialized existing values.
In cases where we don't set all the fields in the event struct
we'll end up leaking random uninitialized data from QEMU's
stack into the guest.

Initializing the struct with "Evt evt = {};" ought to satisfy
Coverity and fix the data leak.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/smmuv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index b3026de..42dc521 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -143,7 +143,7 @@ static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
 
 void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
 {
-    Evt evt;
+    Evt evt = {};
     MemTxResult r;
 
     if (!smmuv3_eventq_enabled(s)) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

[Eric Auger]

Coverity points out that this can overflow if n > 31,
because it's only doing 32-bit arithmetic. Let's use 1ULL instead
of 1. Also the formulae used to compute n can be replaced by
the level_shift() macro.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmu-common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 01c7be8..3c5f724 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
 static inline hwaddr get_block_pte_address(uint64_t pte, int level,
                                            int granule_sz, uint64_t *bsz)
 {
-    int n = (granule_sz - 3) * (4 - level) + 3;
+    int n = level_shift(level, granule_sz);
 
-    *bsz = 1 << n;
+    *bsz = 1ULL << n;
     return PTE_ADDRESS(pte, n);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

[Philippe Mathieu-Daudé]

On 05/16/2018 01:23 PM, Peter Maydell wrote:
> On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Eric,
>>
>> On 05/16/2018 03:03 PM, Eric Auger wrote:
>>> Coverity points out that this can overflow if n > 31,
>>> because it's only doing 32-bit arithmetic. Let's use 1ULL instead
>>> of 1. Also the formulae used to compute n can be replaced by
>>> the level_shift() macro.
>>
>> This level_shift() replacement doesn't seems that obvious to me, can you
>> split it in another patch?
>>
>>>
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  hw/arm/smmu-common.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>> index 01c7be8..3c5f724 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
>>>  static inline hwaddr get_block_pte_address(uint64_t pte, int level,
>>>                                             int granule_sz, uint64_t *bsz)
>>>  {
>>> -    int n = (granule_sz - 3) * (4 - level) + 3;
>>> +    int n = level_shift(level, granule_sz);
>>
>> Shouldn't this be level_shift(level + 1, granule_sz)?
> 
> No. The two expressions are equivalent, they're
> just arranged differently:
> 
>    level_shift(lvl, gsz)
>       == gsz + (3 - lvl) * (gsz - 3)
>       == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3)
>       == gsz - gsz + (4 - lvl) * (gsz - 3) + 3
>       == (gsz - 3) * (4 - lvl) + 3

Argh I failed this middle school demonstrations...

Thanks Peter :)

So for the much cleaner level_shift() use:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

[Auger Eric]

Hi Philippe,
On 05/16/2018 10:01 PM, Philippe Mathieu-Daudé wrote:
> On 05/16/2018 01:23 PM, Peter Maydell wrote:
>> On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Hi Eric,
>>>
>>> On 05/16/2018 03:03 PM, Eric Auger wrote:
>>>> Coverity points out that this can overflow if n > 31,
>>>> because it's only doing 32-bit arithmetic. Let's use 1ULL instead
>>>> of 1. Also the formulae used to compute n can be replaced by
>>>> the level_shift() macro.
>>>
>>> This level_shift() replacement doesn't seems that obvious to me, can you
>>> split it in another patch?
>>>
>>>>
>>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  hw/arm/smmu-common.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>>>> index 01c7be8..3c5f724 100644
>>>> --- a/hw/arm/smmu-common.c
>>>> +++ b/hw/arm/smmu-common.c
>>>> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
>>>>  static inline hwaddr get_block_pte_address(uint64_t pte, int level,
>>>>                                             int granule_sz, uint64_t *bsz)
>>>>  {
>>>> -    int n = (granule_sz - 3) * (4 - level) + 3;
>>>> +    int n = level_shift(level, granule_sz);
>>>
>>> Shouldn't this be level_shift(level + 1, granule_sz)?
>>
>> No. The two expressions are equivalent, they're
>> just arranged differently:
>>
>>    level_shift(lvl, gsz)
>>       == gsz + (3 - lvl) * (gsz - 3)
>>       == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3)
>>       == gsz - gsz + (4 - lvl) * (gsz - 3) + 3
>>       == (gsz - 3) * (4 - lvl) + 3
> 
> Argh I failed this middle school demonstrations...
> 
> Thanks Peter :)
> 
> So for the much cleaner level_shift() use:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thank you for the review!

Eric
> 

Re: [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues

[Peter Maydell]

On 16 May 2018 at 19:03, Eric Auger <eric.auger@redhat.com> wrote:
> This series includes 2 patches that fix Coverity issues respectively
> in smmuv3 and smmu-common code.
>
> Eric Auger (2):
>   hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event
>   hw/arm/smmu-common: Fix coverity issue in get_block_pte_address
>
>  hw/arm/smmu-common.c | 4 ++--
>  hw/arm/smmuv3.c      | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> --



Applied to target-arm.next, thanks.

-- PMM