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: <6feecf9e-10cb-441f-97a4-65c98e130f7a@linux.microsoft.com>
Date:   Fri, 10 Nov 2023 13:27:08 +0100
From:   Jeremi Piotrowski <jpiotrowski@...ux.microsoft.com>
To:     kirill.shutemov@...ux.intel.com
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>, Borislav Petkov <bp@...en8.de>,
        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,
        sashal@...nel.org
Subject: Re: [PATCH] x86/mm: Check cc_vendor when printing memory encryption
 info

On 10/11/2023 13:06, kirill.shutemov@...ux.intel.com wrote:
> On Thu, Nov 09, 2023 at 07:41:33PM +0100, Jeremi Piotrowski wrote:
>> It's not disregard, the way the kernel behaves in this case is correct except
>> for the error in reporting CPU vendor. Users care about seeing the correct
>> information in dmesg.
> 
> I think it is wrong place to advertise CoCo features. It better fits into
> Intel/AMD-specific code that knows better what it is running on. Inferring
> it from generic code has been proved problematic as you showed.
> 

Let's not make this into a bigger issue than it is. There is a check that does
not cover all cases, and needs to be changed into a different check that covers
all cases. I'd wouldn't call that problematic.

I would be open to moving the function to arch/x86/coco/core.c or providing a
mechanism for cpu/paravisor-specific code to do the reporting (which is what
Dexuan originally proposed [1]).

[1]: https://lore.kernel.org/lkml/20231019062030.3206-1-decui@microsoft.com/

> Maybe just remove incorrect info and that's it?
> 

I disagree, other users and I find the print very useful to see which coco
platform the kernel is running on and which confidential computing features
the kernel detected. I'm willing to fix the code to report correct info.

> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index c290c55b632b..f573a97c0524 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -40,42 +40,6 @@ bool force_dma_unencrypted(struct device *dev)
>  	return false;
>  }
>  
> -static void print_mem_encrypt_feature_info(void)
> -{
> -	pr_info("Memory Encryption Features active:");
> -
> -	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> -		pr_cont(" Intel TDX\n");
> -		return;
> -	}
> -
> -	pr_cont(" AMD");
> -
> -	/* Secure Memory Encryption */
> -	if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
> -		/*
> -		 * SME is mutually exclusive with any of the SEV
> -		 * features below.
> -		 */
> -		pr_cont(" SME\n");
> -		return;
> -	}
> -
> -	/* Secure Encrypted Virtualization */
> -	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> -		pr_cont(" SEV");
> -
> -	/* Encrypted Register State */
> -	if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
> -		pr_cont(" SEV-ES");
> -
> -	/* Secure Nested Paging */
> -	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> -		pr_cont(" SEV-SNP");
> -
> -	pr_cont("\n");
> -}
> -
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void)
>  {
> @@ -85,7 +49,7 @@ void __init mem_encrypt_init(void)
>  	/* Call into SWIOTLB to update the SWIOTLB DMA buffers */
>  	swiotlb_update_mem_attributes();
>  
> -	print_mem_encrypt_feature_info();
> +	pr_info("Memory Encryption is active\n");
>  }
>  
>  void __init mem_encrypt_setup_arch(void)