Fix the condition part of the for loop in p2m_invalidate_root() that
uses P2M_ROOT_LEVEL instead of P2M_ROOT_PAGES. The goal here is to
invalidate all root page tables (that can be concatenated), so the loop
must iterate through all these pages. Root level can be 0 or 1, whereas
there can be 1,2,8,16 root pages. The issue may lead to some pages
not being invalidated and therefore the guest access won't be trapped.
We use it to track pages accessed by guest for set/way emulation provided
no IOMMU, IOMMU not enabled for the domain or P2M not shared with IOMMU.

Fixes: 2148a125b73b ("xen/arm: Track page accessed between batch of Set/Way operations")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/mmu/p2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
index d96078f547d5..67296dabb587 100644
--- a/xen/arch/arm/mmu/p2m.c
+++ b/xen/arch/arm/mmu/p2m.c
@@ -1291,7 +1291,7 @@ static void p2m_invalidate_root(struct p2m_domain *p2m)
 
     p2m_write_lock(p2m);
 
-    for ( i = 0; i < P2M_ROOT_LEVEL; i++ )
+    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
         p2m_invalidate_table(p2m, page_to_mfn(p2m->root + i));
 
     p2m_write_unlock(p2m);
-- 
2.25.1

(+Oleksii)

Hi,

Adding Oleksii for visibility.

On 16/06/2025 07:56, Michal Orzel wrote:
> Fix the condition part of the for loop in p2m_invalidate_root() that
> uses P2M_ROOT_LEVEL instead of P2M_ROOT_PAGES. The goal here is to
> invalidate all root page tables (that can be concatenated), so the loop
> must iterate through all these pages. Root level can be 0 or 1, whereas
> there can be 1,2,8,16 root pages. The issue may lead to some pages
> not being invalidated and therefore the guest access won't be trapped.
> We use it to track pages accessed by guest for set/way emulation provided
> no IOMMU, IOMMU not enabled for the domain or P2M not shared with IOMMU.
> 
> Fixes: 2148a125b73b ("xen/arm: Track page accessed between batch of Set/Way operations")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

On 6/16/25 10:15 AM, Julien Grall wrote:
> (+Oleksii)
>
> Hi,
>
> Adding Oleksii for visibility.

Thanks for adding me.

>
> On 16/06/2025 07:56, Michal Orzel wrote:
>> Fix the condition part of the for loop in p2m_invalidate_root() that
>> uses P2M_ROOT_LEVEL instead of P2M_ROOT_PAGES. The goal here is to

I used P2M_ROOT_PAGES for RISC-V for the similar case. This patch just confirms
that it was a correct decision.

Thanks.

~ Oleksii

>> invalidate all root page tables (that can be concatenated), so the loop
>> must iterate through all these pages. Root level can be 0 or 1, whereas
>> there can be 1,2,8,16 root pages. The issue may lead to some pages
>> not being invalidated and therefore the guest access won't be trapped.
>> We use it to track pages accessed by guest for set/way emulation 
>> provided
>> no IOMMU, IOMMU not enabled for the domain or P2M not shared with IOMMU.
>>
>> Fixes: 2148a125b73b ("xen/arm: Track page accessed between batch of 
>> Set/Way operations")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>
> Reviewed-by: Julien Grall <jgrall@amazon.com>
>
> Cheers,
>

On 16.06.2025 08:56, Michal Orzel wrote:
> Fix the condition part of the for loop in p2m_invalidate_root() that
> uses P2M_ROOT_LEVEL instead of P2M_ROOT_PAGES. The goal here is to
> invalidate all root page tables (that can be concatenated), so the loop
> must iterate through all these pages. Root level can be 0 or 1, whereas
> there can be 1,2,8,16 root pages. The issue may lead to some pages
> not being invalidated and therefore the guest access won't be trapped.
> We use it to track pages accessed by guest for set/way emulation provided
> no IOMMU, IOMMU not enabled for the domain or P2M not shared with IOMMU.

IOW no security concerns?

> Fixes: 2148a125b73b ("xen/arm: Track page accessed between batch of Set/Way operations")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Credit Oleksii with a Reported-by?

Jan

Hi Jan,

On 16/06/2025 08:35, Jan Beulich wrote:
> On 16.06.2025 08:56, Michal Orzel wrote:
>> Fix the condition part of the for loop in p2m_invalidate_root() that
>> uses P2M_ROOT_LEVEL instead of P2M_ROOT_PAGES. The goal here is to
>> invalidate all root page tables (that can be concatenated), so the loop
>> must iterate through all these pages. Root level can be 0 or 1, whereas
>> there can be 1,2,8,16 root pages. The issue may lead to some pages
>> not being invalidated and therefore the guest access won't be trapped.
>> We use it to track pages accessed by guest for set/way emulation provided
>> no IOMMU, IOMMU not enabled for the domain or P2M not shared with IOMMU.
> 
> IOW no security concerns?

Copying/pasting what I wrote on the security channel for the record. 
(This was sent after you asked on xen-devel, sorry I should have done it 
before hand):

We both looked at the code and concluded that it is guarantreed that 
P2M_ROOT_PAGES >= P2M_ROOT_LEVEL. This means the only issue is an under 
invalidation. The logic is only used for the benefit of invalidating the 
guest memory when using cache flush by set/way. Because of the issue, 
the guest we may not clean & invalidate some RAM belonging to itself. We 
also don't rely on the p2m_invalidate_root() to ensure any scrubbed 
pages content have reached memory. So any under invalidation will only 
impact the guest.

Hence why we concluded it wasn't a security issue.

Cheers,

-- 
Julien Grall

On 16/06/2025 09:35, Jan Beulich wrote:
> On 16.06.2025 08:56, Michal Orzel wrote:
>> Fix the condition part of the for loop in p2m_invalidate_root() that
>> uses P2M_ROOT_LEVEL instead of P2M_ROOT_PAGES. The goal here is to
>> invalidate all root page tables (that can be concatenated), so the loop
>> must iterate through all these pages. Root level can be 0 or 1, whereas
>> there can be 1,2,8,16 root pages. The issue may lead to some pages
>> not being invalidated and therefore the guest access won't be trapped.
>> We use it to track pages accessed by guest for set/way emulation provided
>> no IOMMU, IOMMU not enabled for the domain or P2M not shared with IOMMU.
> 
> IOW no security concerns?
I discussed this with Julien and we don't think there are any.

> 
>> Fixes: 2148a125b73b ("xen/arm: Track page accessed between batch of Set/Way operations")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> 
> Credit Oleksii with a Reported-by?
Sure thing:
Reported-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~Michal