linux-kernel - Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

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: <a747227c-c194-47c4-89be-ba509c0633e4@amd.com>
Date: Tue, 12 Aug 2025 18:30:11 -0500
From: Kim Phillips <kim.phillips@....com>
To: "Kalra, Ashish" <ashish.kalra@....com>, Tom Lendacky
	<thomas.lendacky@....com>, <corbet@....net>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
	<hpa@...or.com>, <john.allen@....com>, <herbert@...dor.apana.org.au>,
	<davem@...emloft.net>, <akpm@...ux-foundation.org>, <rostedt@...dmis.org>,
	<paulmck@...nel.org>
CC: <nikunj@....com>, <Neeraj.Upadhyay@....com>, <aik@....com>,
	<ardb@...nel.org>, <michael.roth@....com>, <arnd@...db.de>,
	<linux-doc@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>
Subject: Re: [PATCH v7 7/7] KVM: SEV: Add SEV-SNP CipherTextHiding support

On 8/12/25 2:38 PM, Kalra, Ashish wrote:
> On 8/12/2025 2:11 PM, Kim Phillips wrote:
>> On 8/12/25 1:52 PM, Kalra, Ashish wrote:
>>> On 8/12/2025 1:40 PM, Kim Phillips wrote:
>>>>>> It's not as immediately obvious that it needs to (0 < x < minimum SEV ASID 100).
>>>>>> OTOH, if the user inputs "ciphertext_hiding_asids=0x1", they now see:
>>>>>>
>>>>>>         kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>>>>>
>>>>>> which - unlike the original v7 code - shows the user that the '0x1' was not interpreted as a number at all: thus the 99 in the latter condition.
>>>>> This is incorrect, as 0 < 99 < minimum SEV ASID 100 is a valid condition!
>>>> Precisely, meaning it's the '0x' in '0x1' that's the "invalid" part.
>>>>> And how can user input of 0x1, result in max_snp_asid == 99 ?
>>>> It doesn't, again, the 0x is the invalid part.
>>>>
>>>>> This is the issue with combining the checks and emitting a combined error message:
>>>>>
>>>>> Here, kstroint(0x1) fails with -EINVAL and so, max_snp_asid remains set to 99 and then the combined error conveys a wrong information :
>>>>> !(0 < 99 < minimum SEV ASID 100)
>>>> It's not, it says it's *OR* that condition.
>>> To me this is wrong as
>>> !(0 < 99 < minimum SEV ASID 100) is simply not a correct statement!
>> The diff I provided emits exactly this:
>>
>> kvm_amd: invalid ciphertext_hiding_asids "0x1" or !(0 < 99 < minimum SEV ASID 100)
>>
>>
>> which means *EITHER*:
>>
>> invalid ciphertext_hiding_asids "0x1"
>>
>> *OR*
>>
>> !(0 < 99 < minimum SEV ASID 100)
>>
>> but since the latter is 'true', the user is pointed to the former
>> "0x1" as being the interpretation problem.
>>
>> Would adding the word "Either" help?:
>>
>> kvm_amd: Either invalid ciphertext_hiding_asids "0x1", or !(0 < 99 < minimum SEV ASID 100)
>>
>> ?
> No, i simply won't put an invalid expression out there:
>
> !(0 < 99 < minimum SEV ASID 100)

When not quoted out of context, it's not an invalid expression (in the 
99 case) because it's preceded with the word "or:"

..., or !(0 < 99 < minimum SEV ASID 100)

>> If not, feel free to separate them: the code is still much cleaner.
> Separating the checks will make the code not very different from the original function, so i am going to keep the original code.

Take a look at the example diff below, then.  It's still less, simpler 
code because it eliminates:

1. the unnecessary ciphertext_hiding_asid_nr variable

2. the redundant isdigit(ciphertext_hiding_asids[0])) check
and 3. the 'invalid_parameter:' label referenced by only one goto 
statement. Thanks, Kim arch/x86/kvm/svm/sev.c | 44 
++++++++++++++++++++------------------------ 1 file changed, 20 
insertions(+), 24 deletions(-) diff --git a/arch/x86/kvm/svm/sev.c 
b/arch/x86/kvm/svm/sev.c index 7ac0f0f25e68..d0a13f1b0572 100644 --- 
a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2970,8 +2970,6 
@@ static bool is_sev_snp_initialized(void) static bool 
check_and_enable_sev_snp_ciphertext_hiding(void) { - unsigned int 
ciphertext_hiding_asid_nr = 0; - if (!ciphertext_hiding_asids[0]) return 
false; @@ -2980,32 +2978,28 @@ static bool 
check_and_enable_sev_snp_ciphertext_hiding(void) return false; } - if 
(isdigit(ciphertext_hiding_asids[0])) { - if 
(kstrtoint(ciphertext_hiding_asids, 10, &ciphertext_hiding_asid_nr)) - 
goto invalid_parameter; - - /* Do sanity check on user-defined 
ciphertext_hiding_asids */ - if (ciphertext_hiding_asid_nr >= 
min_sev_asid) { - pr_warn("Module parameter ciphertext_hiding_asids (%u) 
exceeds or equals minimum SEV ASID (%u)\n", - ciphertext_hiding_asid_nr, 
min_sev_asid); - return false; - } - } else if 
(!strcmp(ciphertext_hiding_asids, "max")) { - ciphertext_hiding_asid_nr 
= min_sev_asid - 1; + if (!strcmp(ciphertext_hiding_asids, "max")) { + 
max_snp_asid = min_sev_asid - 1; + min_sev_es_asid = max_snp_asid + 1; + 
return true; } - if (ciphertext_hiding_asid_nr) { - max_snp_asid = 
ciphertext_hiding_asid_nr; - min_sev_es_asid = max_snp_asid + 1; - 
pr_info("SEV-SNP ciphertext hiding enabled\n"); + if 
(kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid)) { + 
pr_warn("ciphertext_hiding_asids \"%s\" is not an integer\n", 
ciphertext_hiding_asids); + return false; + } - return true; + /* Do 
sanity check on user-defined ciphertext_hiding_asids */ + if 
(max_snp_asid < 1 || max_snp_asid >= min_sev_asid) { + pr_warn("!(0 < 
ciphertext_hiding_asids %u < minimum SEV ASID %u)\n", + max_snp_asid, 
min_sev_asid); + max_snp_asid = min_sev_asid - 1; + return false; } 
-invalid_parameter: - pr_warn("Module parameter ciphertext_hiding_asids 
(%s) invalid\n", - ciphertext_hiding_asids); - return false; + 
min_sev_es_asid = max_snp_asid + 1; + + return true; } void __init 
sev_hardware_setup(void) @@ -3122,8 +3116,10 @@ void __init 
sev_hardware_setup(void) * ASID range into separate SEV-ES and SEV-SNP 
ASID ranges with * the SEV-SNP ASID starting at 1. */ - if 
(check_and_enable_sev_snp_ciphertext_hiding()) + if 
(check_and_enable_sev_snp_ciphertext_hiding()) { + pr_info("SEV-SNP 
ciphertext hiding enabled\n"); init_args.max_snp_asid = max_snp_asid; + 
} if (sev_platform_init(&init_args)) sev_supported = sev_es_supported = 
sev_snp_supported = false; else if (sev_snp_supported)