linux-kernel - Re: [PATCH v5 6/8] x86/sev: Treat the contiguous RMP table as a single RMP segment

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: <20241108172346.GBZy5JIv9qbbOZbo2k@fat_crate.local>
Date: Fri, 8 Nov 2024 18:23:46 +0100
From: Borislav Petkov <bp@...en8.de>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Michael Roth <michael.roth@....com>,
	Ashish Kalra <ashish.kalra@....com>,
	Nikunj A Dadhania <nikunj@....com>,
	Neeraj Upadhyay <Neeraj.Upadhyay@....com>
Subject: Re: [PATCH v5 6/8] x86/sev: Treat the contiguous RMP table as a
 single RMP segment

On Mon, Nov 04, 2024 at 10:03:22AM -0600, Tom Lendacky wrote:
> >> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> > 
> > 			sizeof(struct rmp_segment_desc)
> 
> Hmmm... I prefer to keep this as sizeof(*desc). If any changes are made
> to the structure name, then this line doesn't need to be changed.

Oh boy, we really do that:

from Documentation/process/coding-style.rst

"The preferred form for passing a size of a struct is the following:

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not."

Well, my problem with using the variable name as sizeof() argument is that you
need to go and look at what type it is to know what you're sizing. And it
doesn't really hurt readability - on the contrary - it makes it more readable.

/facepalm.

> > Why isn't this zeroing out part of alloc_rmp_segment_table()?

First of all:

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 37ff4f98e8d1..1ae782194626 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -293,18 +293,16 @@ static void __init free_rmp_segment_table(void)
 	rmp_segment_table = NULL;
 }
 
-static bool __init alloc_rmp_segment_table(void)
+static struct __init rmp_segment_desc **alloc_rmp_segment_table(void)
 {
 	struct page *page;
 
 	/* Allocate the table used to index into the RMP segments */
 	page = alloc_page(__GFP_ZERO);
 	if (!page)
-		return false;
-
-	rmp_segment_table = page_address(page);
+		return NULL;
 
-	return true;
+	return page_address(page);
 }
 
 /*
@@ -344,7 +342,8 @@ static int __init snp_rmptable_init(void)
 		goto nosnp;
 	}
 
-	if (!alloc_rmp_segment_table())
+	rmp_segment_table = alloc_rmp_segment_table();
+	if (!rmp_segment_table)
 		goto nosnp;
 
 	/* Map only the RMP entries */
---

which makes the code a lot more readable and natural.

> If the SNP_EN bit is set in the SYSCFG MSR, the code needs to skip the
> zeroing of the RMP table since it no longer has write access to the
> table (this happens with kexec).

So we allocate a rmp_segment_table page when we kexec but we can't write the
entries? Why?

This sounds like some weird limitation. Or maybe I'm missing something.

> This keeps the code consistent with the prior code as to where the
> zeroing occurs. I can add another argument to alloc_rmp_segment_desc()
> that tells it whether to clear it or not, if you prefer.

You can also merge that init_rmptable_bookkeeping() and the zeroing of the RMP
entries into one function because they happen back-to-back.

In any case the placement of this whole dance still doesn't look completely
optimal to me.

> This is where I was worried about the VMM/guest being able to get into
> this routine with a bad PFN value.
> 
> This function is invoked from dump_rmpentry(), which can be invoked from:
> 
> rmpupdate() - I think this is safe because the adjust_direct_map() will
> fail if the PFN isn't valid, before the RMP is accessed.
> 
> snp_leak_pages() - I think this is safe because the PFN is based on an
> actual allocation.
> 
> snp_dump_hva_rmpentry() - This is called from the page fault handler. I
> think this invocation is safe for now because because it is only called
> if the fault type is an RMP fault type, which implies that the RMP is
> involved. But as an external function, there's no guarantee as to the
> situation it can be called from in the future.
> 
> I can remove it for now if you think it will be safe going forward.

I like being cautious and you probably should put this deliberation above the
array_index_nospec() so that it is clear to future generations reading this
code.

And yeah, it might be safe but it is not like it costs compared to the
remaining operations happening here so maybe, out of abundance of caution, we
should keep it there and explain why we do so.

Hmmm?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette