lkml.org 
[lkml]   [2023]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] x86/mm/cpa: Warn if set_memory_XXcrypted() fails
    Date
    On Thu, 2023-10-26 at 00:35 +0000, Michael Kelley (LINUX) wrote:
    > I think you mean "shared" as indicated by the guest page tables (vs.
    > "shared"
    > as the state of the page from the host standpoint).  Some precision
    > on
    > that distinction seems useful here and in follow-on patches to make
    > callers'
    > error handling be correct.   As I understand it, the premise is that
    > if the
    > guest is accessing a page as private, and the host/VMM has messed
    > around with the page private/shared status, the confidentiality of
    > the
    > VM is protected.  The risk of leakage occurs when the guest is
    > accessing
    > a page as shared, so kernel code must guard against putting memory
    > on the free list if the guest page tables are marked shared.
    >

    For TDX, the scenario of concern in the VMM error case is if the page
    is mapped as shared in the guest page tables *and* it is either also
    marked as shared in the EPT, or the VMM supports automatically
    converting it on access. In the attacker scenario, I think the problem
    is just that it is marked shared in the guest.

    I can clarify that it needs to be mapped shared in the guest for there
    to be a problem, but I don't see how it will help the patches to fix
    the callers. It seems like too many details for the callers to know
    about. For example, I think some architectures don't change the PTEs at
    all. The callers abstract shared and private at a higher level.


    > To me, this sentence doesn't fully characterize why panic_on_warn
    > would be used.  You describe one reason, which is a caller that fails
    > to
    > properly handle an error and incorrectly puts memory with a "shared"
    > guest PTE on the free list.  But getting an error back also implies
    > that
    > something unknown has gone wrong with the CoCo mechanism for
    > managing private vs. shared pages.  Security focused users would not
    > take the risk of continuing to operate with that kind of unknown
    > error
    > in the core mechanism of a CoCo VM.

    Hmm, yea I could see that some users may want to take a hard line and
    terminate if anything looks strange. The counter point is that the VMM
    is actually returning a legal error here. It may be strange based on
    the details of when HyperV and QEMU/KVM would return this error, but
    not architecturally.

    >
    > > +vmm_fail:
    > > +       WARN_ONCE(1, "CPA VMM failure to convert memory (addr=%p,
    > > numpages=%d) to %s.\n",
    > > +                 (void *)addr, numpages, enc ? "private" :
    > > "shared");
    >
    > I'm not sure about outputting the "addr" value.  It could be
    > useful, but the %p format specifier hashes the value unless the
    > kernel is booted with "no_hash_pointers".   Should %px be used
    > so the address is output unmodified?

    Unfortunately, I don't think we can print the kernel virtual address
    because those are supposed to be hidden for security reasons. Ideally,
    I would prefer to print the PFN, but we won't have it here in the case
    of vmalloc's. I thought it might be useful to still have some address
    printed for debugging purposes.

    >
    > > +
    > > +       return -EIO;
    > >   }
    > >
    > >   static int __set_memory_enc_dec(unsigned long addr, int numpages,
    > > bool enc)
    > > --
    > > 2.34.1
    >
    > My comments notwithstanding, I'm good with this overall change and
    > the additional level of protection it offers to CoCo VM users.

    Thanks.
    \
     
     \ /
      Last update: 2023-10-26 03:42    [W:2.848 / U:0.416 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site