Currently, the enforce_vmpl0() function uses a set argument when testing
for VMPL0 and terminates the guest if the guest is not running at VMPL0.
Make the function more generic by moving it into the common code, renaming
it, allowing it to take an argument for use in the VMPL0 check (RMPADJUST
instruction) and return the result of the check, allowing the caller to
determine the action taken based on the result.
Signed-off-by: Tom Lendacky <[email protected]>
---
arch/x86/boot/compressed/sev.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 5ad0ff4664f1..49dc9661176d 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -335,10 +335,9 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
}
-static void enforce_vmpl0(void)
+static bool running_at_vmpl0(void *va)
{
u64 attrs;
- int err;
/*
* RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
@@ -347,12 +346,11 @@ static void enforce_vmpl0(void)
*
* If the guest is running at VMPL0, it will succeed. Even if that operation
* modifies permission bits, it is still ok to do so currently because Linux
- * SNP guests are supported only on VMPL0 so VMPL1 or higher permission masks
- * changing is a don't-care.
+ * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
+ * permission mask changes are a don't-care.
*/
attrs = 1;
- if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, attrs))
- sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
+ return !rmpadjust((unsigned long)va, RMP_PG_SIZE_4K, attrs);
}
/*
@@ -588,7 +586,8 @@ void sev_enable(struct boot_params *bp)
if (!(get_hv_features() & GHCB_HV_FT_SNP))
sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
- enforce_vmpl0();
+ if (!running_at_vmpl0(&boot_ghcb_page))
+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
}
if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
--
2.43.2
On 3/25/2024 11:26 PM, Tom Lendacky wrote:
> Currently, the enforce_vmpl0() function uses a set argument when testing
> for VMPL0 and terminates the guest if the guest is not running at VMPL0.
>
> Make the function more generic by moving it into the common code, renaming
> it, allowing it to take an argument for use in the VMPL0 check (RMPADJUST
> instruction) and return the result of the check, allowing the caller to
> determine the action taken based on the result.
>
> Signed-off-by: Tom Lendacky <[email protected]>
This is preparatory patch for patch3.
Reviewed-by: Pankaj Gupta <[email protected]>
> ---
> arch/x86/boot/compressed/sev.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 5ad0ff4664f1..49dc9661176d 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -335,10 +335,9 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
> sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
> }
>
> -static void enforce_vmpl0(void)
> +static bool running_at_vmpl0(void *va)
> {
> u64 attrs;
> - int err;
>
> /*
> * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
> @@ -347,12 +346,11 @@ static void enforce_vmpl0(void)
> *
> * If the guest is running at VMPL0, it will succeed. Even if that operation
> * modifies permission bits, it is still ok to do so currently because Linux
> - * SNP guests are supported only on VMPL0 so VMPL1 or higher permission masks
> - * changing is a don't-care.
> + * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> + * permission mask changes are a don't-care.
> */
> attrs = 1;
> - if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, attrs))
> - sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
> + return !rmpadjust((unsigned long)va, RMP_PG_SIZE_4K, attrs);
> }
>
> /*
> @@ -588,7 +586,8 @@ void sev_enable(struct boot_params *bp)
> if (!(get_hv_features() & GHCB_HV_FT_SNP))
> sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>
> - enforce_vmpl0();
> + if (!running_at_vmpl0(&boot_ghcb_page))
> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
> }
>
> if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
On Mon, Mar 25, 2024 at 05:26:21PM -0500, Tom Lendacky wrote:
> -static void enforce_vmpl0(void)
> +static bool running_at_vmpl0(void *va)
Not too crazy about it: you're turning it into a function which runs in
boolean context but takes a void *?!
And the boolean result is only a side-effect or what it does to the
argument - modify its permissions. Which is weird and not really
obvious.
I'd prefer it if you made it into
static void vmpl0_modify_permissions(void *va)
which basically says, modify the permissions of @va in vmpl0, which is
a lot closer to what the function does.
And then do
#define running_at_vmpl0(va) vmpl0_modify_permissions((va))
because then through the indirection is at least clear how that "am
I running at VMPL0?" check is being done.
And later, if we need other VMPLs, we can extend
vmpl0_modify_permissions() and even do a more generic
vmpl_modify_permissions(unsigned int vmpl_level, void *va)
and so on and kill the silly macro.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/17/24 06:46, Borislav Petkov wrote:
> On Mon, Mar 25, 2024 at 05:26:21PM -0500, Tom Lendacky wrote:
>> -static void enforce_vmpl0(void)
>> +static bool running_at_vmpl0(void *va)
>
> Not too crazy about it: you're turning it into a function which runs in
> boolean context but takes a void *?!
>
> And the boolean result is only a side-effect or what it does to the
> argument - modify its permissions. Which is weird and not really
> obvious.
Well, it doesn't really modify any permissions that matter. It tries to
change the permission of a lesser privileged VMPL level. Since the
kernel only runs at a single VMPL it would never be effected. The
operation performed here is to update VMPL1 permission levels (which can
only be done successfully at VMPL0) and return the result of the
operation. A success implies running at VMPL0 and failure implies not
running at VMPL0.
>
> I'd prefer it if you made it into
>
> static void vmpl0_modify_permissions(void *va)
I guess this confuses me, since it sounds like you're trying to modify
the VMPL0 permissions, which you can't do. Maybe calling it
modify_vmpl1_permissions() would be better. And a void return doesn't
tell me whether it was successful and, therefore, whether the kernel is
running at VMPL0.
Thanks,
Tom
>
> which basically says, modify the permissions of @va in vmpl0, which is
> a lot closer to what the function does.
>
> And then do
>
> #define running_at_vmpl0(va) vmpl0_modify_permissions((va))
>
> because then through the indirection is at least clear how that "am
> I running at VMPL0?" check is being done.
>
> And later, if we need other VMPLs, we can extend
> vmpl0_modify_permissions() and even do a more generic
>
> vmpl_modify_permissions(unsigned int vmpl_level, void *va)
>
> and so on and kill the silly macro.
>
> Thx.
>
On Wed, Apr 17, 2024 at 03:35:53PM -0500, Tom Lendacky wrote:
> Well, it doesn't really modify any permissions that matter. It tries to
> change the permission of a lesser privileged VMPL level.
Potato potato. :-P
> Since the kernel only runs at a single VMPL it would never be
> effected. The operation performed here is to update VMPL1 permission
> levels (which can only be done successfully at VMPL0) and return the
> result of the operation. A success implies running at VMPL0 and
> failure implies not running at VMPL0.
Yap.
The point is, it is calling RMPADJUST. And it does modify RMP
permissions of a guest page.
Thus, if you prefer, you can go all out and call the helper
rmp_adjust_permissions(unsigned int tgt_vmpl, void *pa)
and make it generic from the get-go.
And then have the macro wrap around it in order to explain that
particular use with the macro name.
If it is still not clear what I mean, lemme know and I'll do a diff
ontop.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/17/24 15:50, Borislav Petkov wrote:
> On Wed, Apr 17, 2024 at 03:35:53PM -0500, Tom Lendacky wrote:
>
> Yap.
>
> The point is, it is calling RMPADJUST. And it does modify RMP
> permissions of a guest page.
>
> Thus, if you prefer, you can go all out and call the helper
>
> rmp_adjust_permissions(unsigned int tgt_vmpl, void *pa)
>
> and make it generic from the get-go.
I think I'll just eliminate the function then and call rmpadjust directly,
that should make it clear.
Thanks,
Tom
>
> And then have the macro wrap around it in order to explain that
> particular use with the macro name.
>
> If it is still not clear what I mean, lemme know and I'll do a diff
> ontop.
>
> Thx.
>
On Thu, Apr 18, 2024 at 01:38:49PM -0500, Tom Lendacky wrote:
> I think I'll just eliminate the function then and call rmpadjust directly,
> that should make it clear.
Your call.
However, if we're anticipating more uses around RMPADJUST like that we
might as well design the interface properly from the very beginning.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette