[PATCH] x86/mm: PGC_page_table is used by shadow code only

Jan Beulich posted 1 patch 1 year, 4 months ago
Failed in applying to current master (apply log)
[PATCH] x86/mm: PGC_page_table is used by shadow code only
Posted by Jan Beulich 1 year, 4 months ago
By defining the constant to zero when !SHADOW_PAGING we give compilers
the chance to eliminate a little more dead code elsewhere in the tree.
Plus, as a minor benefit, the general reference count can be one bit
wider. (To simplify things, have PGC_page_table change places with
PGC_extra.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
tboot.c's update_pagetable_mac() is suspicious: It effectively is a
no-op even prior to this change when !SHADOW_PAGING, which can't be
quite right. If (guest) page tables are relevant to include in the
verification, shouldn't this look for PGT_l<N>_page_table as well? How
to deal with HAP guests there is entirely unclear.

--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -70,9 +70,9 @@
  /* Page is Xen heap? */
 #define _PGC_xen_heap     PG_shift(2)
 #define PGC_xen_heap      PG_mask(1, 2)
- /* Set when is using a page as a page table */
-#define _PGC_page_table   PG_shift(3)
-#define PGC_page_table    PG_mask(1, 3)
+ /* Page is not reference counted */
+#define _PGC_extra        PG_shift(3)
+#define PGC_extra         PG_mask(1, 3)
  /* Page is broken? */
 #define _PGC_broken       PG_shift(4)
 #define PGC_broken        PG_mask(1, 4)
@@ -83,12 +83,20 @@
 #define PGC_state_offlined  PG_mask(2, 6)
 #define PGC_state_free      PG_mask(3, 6)
 #define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
-/* Page is not reference counted */
-#define _PGC_extra        PG_shift(7)
-#define PGC_extra         PG_mask(1, 7)
+#ifdef CONFIG_SHADOW_PAGING
+ /* Set when is using a page as a page table */
+#define _PGC_page_table   PG_shift(7)
+#define PGC_page_table    PG_mask(1, 7)
+#else
+#define PGC_page_table    0
+#endif
 
 /* Count of references to this frame. */
+#if PGC_page_table
 #define PGC_count_width   PG_shift(7)
+#else
+#define PGC_count_width   PG_shift(6)
+#endif
 #define PGC_count_mask    ((1UL<<PGC_count_width)-1)
 
 /*
Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only
Posted by Andrew Cooper 1 year, 4 months ago
On 29/11/2022 14:55, Jan Beulich wrote:
> By defining the constant to zero when !SHADOW_PAGING we give compilers
> the chance to eliminate a little more dead code elsewhere in the tree.
> Plus, as a minor benefit, the general reference count can be one bit
> wider. (To simplify things, have PGC_page_table change places with
> PGC_extra.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ahead of making this change, can we please rename it to something less
confusing, and fix it's comment which is wrong.

PGC_shadowed_pt is the best I can think of.

> ---
> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
> no-op even prior to this change when !SHADOW_PAGING, which can't be
> quite right. If (guest) page tables are relevant to include in the
> verification, shouldn't this look for PGT_l<N>_page_table as well? How
> to deal with HAP guests there is entirely unclear.

Considering the caller, it MACs every domheap page for domains with
CDF_s3_integrity.

The tboot logical also blindly assumes that any non-idle domain has an
Intel IOMMU context with it.  This only doesn't (trivially) expose
because struct domain_iommu is embedded in struct domain (rather than
allocated separately), and reaching into the wrong part of the arch
union is only mitigated by the tboot logic not being invoked on
non-intel systems.  (Also the idle domain check is useless, given that
it's in a for_each_domain() loop).

It does look a little like the caller is wanting to MAC all Xen data
that describes the guest, but doing this unilaterally for all shadowed
guests seems wrong beside the per-domain s3_integrity setting.

~Andrew
Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only
Posted by Jan Beulich 1 year, 3 months ago
On 29.11.2022 21:56, Andrew Cooper wrote:
> On 29/11/2022 14:55, Jan Beulich wrote:
>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>> the chance to eliminate a little more dead code elsewhere in the tree.
>> Plus, as a minor benefit, the general reference count can be one bit
>> wider. (To simplify things, have PGC_page_table change places with
>> PGC_extra.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Ahead of making this change, can we please rename it to something less
> confusing, and fix it's comment which is wrong.
> 
> PGC_shadowed_pt is the best I can think of.

Can do, sure.

>> ---
>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>> quite right. If (guest) page tables are relevant to include in the
>> verification, shouldn't this look for PGT_l<N>_page_table as well? How
>> to deal with HAP guests there is entirely unclear.
> 
> Considering the caller, it MACs every domheap page for domains with
> CDF_s3_integrity.
> 
> The tboot logical also blindly assumes that any non-idle domain has an
> Intel IOMMU context with it.  This only doesn't (trivially) expose
> because struct domain_iommu is embedded in struct domain (rather than
> allocated separately), and reaching into the wrong part of the arch
> union is only mitigated by the tboot logic not being invoked on
> non-intel systems.  (Also the idle domain check is useless, given that
> it's in a for_each_domain() loop).
> 
> It does look a little like the caller is wanting to MAC all Xen data
> that describes the guest, but doing this unilaterally for all shadowed
> guests seems wrong beside the per-domain s3_integrity setting.

Question is - do we care about addressing this (when, as said, it's
unclear how to deal with HAP domains; maybe their actively used p2m
pages would need including instead)? Or should we rather consider
ripping out tboot support?

Jan

Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only
Posted by Daniel P. Smith 1 year, 3 months ago
On 11/30/22 02:52, Jan Beulich wrote:
> On 29.11.2022 21:56, Andrew Cooper wrote:
>> On 29/11/2022 14:55, Jan Beulich wrote:
>>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>>> the chance to eliminate a little more dead code elsewhere in the tree.
>>> Plus, as a minor benefit, the general reference count can be one bit
>>> wider. (To simplify things, have PGC_page_table change places with
>>> PGC_extra.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Ahead of making this change, can we please rename it to something less
>> confusing, and fix it's comment which is wrong.
>>
>> PGC_shadowed_pt is the best I can think of.
> 
> Can do, sure.
> 
>>> ---
>>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>>> quite right. If (guest) page tables are relevant to include in the
>>> verification, shouldn't this look for PGT_l<N>_page_table as well? How
>>> to deal with HAP guests there is entirely unclear.
>>
>> Considering the caller, it MACs every domheap page for domains with
>> CDF_s3_integrity.
>>
>> The tboot logical also blindly assumes that any non-idle domain has an
>> Intel IOMMU context with it.  This only doesn't (trivially) expose
>> because struct domain_iommu is embedded in struct domain (rather than
>> allocated separately), and reaching into the wrong part of the arch
>> union is only mitigated by the tboot logic not being invoked on
>> non-intel systems.  (Also the idle domain check is useless, given that
>> it's in a for_each_domain() loop).
>>
>> It does look a little like the caller is wanting to MAC all Xen data
>> that describes the guest, but doing this unilaterally for all shadowed
>> guests seems wrong beside the per-domain s3_integrity setting.
> 
> Question is - do we care about addressing this (when, as said, it's
> unclear how to deal with HAP domains; maybe their actively used p2m
> pages would need including instead)? Or should we rather consider
> ripping out tboot support?

This would break a significant number of production deployed OpenXT 
derivative solutions. I would respectively request that a middle ground 
be found that will allow the capability to remain until TrenchBoot has 
had time to build a Secure Launch for Xen that mirrors Secure Launch for 
Linux.

NB: I have a long list of changes for the tboot code but have opted thus 
far to let them lie. Mainly as they would be hole patching that would 
mostly be tossed with the clean room implementation that will come from TB.

v/r,
dps

Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only
Posted by Andrew Cooper 1 year, 3 months ago
On 30/11/2022 07:52, Jan Beulich wrote:On 29.11.2022 21:56, Andrew
Cooper wrote:
>> On 29/11/2022 14:55, Jan Beulich wrote:
>>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>>> the chance to eliminate a little more dead code elsewhere in the tree.
>>> Plus, as a minor benefit, the general reference count can be one bit
>>> wider. (To simplify things, have PGC_page_table change places with
>>> PGC_extra.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Ahead of making this change, can we please rename it to something less
>> confusing, and fix it's comment which is wrong.
>>
>> PGC_shadowed_pt is the best I can think of.
> Can do, sure.
>
>>> ---
>>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>>> quite right. If (guest) page tables are relevant to include in the
>>> verification, shouldn't this look for PGT_l<N>_page_table as well? How
>>> to deal with HAP guests there is entirely unclear.
>> Considering the caller, it MACs every domheap page for domains with
>> CDF_s3_integrity.
>>
>> The tboot logical also blindly assumes that any non-idle domain has an
>> Intel IOMMU context with it.  This only doesn't (trivially) expose
>> because struct domain_iommu is embedded in struct domain (rather than
>> allocated separately), and reaching into the wrong part of the arch
>> union is only mitigated by the tboot logic not being invoked on
>> non-intel systems.  (Also the idle domain check is useless, given that
>> it's in a for_each_domain() loop).
>>
>> It does look a little like the caller is wanting to MAC all Xen data
>> that describes the guest, but doing this unilaterally for all shadowed
>> guests seems wrong beside the per-domain s3_integrity setting.
> Question is - do we care about addressing this (when, as said, it's
> unclear how to deal with HAP domains; maybe their actively used p2m
> pages would need including instead)? Or should we rather consider
> ripping out tboot support?

Having contemplated this a bit longer...

The principle of MAC-ing pagetables is incompatible with A/D updates. 
IOMMUs have A/D support support these days, and older Intel CPUs have
errata where they can set the D bit on a read-only mapping.  The things
which are MACed can legally be changed by hardware after the MAC is taken.

The current logic is clearly not doing sensible things.  It likely
predates HAP support, but I haven't gone looking.

Also, tboot isn't long for this world.  Trenchboot is progressing
(slowly) but the end result will be something that functions, is
supported, and doesn't suffer from several CVEs which Intel have elected
not to fix in their "reference TXT implementation".

I've debated several times about removing the tboot implementation, but
have chosen not to do so thus far because there's almost certainly bits
of infrastructure that trenchboot will want to reuse.

But as far as this goes, I think we can reasonably remove the
clearly-junk aspects while cleaning up / fixing other areas of Xen.

~Andrew
Re: [PATCH] x86/mm: PGC_page_table is used by shadow code only
Posted by Andrew Cooper 1 year, 4 months ago
On 29/11/2022 20:56, Andrew Cooper wrote:
> On 29/11/2022 14:55, Jan Beulich wrote:
>> By defining the constant to zero when !SHADOW_PAGING we give compilers
>> the chance to eliminate a little more dead code elsewhere in the tree.
>> Plus, as a minor benefit, the general reference count can be one bit
>> wider. (To simplify things, have PGC_page_table change places with
>> PGC_extra.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Ahead of making this change, can we please rename it to something less
> confusing, and fix it's comment which is wrong.
>
> PGC_shadowed_pt is the best I can think of.
>
>> ---
>> tboot.c's update_pagetable_mac() is suspicious: It effectively is a
>> no-op even prior to this change when !SHADOW_PAGING, which can't be
>> quite right. If (guest) page tables are relevant to include in the
>> verification, shouldn't this look for PGT_l<N>_page_table as well? How
>> to deal with HAP guests there is entirely unclear.
> Considering the caller, it MACs every domheap page for domains with
> CDF_s3_integrity.
>
> The tboot logical also blindly assumes that any non-idle domain has an
> Intel IOMMU context with it.  This only doesn't (trivially) expose
> because struct domain_iommu is embedded in struct domain (rather than
> allocated separately), and reaching into the wrong part of the arch
> union is only mitigated by the tboot logic not being invoked on
> non-intel systems.  (Also the idle domain check is useless, given that
> it's in a for_each_domain() loop).

Wow I really failed at typing here.  "The tboot logic", and "doesn't
(trivially) explode".

~Andrew

>
> It does look a little like the caller is wanting to MAC all Xen data
> that describes the guest, but doing this unilaterally for all shadowed
> guests seems wrong beside the per-domain s3_integrity setting.
>
> ~Andrew