| 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: <a6864a2c-b88f-4639-bf66-0b0cfbc5b20c@amd.com>
Date: Tue, 12 Aug 2025 11:45:00 -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 9:40 AM, Kalra, Ashish wrote:
> On 8/12/2025 7:06 AM, Kim Phillips wrote:
>> arch/x86/kvm/svm/sev.c | 47 ++++++++++++++++++-----------------------------
>> 1 file changed, 18 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 7ac0f0f25e68..57c6e4717e51 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2970,42 +2970,29 @@ 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;
>> -
>> - if (!sev_is_snp_ciphertext_hiding_supported()) {
>> + if (ciphertext_hiding_asids[0] && !sev_is_snp_ciphertext_hiding_supported()) {
>> pr_warn("Module parameter ciphertext_hiding_asids specified but ciphertext hiding not supported\n");
>> return false;
>> }
>>
> This is incorrect, if ciphertext_hiding_asids module parameter is never specified, user will always
> get a warning of an invalid ciphertext_hiding_asids module parameter.
>
> When this module parameter is optional why should the user get a warning about an invalid module parameter.
Ack, sorry, new diff below that fixes this.
> Again, why do we want to do all these checks below if this module parameter has not been specified by
> the user ?
Not sure what you mean by 'below' here (assuming in the resulting code),
but, in general, there are less checks with this diff than the original
v7 code.
>> - 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);
> A *combined* error message such as this:
> "invalid ciphertext_hiding_asids XXX or !(0 < XXX < minimum SEV ASID 100)"
>
> is going to be really confusing to the user.
>
> It is much simpler for user to understand if the error/warning is:
> "Module parameter ciphertext_hiding_asids XXX exceeds or equals minimum SEV ASID YYY"
> OR
> "Module parameter ciphertext_hiding_asids XXX invalid"
I tend to disagree. If, e.g., the user sets ciphertext_hiding_asids=100,
they see:
kvm_amd: invalid ciphertext_hiding_asids "100" or !(0 < 100 <
minimum SEV ASID 100)
which the user can easily unmistakably and quickly deduce that the
problem is the latter - not the former - condition that has the problem.
The original v7 code in that same case would emit:
kvm_amd: Module parameter ciphertext_hiding_asids (100) exceeds or
equals minimum SEV ASID (100)
...to which the user would ask themselves "What's wrong with equalling
the minimum SEV ASID (100)"?
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.
But all this is nothing compared to the added simplicity resulting from
making the change to the original v7 code.
New diff from original v7 below:
arch/x86/kvm/svm/sev.c | 42 +++++++++++++++++-------------------------
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7ac0f0f25e68..a879ea5f53f2 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,24 @@ 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 (ciphertext_hiding_asid_nr) {
- max_snp_asid = ciphertext_hiding_asid_nr;
+ if (!strcmp(ciphertext_hiding_asids, "max")) {
+ max_snp_asid = min_sev_asid - 1;
min_sev_es_asid = max_snp_asid + 1;
- pr_info("SEV-SNP ciphertext hiding enabled\n");
-
return true;
}
-invalid_parameter:
- pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
- ciphertext_hiding_asids);
- return false;
+ /* Do sanity check on user-defined ciphertext_hiding_asids */
+ if (kstrtoint(ciphertext_hiding_asids, 10, &max_snp_asid) ||
+ !max_snp_asid || max_snp_asid >= min_sev_asid) {
+ pr_warn("invalid ciphertext_hiding_asids \"%s\" or !(0 <
%u < minimum SEV ASID %u)\n",
+ ciphertext_hiding_asids, max_snp_asid,
min_sev_asid);
+ max_snp_asid = min_sev_asid - 1;
+ return false;
+ }
+
+ min_sev_es_asid = max_snp_asid + 1;
+
+ return true;
}
void __init sev_hardware_setup(void)
@@ -3122,8 +3112,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)
Thanks,
Kim
Powered by blists - more mailing lists