Changeset
hw/arm/smmu-common.c | 4 ++--
hw/arm/smmuv3.c      | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
Git apply log
Switched to a new branch '1526493784-25328-1-git-send-email-eric.auger@redhat.com'
Applying: hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event
Applying: hw/arm/smmu-common: Fix coverity issue in get_block_pte_address
To https://github.com/patchew-project/qemu
 + 25ed08b...beb074a patchew/1526493784-25328-1-git-send-email-eric.auger@redhat.com -> patchew/1526493784-25328-1-git-send-email-eric.auger@redhat.com (forced update)
Test passed: checkpatch

loading

Test passed: docker-mingw@fedora

loading

Test passed: docker-quick@centos7

loading

Test passed: s390x

loading

[Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues
Posted by Eric Auger, 6 days ago
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


Re: [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues
Posted by Peter Maydell, 5 days ago
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

[Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event
Posted by Eric Auger, 6 days ago
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


Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event
Posted by Philippe Mathieu-Daudé, 6 days ago
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)) {
> 

[Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address
Posted by Eric Auger, 6 days ago
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
Posted by Philippe Mathieu-Daudé, 6 days ago
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
Posted by Peter Maydell, 6 days ago
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

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address
Posted by Philippe Mathieu-Daudé, 6 days ago
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
Posted by Auger Eric, 5 days ago
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
>