linux-kernel - Re: [PATCH] x86/mm: Check cc_vendor when printing memory encryption info

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: <73b51be2-cc60-4818-bdba-14b33576366d@linux.microsoft.com>
Date:   Fri, 10 Nov 2023 16:51:43 +0100
From:   Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Dave Hansen <dave.hansen@...el.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Michael Kelley <mhklinux@...look.com>,
        Dexuan Cui <decui@...rosoft.com>, linux-hyperv@...r.kernel.org,
        stefan.bader@...onical.com, tim.gardner@...onical.com,
        roxana.nicolescu@...onical.com, cascardo@...onical.com,
        kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
        kirill.shutemov@...ux.intel.com, sashal@...nel.org
Subject: Re: [PATCH] x86/mm: Check cc_vendor when printing memory encryption
 info

On 10/11/2023 14:17, Borislav Petkov wrote:
> On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
>> tdx_early_init() changes kernel behavior with the assumption that it
>> can talk directly to the TD module or change page visibility in
>> a certain way, instead of talking to a paravisor. So that CPUID is
>> hidden to prevent this.  Otherwise tdx_early_init() would need to be
>> modified to check "am I running with TD partitioning and if so
>> - switch to other implementations".
> 
> Here we go with the virt zoo again. If you hide TDX_CPUID_LEAF_ID from
> it, then it of course doesn't know that it is a TDX guest. This is the
> same thing as the SNP vTom thing: the only viable way going forward is
> for the guest kernel to detect correctly what it runs on and act
> accordingly.

That part already works correctly. The kernel knows very well that it is a
TDX guest because TD partitioning (same as SNP vTOM) support uses the standard
coco mechanisms to indicate that to the kernel. The kernel is well aware of
how to operate in this case: use bounce buffers, flip page visibility by calling
the correct hoosk, etc. Same flow as for every other flavor of confidential guest.

> 
> You can't just do some semi-correct tests for vendor - correct only
> if you squint hard enough - and hope that it works because it'll break
> apart eventually, when that second-level TDX fun needs to add more
> hackery to the guest kernel.
> 

What's semi-correct about checking for CC_VENDOR_INTEL and then printing Intel?
I can post a v2 that checks CC_ATTR_GUEST_MEM_ENCRYPT before printing "TDX".
Feature printing needs to evolve as new technologies come along.

> So, instead, think about how the paravisor tells the guest it is running
> on one - a special CPUID leaf or an MSR in the AMD case - and use that> to detect it properly.

The paravisor *is* telling the guest it is running on one - using a CPUID leaf
(HYPERV_CPUID_ISOLATION_CONFIG). A paravisor is a hypervisor for a confidential
guest, that's why paravisor detection shares logic with hypervisor detection.

tdx_early_init() runs extremely early, way before hypervisor(/paravisor) detection.
If the TDX_CPUID_LEAF_ID leaf were present it would require duplicating hypervisor/paravisor
logic in that function (and in sme_early_init()). As soon as we'd detect the
paravisor we'd need to avoid performing tdx_module_calls() (because they're not
allowed) so the function would return without doing anything useful:

void __init tdx_early_init(void)
{
        u64 cc_mask;
        u32 eax, sig[3];

        cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2],  &sig[1]);

        if (memcmp(TDX_IDENT, sig, sizeof(sig)))
                return;

        setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);

        cc_vendor = CC_VENDOR_INTEL;

        /* Can't perform tdx_module_calls when a paravisor is present */
        if (early_detect_paravisor())
                goto exit;

        ....
exit:
        pr_info("Guest detected\n");
}

Additionally we'd need to sprinkle paravisor checks along with existing X86_FEATURE_TDX_GUEST
checks. And any time someone adds a new feature that depends solely on X86_FEATURE_TDX_GUEST
we'd run the chance of it breaking things.

That would be a mess.

Jeremi

> 
> Everything else is a mess waiting to happen.
> 
> Thx.
>