[PATCH 0/2] initialize 'taddr' in pnv_phbX_translate_tve()

[PATCH 0/2] initialize 'taddr' in pnv_phbX_translate_tve()

[Daniel Henrique Barboza]

Hi,

First patch aims to fix https://gitlab.com/qemu-project/qemu/-/issues/573.
Second patch is the same fix applied to pnv_phb4 code that presents the
same potential issue.


Daniel Henrique Barboza (2):
  ppc/pnv: initialize 'taddr' in pnv_phb3_translate_tve()
  ppc/pnv: initialize 'taddr' in pnv_phb4_translate_tve()

 hw/pci-host/pnv_phb3.c | 11 +++++++++++
 hw/pci-host/pnv_phb4.c |  8 ++++++++
 2 files changed, 19 insertions(+)

-- 
2.34.1

[PATCH 1/2] ppc/pnv: initialize 'taddr' 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);

Setting 'taddr' to the top level base address will make compilers happy.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/573
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb3.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 7fb35dc031..617d42c5a0 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -788,6 +788,17 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr,
         /* Top level table base address */
         base = tta << 12;
 
+        /*
+         * Some compilers will complain that the "TCE access fault"
+         * phb3_error() down below will use 'taddr' uninitialized
+         * because, in theory, the loop that sets 'taddr' is skippable
+         * due to 'lev' being an signed int.
+         *
+         * Setting 'taddr 'to the base address will bring piece of mind
+         * to such compilers.
+         */
+        taddr = base;
+
         /* Total shift to first level */
         sh = tbl_shift * lev + tce_shift;
 
-- 
2.34.1

Re: [PATCH 1/2] ppc/pnv: initialize 'taddr' in pnv_phb3_translate_tve()

[Matheus K. Ferst]

On 26/01/2022 10:41, 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.

If we expect this code to execute at least once, wouldn't it be better 
to use a do-while? E.g.:

do {
     lev--;

     /* Grab the TCE address */
     taddr = base | (((addr >> sh) & ((1ul << tbl_shift) - 1)) << 3);
     if (dma_memory_read(&address_space_memory, taddr, &tce,
     /* ... */
     }
     sh -= tbl_shift;
     base = tce & ~0xfffull;
} while (lev >= 0);

Otherwise, I think we'll need to initialize tce too.

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 1/2] ppc/pnv: initialize 'taddr' in pnv_phb3_translate_tve()

[Daniel Henrique Barboza]

On 1/26/22 14:28, Matheus K. Ferst wrote:
> On 26/01/2022 10:41, 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.
> 
> If we expect this code to execute at least once, wouldn't it be better to use a do-while? E.g.:
> 
> do {
>      lev--;
> 
>      /* Grab the TCE address */
>      taddr = base | (((addr >> sh) & ((1ul << tbl_shift) - 1)) << 3);
>      if (dma_memory_read(&address_space_memory, taddr, &tce,
>      /* ... */
>      }
>      sh -= tbl_shift;
>      base = tce & ~0xfffull;
> } while (lev >= 0);
> 
> Otherwise, I think we'll need to initialize tce too.

Initializing tce isn't necessary, at least as far as compiler warning goes,
because tce will be defaulted to zero and its current use (tce & 3, tce & 2,
tce & 1 operations) isn't offending the compiler. For now at least.

That said, I think using a do/while() loop is an idea that fixes the issue while
keeping the code flow, without having to add extra initializations, so I ended
up changing it as you suggested.


Thanks,


Daniel

> 
> 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>

[PATCH 2/2] ppc/pnv: initialize 'taddr' 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index a78add75b0..f5e32e856b 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1261,6 +1261,14 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr,
         /* Top level table base address */
         base = tta << 12;
 
+        /*
+         * There were reports of compilers complaining about 'taddr'
+         * being used uninitialized in pnv_phb3_translate_tve(), and
+         * the same scenario is happening here. Initialize 'taddr'
+         * just in case.
+         */
+        taddr = base;
+
         /* Total shift to first level */
         sh = tbl_shift * lev + tce_shift;
 
-- 
2.34.1