From 50c8e11ac0672b726a2b3e2217cb32dc8416299f Mon Sep 17 00:00:00 2001 From: Frederic Barrat Date: Fri, 28 Jan 2022 13:15:02 +0100 Subject: ppc/pnv: Fail DMA access if page permissions are not correct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If an iommu page has wrong permissions, an error message is displayed, but the access is allowed, which is odd. This patch fixes it. Signed-off-by: Frederic Barrat Reviewed-by: Daniel Henrique Barboza Message-Id: <20220121152350.381685-1-fbarrat@linux.ibm.com> Signed-off-by: Cédric Le Goater --- hw/pci-host/pnv_phb3.c | 11 ++++++----- hw/pci-host/pnv_phb4.c | 11 ++++++----- 2 files changed, 12 insertions(+), 10 deletions(-) (limited to 'hw/pci-host') diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index 7fb35dc031..a757f1a58e 100644 --- a/hw/pci-host/pnv_phb3.c +++ b/hw/pci-host/pnv_phb3.c @@ -816,18 +816,19 @@ static void pnv_phb3_translate_tve(PnvPhb3DMASpace *ds, hwaddr addr, } /* We exit the loop with TCE being the final TCE */ - tce_mask = ~((1ull << tce_shift) - 1); - tlb->iova = addr & tce_mask; - tlb->translated_addr = tce & tce_mask; - tlb->addr_mask = ~tce_mask; - tlb->perm = tce & 3; if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { phb3_error(phb, "TCE access fault at 0x%"PRIx64, taddr); phb3_error(phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, is_write ? 'W' : 'R', tve); phb3_error(phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", tta, lev, tts, tps); + return; } + tce_mask = ~((1ull << tce_shift) - 1); + tlb->iova = addr & tce_mask; + tlb->translated_addr = tce & tce_mask; + tlb->addr_mask = ~tce_mask; + tlb->perm = tce & 3; } } diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index a78add75b0..ee56377c02 100644 --- a/hw/pci-host/pnv_phb4.c +++ b/hw/pci-host/pnv_phb4.c @@ -1291,18 +1291,19 @@ static void pnv_phb4_translate_tve(PnvPhb4DMASpace *ds, hwaddr addr, } /* We exit the loop with TCE being the final TCE */ - tce_mask = ~((1ull << tce_shift) - 1); - tlb->iova = addr & tce_mask; - tlb->translated_addr = tce & tce_mask; - tlb->addr_mask = ~tce_mask; - tlb->perm = tce & 3; if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { phb_error(ds->phb, "TCE access fault at 0x%"PRIx64, taddr); phb_error(ds->phb, " xlate %"PRIx64":%c TVE=%"PRIx64, addr, is_write ? 'W' : 'R', tve); phb_error(ds->phb, " tta=%"PRIx64" lev=%d tts=%d tps=%d", tta, lev, tts, tps); + return; } + tce_mask = ~((1ull << tce_shift) - 1); + tlb->iova = addr & tce_mask; + tlb->translated_addr = tce & tce_mask; + tlb->addr_mask = ~tce_mask; + tlb->perm = tce & 3; } } -- cgit 1.4.1 From 83d2bea68a778b98ecbf9472be6f1ed8031719ac Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Fri, 28 Jan 2022 13:15:02 +0100 Subject: ppc/pnv: use a do-while() loop in pnv_phb3_translate_tve() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/573 Signed-off-by: Daniel Henrique Barboza Message-Id: <20220127122234.842145-2-danielhb413@gmail.com> Signed-off-by: Cédric Le Goater --- hw/pci-host/pnv_phb3.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'hw/pci-host') diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c index a757f1a58e..aafd46b635 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 */ if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { -- cgit 1.4.1 From 799c179ed844b01542e729405c0bf5ef4cd294dc Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Fri, 28 Jan 2022 13:15:02 +0100 Subject: ppc/pnv: use a do-while() loop in pnv_phb4_translate_tve() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Matheus Ferst Message-Id: <20220127122234.842145-3-danielhb413@gmail.com> Signed-off-by: Cédric Le Goater --- hw/pci-host/pnv_phb4.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'hw/pci-host') diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c index ee56377c02..e91249ef64 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 */ if ((is_write & !(tce & 2)) || ((!is_write) && !(tce & 1))) { -- cgit 1.4.1