[PATCH v2 0/2] hw/arm/smmu-common: Avoid smmu_find_smmu_pcibus return dangling pointer

[PATCH v2 0/2] hw/arm/smmu-common: Avoid smmu_find_smmu_pcibus return dangling pointer

[Philippe Mathieu-Daudé]

This series include the previous patch from Eric, then a
code refactor to avoid similar mistakes in the future.

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg683451.html
Supersedes: <20200226172628.17449-1-eric.auger@redhat.com>

Eric Auger (1):
  hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus

Philippe Mathieu-Daudé (1):
  hw/arm/smmu-common: Simplify smmu_find_smmu_pcibus() logic

 hw/arm/smmu-common.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

-- 
2.21.1

[PATCH v2 1/2] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus

[Philippe Mathieu-Daudé]

From: Eric Auger <eric.auger@redhat.com>

Make sure a null SMMUPciBus is returned in case we were
not able to identify a pci bus matching the @bus_num.

This matches the fix done on intel iommu in commit:
a2e1cd41ccfe796529abfd1b6aeb1dd4393762a2

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Message-Id: <20200226172628.17449-1-eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/smmu-common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 0f2573f004..67d7b2d0fd 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -301,6 +301,7 @@ SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
                 return smmu_pci_bus;
             }
         }
+        smmu_pci_bus = NULL;
     }
     return smmu_pci_bus;
 }
-- 
2.21.1

[PATCH v2 2/2] hw/arm/smmu-common: Simplify smmu_find_smmu_pcibus() logic

[Philippe Mathieu-Daudé]

The smmu_find_smmu_pcibus() function was introduced (in commit
cac994ef43b) in a code format that could return an incorrect
pointer, which was then fixed by the previous commit.
We could have avoid this by writing the if() statement differently.
Do it now, in case this function is re-used. The code is easier to
review (harder to miss bugs).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
This patch is easier to review with 'git-diff -w' (--ignore-all-space)
---
 hw/arm/smmu-common.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 67d7b2d0fd..e13a5f4a7c 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -290,20 +290,21 @@ inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
 SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
 {
     SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
+    GHashTableIter iter;
 
-    if (!smmu_pci_bus) {
-        GHashTableIter iter;
-
-        g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
-        while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
-            if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
-                s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
-                return smmu_pci_bus;
-            }
-        }
-        smmu_pci_bus = NULL;
+    if (smmu_pci_bus) {
+        return smmu_pci_bus;
     }
-    return smmu_pci_bus;
+
+    g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
+    while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
+        if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
+            s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
+            return smmu_pci_bus;
+        }
+    }
+
+    return NULL;
 }
 
 static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
-- 
2.21.1

Re: [PATCH v2 2/2] hw/arm/smmu-common: Simplify smmu_find_smmu_pcibus() logic

[Auger Eric]

Hi Philippe,
On 2/27/20 8:51 AM, Philippe Mathieu-Daudé wrote:
> The smmu_find_smmu_pcibus() function was introduced (in commit
> cac994ef43b) in a code format that could return an incorrect
> pointer, which was then fixed by the previous commit.
> We could have avoid this by writing the if() statement differently.
nit: avoided
> Do it now, in case this function is re-used. The code is easier to
> review (harder to miss bugs).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> This patch is easier to review with 'git-diff -w' (--ignore-all-space)
> ---
>  hw/arm/smmu-common.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 67d7b2d0fd..e13a5f4a7c 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -290,20 +290,21 @@ inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>  SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
>  {
>      SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
> +    GHashTableIter iter;
>  
> -    if (!smmu_pci_bus) {
> -        GHashTableIter iter;
> -
> -        g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
> -        while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
> -            if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
> -                s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
> -                return smmu_pci_bus;
> -            }
> -        }
> -        smmu_pci_bus = NULL;
> +    if (smmu_pci_bus) {
> +        return smmu_pci_bus;
>      }
> -    return smmu_pci_bus;
> +
> +    g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
> +    while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
> +        if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
> +            s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
> +            return smmu_pci_bus;
> +        }
> +    }
> +
> +    return NULL;
>  }
>  
>  static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> 
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Eric

Re: [PATCH v2 2/2] hw/arm/smmu-common: Simplify smmu_find_smmu_pcibus() logic

[Peter Xu]

On Thu, Feb 27, 2020 at 08:51:11AM +0100, Philippe Mathieu-Daudé wrote:
> The smmu_find_smmu_pcibus() function was introduced (in commit
> cac994ef43b) in a code format that could return an incorrect
> pointer, which was then fixed by the previous commit.
> We could have avoid this by writing the if() statement differently.
> Do it now, in case this function is re-used. The code is easier to
> review (harder to miss bugs).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu