| 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
| ||
|
Date: Sat, 19 Feb 2022 00:33:00 +0300
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
luto@...nel.org, peterz@...radead.org,
sathyanarayanan.kuppuswamy@...ux.intel.com, aarcange@...hat.com,
ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
hpa@...or.com, jgross@...e.com, jmattson@...gle.com,
joro@...tes.org, jpoimboe@...hat.com, knsathya@...nel.org,
pbonzini@...hat.com, sdeep@...are.com, seanjc@...gle.com,
tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask
On Fri, Feb 18, 2022 at 12:36:02PM -0800, Dave Hansen wrote:
> On 2/18/22 08:16, Kirill A. Shutemov wrote:
> > +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> > +u64 cc_get_mask(bool enc);
> > +u64 cc_mkenc(u64 val);
> > +u64 cc_mkdec(u64 val);
> > +#else
> > +#define cc_get_mask(enc) 0
> > +#define cc_mkenc(val) (val)
> > +#define cc_mkdec(val) (val)
> > +#endif
>
> Is there a reason the stubs as #defines? Static inlines are preferred
> for consistent type safety among other things.
I was slightly worried about 32-bit non-PAE that has phys_addr_t and
pgprotval_t 32-bit. I was not completely sure it will not cause any
issue due to type mismatch. Maybe it is ungrounded.
With CONFIG_ARCH_HAS_CC_PLATFORM=y, all relevant types are 64-bit.
> It would also be nice to talk about the u64 type in the changelog. If I
> remember right, there was a reason you didn't want to have a pgprot_t
> here.
With standalone <asm/coco.h> I think we can make it work with other type.
But I'm not sure what it has to be.
I found helpers useful for modifying pgprotval_t and phys_addr_t. I
considered u64 a common ground.
Should I change this to something else?
> ...
> > @@ -74,12 +72,52 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)
> >
> > bool cc_platform_has(enum cc_attr attr)
> > {
> > - if (sme_me_mask)
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > return amd_cc_platform_has(attr);
> > -
> > - if (hv_is_isolation_supported())
> > + case CC_VENDOR_INTEL:
> > + return intel_cc_platform_has(attr);
> > + case CC_VENDOR_HYPERV:
> > return hyperv_cc_platform_has(attr);
> > -
> > - return false;
> > + default:
> > + return false;
> > + }
> > }
> > EXPORT_SYMBOL_GPL(cc_platform_has);
>
> This patch is bordering on doing too many different things. Adding the
> CC_VENDOR_*'s in a separate patch probably would have been nice, for
> instance.
>
> This wasn't really broached at all in the changelog.
I'll update a log. Or do you want a separate patch for the vendor
introduction?
> > +u64 cc_get_mask(bool enc)
> > +{
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > + return enc ? cc_mask : 0;
> > + default:
> > + return 0;
> > + }
> > +}
>
> I had to ask myself why you need all three of these functions. It's
> obvious _after_ reading the whole patch, but it left me wanting more in
> the changelog about it.
Okay.
> > +u64 cc_mkenc(u64 val)
> > +{
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > + return val | cc_mask;
> > + default:
> > + return val;
> > + }
> > +}
> > +
> > +u64 cc_mkdec(u64 val)
> > +{
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > + return val & ~cc_mask;
> > + default:
> > + return val;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(cc_mkdec);
> > +
> > +__init void cc_init(enum cc_vendor vendor, u64 mask)
> > +{
> > + cc_vendor = vendor;
> > + cc_mask = mask;
> > +}
> > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > index 5a99f993e639..9af6be143998 100644
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -33,6 +33,7 @@
> > #include <asm/nmi.h>
> > #include <clocksource/hyperv_timer.h>
> > #include <asm/numa.h>
> > +#include <asm/coco.h>
> >
> > /* Is Linux running as the root partition? */
> > bool hv_root_partition;
> > @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> > */
> > swiotlb_force = SWIOTLB_FORCE;
> > #endif
> > + if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> > + cc_init(CC_VENDOR_HYPERV, 0);
> > }
> >
> > if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
> > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > index 3f0abb403340..fa758247ab57 100644
> > --- a/arch/x86/mm/mem_encrypt_identity.c
> > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > @@ -44,6 +44,7 @@
> > #include <asm/setup.h>
> > #include <asm/sections.h>
> > #include <asm/cmdline.h>
> > +#include <asm/coco.h>
> >
> > #include "mm_internal.h"
> >
> > @@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
> > } else {
> > /* SEV state cannot be controlled by a command line option */
> > sme_me_mask = me_mask;
> > - physical_mask &= ~sme_me_mask;
> > - return;
> > + goto out;
> > }
> >
> > /*
> > @@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
> > sme_me_mask = 0;
> > else
> > sme_me_mask = active_by_default ? me_mask : 0;
> > -
> > +out:
> > physical_mask &= ~sme_me_mask;
> > + if (sme_me_mask)
> > + cc_init(CC_VENDOR_AMD, sme_me_mask);
> > }
>
> I don't think you need to mop it up here, but where does this leave
> sme_me_mask?
I think sme_me_mask still can be useful to indicate that the code is only
relevant for AMD context.
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index b4072115c8ef..e79366b8a9da 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -1999,8 +1999,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
> > memset(&cpa, 0, sizeof(cpa));
> > cpa.vaddr = &addr;
> > cpa.numpages = numpages;
> > - cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
> > - cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
> > + cpa.mask_set = __pgprot(cc_get_mask(enc));
> > + cpa.mask_clr = __pgprot(cc_get_mask(!enc));
> > cpa.pgd = init_mm.pgd;
> >
> > /* Must avoid aliasing mappings in the highmem code */
>
> This actually ended up looking pretty nice. It was a real mess along
> the way, but this makes me think this is a _good_ solution.
--
Kirill A. Shutemov
Powered by blists - more mailing lists