Re: Runtime Memory Validation in Intel-TDX and AMD-SNP

From: Erdem Aktas <erdemaktas@google.com>
To: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: Joerg Roedel <jroedel@suse.de>, Andi Kleen <ak@linux.intel.com>,
	 David Rientjes <rientjes@google.com>,
	Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
	 Sean Christopherson <seanjc@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	 Vlastimil Babka <vbabka@suse.cz>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	 Brijesh Singh <brijesh.singh@amd.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	 Jon Grimm <jon.grimm@amd.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	 Peter Zijlstra <peterz@infradead.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	 Ingo Molnar <mingo@redhat.com>,
	"Kaplan, David" <David.Kaplan@amd.com>,
	 Varad Gautam <varad.gautam@suse.com>,
	Dario Faggioli <dfaggioli@suse.com>, x86 <x86@kernel.org>,
	 linux-mm@kvack.org, linux-coco@lists.linux.dev
Subject: Re: Runtime Memory Validation in Intel-TDX and AMD-SNP
Date: Mon, 26 Jul 2021 16:02:56 -0700	[thread overview]
Message-ID: <CAAYXXYxS2mK=AkqZDzG=k3=W4ChbYnwzo-hMLU8d6iusDpqXbQ@mail.gmail.com> (raw)
In-Reply-To: <20210722195130.beazbb5blvj3mruo@box>

On Thu, Jul 22, 2021 at 12:51 PM Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> +void mark_unaccepted(phys_addr_t start, phys_addr_t end)
> +{
> +       unsigned int npages;
> +
> +       if (start & ~PMD_MASK) {
> +               npages = (round_up(start, PMD_SIZE) - start) / PAGE_SIZE;
> +               tdx_hcall_gpa_intent(start, npages, TDX_MAP_PRIVATE);
> +               start = round_up(start, PMD_SIZE);
> +       }
> +
> +       if (end & ~PMD_MASK) {
> +               npages = (end - round_down(end, PMD_SIZE)) / PAGE_SIZE;
> +               end = round_down(end, PMD_SIZE);
> +               tdx_hcall_gpa_intent(end, npages, TDX_MAP_PRIVATE);
> +       }

Is not the above code will accept the pages that are already accepted?
It is accepting the pages in the same 2MB region that is before start
and after end. We do not know what code/data is stored on those pages,
right? This might cause security issues depending on what is stored on
those pages.

> +static void __accept_pages(phys_addr_t start, phys_addr_t end)
> +{
> +       unsigned int rs, re;
> +
> +       bitmap_for_each_set_region(unaccepted_memory, rs, re,
> +                                  start / PMD_SIZE, end / PMD_SIZE) {
> +               tdx_hcall_gpa_intent(rs * PMD_SIZE, (re - rs) * PMD_NR,
> +                                    TDX_MAP_PRIVATE);
> +
This assumes that the granularity of the unaccepted pages is always in
PMD_SIZE. I  have seen the answer above saying that mark_unaccepted
makes sure that we have only 2MB unaccepted pages in our bitmap but it
is not enough IMO. This function, as it is, will do double TDACCEPT
for the already accepted 4KB pages in the same 2MB region.

> +void maybe_set_page_offline(struct page *page, unsigned int order)
> +{
> +       phys_addr_t addr = page_to_phys(page);
> +       bool unaccepted = true;
> +       unsigned int i;
> +
> +       spin_lock(&unaccepted_memory_lock);
> +       if (order < PMD_ORDER) {
> +               BUG_ON(test_bit(addr / PMD_SIZE, unaccepted_memory));
> +               goto out;
> +       }
don't we need to throw a bug when order is < PMD_ORDER, independent of
what test_bit() is saying? If the page is accepted or not accepted,
there is a possibility of double accepting pages.
> +       for (i = 0; i < (1 << (order - PMD_ORDER)); i++) {

and if order < PMD_ORDER, this will be a wrong shift operation, right?

> +       if (unaccepted)
> +               __SetPageOffline(page);
> +       else
> +               __accept_pages(addr, addr + (PAGE_SIZE << order));
so all the pages that were accepted will be reaccepted?