[PATCH v3 0/2] use a do-while() loop in pnv_phbX_translate_tve()

[PATCH v3 0/2] use a do-while() loop in pnv_phbX_translate_tve()

[Daniel Henrique Barboza]

Hi,

This version contains fixes for the problems detected by Matheus
in the v2.

Changes from v2:
- patch 1:
  * fix loop condition
- patch 2:
  * fix loop condition
  * remove the initialization of 'taddr' that was being done in v1.
- v2 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg05591.html

Daniel Henrique Barboza (2):
  ppc/pnv: use a do-while() loop in pnv_phb3_translate_tve()
  ppc/pnv: use a do-while() loop in pnv_phb4_translate_tve()

 hw/pci-host/pnv_phb3.c | 6 ++++--
 hw/pci-host/pnv_phb4.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] ppc/pnv: use a do-while() loop in pnv_phb3_translate_tve()

[Daniel Henrique Barboza]

The 'taddr' variable is left unintialized, being set only inside the
"while ((lev--) >= 0)" loop where we get the TCE address. The 'lev' var
is an int32_t that is being initiliazed by the GETFIELD() macro, which
returns an uint64_t.

For a human reader this means that 'lev' will always be positive or zero.
But some compilers may beg to differ. 'lev' being an int32_t can in theory
be set as negative, and the "while ((lev--) >= 0)" loop might never be
reached, and 'taddr' will be left unitialized. This can cause phb3_error()
to use 'taddr' uninitialized down below:

if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) {
    phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr);

A quick way of fixing it is to use a do/while() loop. This will keep the
same semanting as the existing while() loop does and the compiler will
understand that 'taddr' will be initialized at least once.

Suggested-by: Matheus K. Ferst <matheus.ferst@eldorado.org.br>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/573
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb3.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 7fb35dc031..466b834f0f 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -792,7 +792,9 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr,
         sh = tbl_shift * lev + tce_shift;
 
         /* TODO: Multi-level untested */
-        while ((lev--) >= 0) {
+        do {
+            lev--;
+
             /* Grab the TCE address */
             taddr = base | (((addr >> sh) & ((1ul << tbl_shift) - 1)) << 3);
             if (dma_memory_read(&address_space_memory, taddr, &tce,
@@ -813,7 +815,7 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr,
             }
             sh -= tbl_shift;
             base = tce & ~0xfffull;
-        }
+        } while (lev >= 0);
 
         /* We exit the loop with TCE being the final TCE */
         tce_mask = ~((1ull << tce_shift) - 1);
-- 
2.34.1

[PATCH v3 2/2] ppc/pnv: use a do-while() loop in pnv_phb4_translate_tve()

[Daniel Henrique Barboza]

pnv_phb4_translate_tve() is quite similar to pnv_phb3_translate_tve(),
and that includes the fact that 'taddr' can be considered uninitialized
when throwing the "TCE access fault" error because, in theory, the loop
that sets 'taddr' can be skippable due to 'lev' being an signed int.

No one complained about this specific case yet, but since we took the
time to handle the same situtation in pnv_phb3_translate_tve(), let's
replicate it here as well.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index a78add75b0..b6c74553fa 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1267,7 +1267,9 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
         /* TODO: Limit to support IO page sizes */
 
         /* TODO: Multi-level untested */
-        while ((lev--) >= 0) {
+        do {
+            lev--;
+
             /* Grab the TCE address */
             taddr = base | (((addr >> sh) & ((1ul << tbl_shift) - 1)) << 3);
             if (dma_memory_read(&address_space_memory, taddr, &tce,
@@ -1288,7 +1290,7 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
             }
             sh -= tbl_shift;
             base = tce & ~0xfffull;
-        }
+        } while (lev >= 0);
 
         /* We exit the loop with TCE being the final TCE */
         tce_mask = ~((1ull << tce_shift) - 1);
-- 
2.34.1

Re: [PATCH v3 2/2] ppc/pnv: use a do-while() loop in pnv_phb4_translate_tve()

[Matheus K. Ferst]

On 27/01/2022 09:22, Daniel Henrique Barboza wrote:
> pnv_phb4_translate_tve() is quite similar to pnv_phb3_translate_tve(),
> and that includes the fact that 'taddr' can be considered uninitialized
> when throwing the "TCE access fault" error because, in theory, the loop
> that sets 'taddr' can be skippable due to 'lev' being an signed int.
> 
> No one complained about this specific case yet, but since we took the
> time to handle the same situtation in pnv_phb3_translate_tve(), let's
> replicate it here as well.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v3 1/2] ppc/pnv: use a do-while() loop in pnv_phb3_translate_tve()

[Matheus K. Ferst]

On 27/01/2022 09:22, Daniel Henrique Barboza wrote:
> The 'taddr' variable is left unintialized, being set only inside the
> "while ((lev--) >= 0)" loop where we get the TCE address. The 'lev' var
> is an int32_t that is being initiliazed by the GETFIELD() macro, which
> returns an uint64_t.
> 
> For a human reader this means that 'lev' will always be positive or zero.
> But some compilers may beg to differ. 'lev' being an int32_t can in theory
> be set as negative, and the "while ((lev--) >= 0)" loop might never be
> reached, and 'taddr' will be left unitialized. This can cause phb3_error()
> to use 'taddr' uninitialized down below:
> 
> if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) {
>      phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr);
> 
> A quick way of fixing it is to use a do/while() loop. This will keep the
> same semanting as the existing while() loop does and the compiler will
> understand that 'taddr' will be initialized at least once.
> 
> Suggested-by: Matheus K. Ferst <matheus.ferst@eldorado.org.br>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/573
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb3.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>