linux-kernel - Re: [PATCH v3 02/14] x86/sev: Make the VMPL0 checking function more generic

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]

Date: Wed, 17 Apr 2024 15:35:53 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, linux-coco@...ts.linux.dev,
 svsm-devel@...onut-svsm.dev, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>,
 Dan Williams <dan.j.williams@...el.com>, Michael Roth
 <michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>
Subject: Re: [PATCH v3 02/14] x86/sev: Make the VMPL0 checking function more
 generic

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.
>