diff mbox series

[RESEND] x86/sev: Fix SEV check in sev_map_percpu_data()

Message ID 20240124130317.495519-1-kirill.shutemov@linux.intel.com
State New
Headers
Series [RESEND] x86/sev: Fix SEV check in sev_map_percpu_data() |

Commit Message

Kirill A. Shutemov Jan. 24, 2024, 1:03 p.m. UTC 
  The function sev_map_percpu_data() checks if it is running on an SEV
platform by checking the CC_ATTR_GUEST_MEM_ENCRYPT attribute. However,
this attribute is also defined for TDX.

To avoid false positives, add a cc_vendor check.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: 4d96f9109109 ("x86/sev: Replace occurrences of sev_active() with cc_platform_has()")
Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
Acked-by: David Rientjes <rientjes@google.com>
---
 arch/x86/kernel/kvm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini Jan. 26, 2024, 6:43 p.m. UTC | #1 
On Wed, Jan 24, 2024 at 2:09 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> The function sev_map_percpu_data() checks if it is running on an SEV
> platform by checking the CC_ATTR_GUEST_MEM_ENCRYPT attribute. However,
> this attribute is also defined for TDX.
>
> To avoid false positives, add a cc_vendor check.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: 4d96f9109109 ("x86/sev: Replace occurrences of sev_active() with cc_platform_has()")
> Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
> Acked-by: David Rientjes <rientjes@google.com>

Queued, with "x86/kvm in the subject".

Paolo

> ---

>  arch/x86/kernel/kvm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index dfe9945b9bec..428ee74002e1 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -434,7 +434,8 @@ static void __init sev_map_percpu_data(void)
>  {
>         int cpu;
>
> -       if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> +       if (cc_vendor != CC_VENDOR_AMD ||
> +           !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
>                 return;
>
>         for_each_possible_cpu(cpu) {
> --
> 2.43.0
>
Nathan Chancellor Feb. 1, 2024, 7:38 p.m. UTC | #2 
On Wed, Jan 24, 2024 at 03:03:17PM +0200, Kirill A. Shutemov wrote:
> The function sev_map_percpu_data() checks if it is running on an SEV
> platform by checking the CC_ATTR_GUEST_MEM_ENCRYPT attribute. However,
> this attribute is also defined for TDX.
> 
> To avoid false positives, add a cc_vendor check.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: 4d96f9109109 ("x86/sev: Replace occurrences of sev_active() with cc_platform_has()")
> Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
> Acked-by: David Rientjes <rientjes@google.com>
> ---
>  arch/x86/kernel/kvm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index dfe9945b9bec..428ee74002e1 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -434,7 +434,8 @@ static void __init sev_map_percpu_data(void)
>  {
>  	int cpu;
>  
> -	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> +	if (cc_vendor != CC_VENDOR_AMD ||
> +	    !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
>  		return;
>  
>  	for_each_possible_cpu(cpu) {
> -- 
> 2.43.0
> 

Our CI has started seeing a build failure as a result of this patch when
using LLVM to build x86_64_defconfig + CONFIG_GCOV_KERNEL=y +
CONFIG_GCOV_PROFILE_ALL=y:

  $ echo 'CONFIG_GCOV_KERNEL=y
  CONFIG_GCOV_PROFILE_ALL=y' >kernel/configs/gcov.config

  $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig gcov.config vmlinux
  ...
  ld.lld: error: undefined symbol: cc_vendor
  >>> referenced by kvm.c
  >>>               arch/x86/kernel/kvm.o:(kvm_smp_prepare_boot_cpu) in archive vmlinux.a
  ...

I was somewhat confused at first why this build error only shows up with
GCOV until I looked at the optimized IR. This configuration has
CONFIG_ARCH_HAS_CC_PLATFORM=n, which means that cc_vendor is declared
but not defined anywhere, so I was expecting an unconditional error.
Looking closer, I realized that cc_platform_has() evaluates to
false in that configuration, so the compiler can always turn

  if (cond || !false)
      action();

into

  action();

but it seems like with the GCOV instrumentation, it keeps both branches
(since GCOV is about code coverage, it makes sense that you would want
to see if each branch is ever taken). I can eliminate the error with the
following diff, I am not sure if that is too much though.

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 428ee74002e1..4432ee09cbcb 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -434,7 +434,7 @@ static void __init sev_map_percpu_data(void)
 {
 	int cpu;
 
-	if (cc_vendor != CC_VENDOR_AMD ||
+	if ((IS_ENABLED(CONFIG_ARCH_HAS_CC_PLATFORM) && cc_vendor != CC_VENDOR_AMD) ||
 	    !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
 		return;
 
Perhaps another solution would be to just

  #define cc_vendor (CC_VENDOR_NONE)

if CONFIG_ARCH_HAS_CC_PLATFORM is not set, since it can never be changed
from the default in arch/x86/coco/core.c.

diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 6ae2d16a7613..f3909894f82f 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -10,13 +10,13 @@ enum cc_vendor {
 	CC_VENDOR_INTEL,
 };
 
-extern enum cc_vendor cc_vendor;
-
 #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+extern enum cc_vendor cc_vendor;
 void cc_set_mask(u64 mask);
 u64 cc_mkenc(u64 val);
 u64 cc_mkdec(u64 val);
 #else
+#define cc_vendor (CC_VENDOR_NONE)
 static inline u64 cc_mkenc(u64 val)
 {
 	return val;

Cheers,
Nathan
Kirill A. Shutemov Feb. 2, 2024, 10:30 a.m. UTC | #3 
On Thu, Feb 01, 2024 at 12:38:09PM -0700, Nathan Chancellor wrote:
> Perhaps another solution would be to just
> 
>   #define cc_vendor (CC_VENDOR_NONE)
> 
> if CONFIG_ARCH_HAS_CC_PLATFORM is not set, since it can never be changed
> from the default in arch/x86/coco/core.c.

I think this approach is cleaner.

Could you post a proper patch?

> 
> diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> index 6ae2d16a7613..f3909894f82f 100644
> --- a/arch/x86/include/asm/coco.h
> +++ b/arch/x86/include/asm/coco.h
> @@ -10,13 +10,13 @@ enum cc_vendor {
>  	CC_VENDOR_INTEL,
>  };
>  
> -extern enum cc_vendor cc_vendor;
> -
>  #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> +extern enum cc_vendor cc_vendor;
>  void cc_set_mask(u64 mask);
>  u64 cc_mkenc(u64 val);
>  u64 cc_mkdec(u64 val);
>  #else
> +#define cc_vendor (CC_VENDOR_NONE)
>  static inline u64 cc_mkenc(u64 val)
>  {
>  	return val;
> 
> Cheers,
> Nathan
>
diff mbox series

Patch

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index dfe9945b9bec..428ee74002e1 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -434,7 +434,8 @@  static void __init sev_map_percpu_data(void)
 {
 	int cpu;
 
-	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
+	if (cc_vendor != CC_VENDOR_AMD ||
+	    !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
 		return;
 
 	for_each_possible_cpu(cpu) {