| 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
| ||
|
Message-ID: <e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com>
Date: Thu, 14 Aug 2025 06:54:10 -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 6:30 PM, Kim Phillips wrote:
> 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.
Re-posting, since I believe the previous email's diff got mangled:
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..1b9702500c73 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 or 'max'\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)
Powered by blists - more mailing lists