On Mon, Jul 26, 2021 at 04:02:56PM -0700, Erdem Aktas wrote:
> On Thu, Jul 22, 2021 at 12:51 PM Kirill A. Shutemov
> <kirill@xxxxxxxxxxxxx> 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?
No. This code will get called for all UNACCEPTED ranges in EFI table.
If such memory is accepted it is a bug.
> 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.
As I told above, it only get called for unaccepted memory and nothing can
be stored there before the point.
> > +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.
Yes, because we constructed the bitmap this way. Non-2M-aligned chunks get
accepted when we accept upfront when we populate the bitmap.
See mark_unaccepted().
(mark_unaccepted() has few bugs that will be fixed in the next version)
> 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.
No. maybe_set_page_offline() get called on all pages that get added to the
free list on boot. Any pages with order < 9 has to belong to already
accepted regions.
> > + for (i = 0; i < (1 << (order - PMD_ORDER)); i++) {
>
> and if order < PMD_ORDER, this will be a wrong shift operation, right?
order < PMD_ORDER handed by the 'if' above.
> > + if (unaccepted)
> > + __SetPageOffline(page);
> > + else
> > + __accept_pages(addr, addr + (PAGE_SIZE << order));
> so all the pages that were accepted will be reaccepted?
Have you looked at what __accept_pages() does? It only accept unaccepted
pages, according to the bitmap.
--
Kirill A. Shutemov
