[PATCH] ppc/pnv: Fail DMA access if page permissions are not correct

Frederic Barrat posted 1 patch 2 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220121152350.381685-1-fbarrat@linux.ibm.com
Maintainers: "Cédric Le Goater" <clg@kaod.org>
hw/pci-host/pnv_phb3.c | 11 ++++++-----
hw/pci-host/pnv_phb4.c | 11 ++++++-----
2 files changed, 12 insertions(+), 10 deletions(-)
[PATCH] ppc/pnv: Fail DMA access if page permissions are not correct
Posted by Frederic Barrat 2 years, 2 months ago
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 <fbarrat@linux.ibm.com>
---
 hw/pci-host/pnv_phb3.c | 11 ++++++-----
 hw/pci-host/pnv_phb4.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

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;
     }
 }
 
-- 
2.34.1


Re: [PATCH] ppc/pnv: Fail DMA access if page permissions are not correct
Posted by Daniel Henrique Barboza 2 years, 2 months ago

On 1/21/22 12:23, Frederic Barrat wrote:
> 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 <fbarrat@linux.ibm.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/pci-host/pnv_phb3.c | 11 ++++++-----
>   hw/pci-host/pnv_phb4.c | 11 ++++++-----
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> 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;
>       }
>   }
>   

Re: [PATCH] ppc/pnv: Fail DMA access if page permissions are not correct
Posted by Cédric Le Goater 2 years, 2 months ago
On 1/21/22 16:23, Frederic Barrat wrote:
> If an iommu page has wrong permissions, an error message is displayed,
> but the access is allowed, which is odd. This patch fixes it.


Being curious. How do you generate such errors ? could we have the
output ?

Thanks,

C.


> 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>   hw/pci-host/pnv_phb3.c | 11 ++++++-----
>   hw/pci-host/pnv_phb4.c | 11 ++++++-----
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> 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;
>       }
>   }
>   
> 


Re: [PATCH] ppc/pnv: Fail DMA access if page permissions are not correct
Posted by Frederic Barrat 2 years, 2 months ago

On 21/01/2022 18:35, Cédric Le Goater wrote:
> On 1/21/22 16:23, Frederic Barrat wrote:
>> If an iommu page has wrong permissions, an error message is displayed,
>> but the access is allowed, which is odd. This patch fixes it.
> 
> 
> Being curious. How do you generate such errors ? could we have the
> output ?


By acking the code building the tce table to remove permissions on the 
page. See pnv_tce_build().
Also, on bare metal, device drivers using 64-bit dma will hit a 
translation entry (TVE#1) configured in bypass mode. So to avoid that 
and force translation, you need to add "iommu=nobypass" to the kernel 
boot args.

Then you will see:
phb4[0:1]: TCE access fault at 0x2a510080
phb4[0:1]:  xlate 100000:R TVE=32c702505
phb4[0:1]:  tta=32c70 lev=-2 tts=5 tps=5
Invalid read at addr 0x100000, size 4, region '(null)', reason: rejected

P8 is more complicated...

   Fred



> 
> Thanks,
> 
> C.
> 
> 
>>
>> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>>   hw/pci-host/pnv_phb3.c | 11 ++++++-----
>>   hw/pci-host/pnv_phb4.c | 11 ++++++-----
>>   2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> 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;
>>       }
>>   }
>>
>