lkml.org 
[lkml]   [2019]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active
    Date
    On 3/25/19 1:17 PM, Singh, Brijesh wrote:
    >
    >
    > On 3/25/19 12:32 PM, Borislav Petkov wrote:
    >> On Mon, Mar 25, 2019 at 05:17:55PM +0000, Singh, Brijesh wrote:
    >>> By default all the memory regions are mapped encrypted. The
    >>> set_memory_{encrypt,decrypt}() is a generic function which can be
    >>> called explicitly to clear/set the encryption mask from the existing
    >>> memory mapping. The mem_encrypt_active() returns true if either SEV or
    >>> SME is active. So the __set_memory_enc_dec() uses the
    >>> memory_encrypt_active() check to ensure that the function is no-op when
    >>> SME/SEV are not active.
    >>>
    >>> Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
    >>> encryption mask from the kexec area. In case of SEV, we should not clear
    >>> the encryption mask.
    >>
    >> Brijesh, I know all that.
    >>
    >> Please read what I said here at the end:
    >>
    >> https://lkml.kernel.org/r/20190324150034.GH23289@zn.tnic
    >>
    >> With this change, the code looks like this:
    >>
    >> + if (sme_active())
    >> + return set_memory_decrypted((unsigned long)vaddr, pages);
    >>
    >> now in __set_memory_enc_dec via set_memory_decrypted():
    >>
    >> /* Nothing to do if memory encryption is not active */
    >> if (!mem_encrypt_active())
    >> return 0;
    >>
    >>
    >> so you have:
    >>
    >> if (sme_active())
    >>
    >> ...
    >>
    >> if (!mem_encrypt_active())
    >>
    >>
    >> now maybe this is all clear to you and Tom but I betcha others will get
    >> confused. Probably something like "well, what should be active now, SME,
    >> SEV or memory encryption in general"?
    >>
    >> I hope you're catching my drift.
    >>
    >> So if you want to *not* decrypt memory in the SEV case, then doing something
    >> like this should make it a bit more clear:
    >>
    >>
    >> if (sev_active())
    >> return;
    >>
    >> return set_memory_decrypted((unsigned long)vaddr, pages);
    >>
    >
    >
    > I see your point. I agree it can get confusing.
    >
    >
    >> along with a comment *why* we're checking here.
    >>
    >> But actually, I'd prefer if you had separate wrappers which are called
    >> for SME and for SEV.
    >
    >
    > Just a thought, maybe we can move the above if(sev_active()) check up in
    > kernel/kexec_core.c because we don't need to set/clear the encryption
    > masks when SEV is active so there is no need to call the wrapper.

    IIRC, the wrapper was created to avoid checking in the main kexec support
    and move the check down to the arch specific support. So I don't think we
    should move it.

    I'm not sure about the separate wrappers, I like the above code where the
    arch callback returns if sev_active() is true.

    Maybe what would help is to describe why there is a difference between SME
    and SEV in regards to kexec. During a traditional boot under SME, SME will
    encrypt the kernel, so the SME kexec kernel also needs to be un-encrypted
    in order to replicate a normal SME boot. During a traditional boot under
    SEV, the kernel has already been loaded encrypted, so the SEV kexec kernel
    needs to be encrypted in order to replicate a normal SEV boot.

    Thanks,
    Tom

    >
    >
    >>
    >> I'll let Tom chime in too.
    >>
    >
    \
     
     \ /
      Last update: 2019-03-25 21:00    [W:3.145 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site