linux-kernel - Re: [PATCH] x86/mm/cpa: Warn if set_memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fd2426ee900e4f3a57659f3688c5bb652779785c.camel@intel.com>
Date:   Thu, 26 Oct 2023 01:40:38 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "Lutomirski, Andy" <luto@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "Reshetova, Elena" <elena.reshetova@...el.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Cui, Dexuan" <decui@...rosoft.com>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "mikelley@...rosoft.com" <mikelley@...rosoft.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "bp@...en8.de" <bp@...en8.de>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH] x86/mm/cpa: Warn if set_memory_XXcrypted() fails

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.