| Message ID | 83f510bd-9a9c-3063-27dd-d894b1e8031c@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: follow-on to XSA-408 | expand |
On 06.12.2022 14:53, Jan Beulich wrote: > While plausible to do what was intended based on the name of the flag > (PGC_page_table), that name was misleading and is going to be changed. > It marks page tables pages _having_ a shadow, not shadows of page table > pages. The attempt also didn't cover the HAP case at all, and it > constituted a potentially very long loop doing nothing when > !SHADOW_PAGING. Instead leave a comment of what actually wants doing > there (which then also may need to account for e.g. the risk of A/D bits > becoming set behind our backs). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I'm sorry, I should have Cc-ed tboot reviewers here as well. Jan > --- a/xen/arch/x86/tboot.c > +++ b/xen/arch/x86/tboot.c > @@ -177,29 +177,6 @@ static void update_iommu_mac(vmac_ctx_t > #define is_page_in_use(page) \ > (page_state_is(page, inuse) || page_state_is(page, offlining)) > > -static void update_pagetable_mac(vmac_ctx_t *ctx) > -{ > - unsigned long mfn; > - > - for ( mfn = 0; mfn < max_page; mfn++ ) > - { > - struct page_info *page = mfn_to_page(_mfn(mfn)); > - > - if ( !mfn_valid(_mfn(mfn)) ) > - continue; > - if ( is_page_in_use(page) && !is_special_page(page) ) > - { > - if ( page->count_info & PGC_page_table ) > - { > - void *pg = map_domain_page(_mfn(mfn)); > - > - vmac_update(pg, PAGE_SIZE, ctx); > - unmap_domain_page(pg); > - } > - } > - } > -} > - > static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE], > vmac_t *mac) > { > @@ -233,8 +210,7 @@ static void tboot_gen_domain_integrity(c > } > } > > - /* MAC all shadow page tables */ > - update_pagetable_mac(&ctx); > + /* TODO: MAC all shadow / HAP page tables */ > > *mac = vmac(NULL, 0, nonce, NULL, &ctx); > > >
On Tue, Dec 6, 2022 at 8:58 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 06.12.2022 14:53, Jan Beulich wrote: > > While plausible to do what was intended based on the name of the flag > > (PGC_page_table), that name was misleading and is going to be changed. > > It marks page tables pages _having_ a shadow, not shadows of page table > > pages. The attempt also didn't cover the HAP case at all, and it > > constituted a potentially very long loop doing nothing when > > !SHADOW_PAGING. Instead leave a comment of what actually wants doing > > there (which then also may need to account for e.g. the risk of A/D bits > > becoming set behind our backs). > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
--- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c @@ -177,29 +177,6 @@ static void update_iommu_mac(vmac_ctx_t #define is_page_in_use(page) \ (page_state_is(page, inuse) || page_state_is(page, offlining)) -static void update_pagetable_mac(vmac_ctx_t *ctx) -{ - unsigned long mfn; - - for ( mfn = 0; mfn < max_page; mfn++ ) - { - struct page_info *page = mfn_to_page(_mfn(mfn)); - - if ( !mfn_valid(_mfn(mfn)) ) - continue; - if ( is_page_in_use(page) && !is_special_page(page) ) - { - if ( page->count_info & PGC_page_table ) - { - void *pg = map_domain_page(_mfn(mfn)); - - vmac_update(pg, PAGE_SIZE, ctx); - unmap_domain_page(pg); - } - } - } -} - static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE], vmac_t *mac) { @@ -233,8 +210,7 @@ static void tboot_gen_domain_integrity(c } } - /* MAC all shadow page tables */ - update_pagetable_mac(&ctx); + /* TODO: MAC all shadow / HAP page tables */ *mac = vmac(NULL, 0, nonce, NULL, &ctx);
While plausible to do what was intended based on the name of the flag (PGC_page_table), that name was misleading and is going to be changed. It marks page tables pages _having_ a shadow, not shadows of page table pages. The attempt also didn't cover the HAP case at all, and it constituted a potentially very long loop doing nothing when !SHADOW_PAGING. Instead leave a comment of what actually wants doing there (which then also may need to account for e.g. the risk of A/D bits becoming set behind our backs). Signed-off-by: Jan Beulich <jbeulich@suse.com>