| 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: <29bff13f-5926-49bb-af54-d4966ff3be96@amd.com>
Date: Tue, 12 Aug 2025 07:06:38 -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 7/25/25 1:46 PM, Kalra, Ashish wrote:
> On 7/25/2025 1:28 PM, Tom Lendacky wrote:
>> On 7/25/25 12:58, Kim Phillips wrote:
>>> Hi Ashish,
>>>
>>> For patches 1 through 6 in this series:
>>>
>>> Reviewed-by: Kim Phillips <kim.phillips@....com>
>>>
>>> For this 7/7 patch, consider making the simplification changes I've supplied
>>> in the diff at the bottom of this email: it cuts the number of lines for
>>> check_and_enable_sev_snp_ciphertext_hiding() in half.
>> Not sure that change works completely... see below.
>>
>>> Thanks,
>>>
>>> Kim
>>>
>>> On 7/21/25 9:14 AM, Ashish Kalra wrote:
>>>> From: Ashish Kalra <ashish.kalra@....com>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 7ac0f0f25e68..bd0947360e18 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -59,7 +59,7 @@ static bool sev_es_debug_swap_enabled = true;
>>> module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0444);
>>> static u64 sev_supported_vmsa_features;
>>>
>>> -static char ciphertext_hiding_asids[16];
>>> +static char ciphertext_hiding_asids[10];
>>> module_param_string(ciphertext_hiding_asids, ciphertext_hiding_asids,
>>> sizeof(ciphertext_hiding_asids), 0444);
>>> MODULE_PARM_DESC(ciphertext_hiding_asids, " Enable ciphertext hiding for
>>> SEV-SNP guests and specify the number of ASIDs to use ('max' to utilize
>>> all available SEV-SNP ASIDs");
>>> @@ -2970,42 +2970,22 @@ 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 the parameter was never specified
>>> -
>>> - if (!sev_is_snp_ciphertext_hiding_supported()) {
>>> - pr_warn("Module parameter ciphertext_hiding_asids specified but
>>> ciphertext hiding not supported\n");
>>> - return false;
>>> - }
>> Removing this block will create an issue below.
>>
>>> -
>>> - 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;
>>> + return true;
>>> }
> As Tom has already pointed out, we will try enabling ciphertext hiding with SNP_INIT_EX even if ciphertext hiding feature is not supported and enabled.
AFAICT, Tom pointed out two bugs with my changes: the 'base' argument to
kstrtoint(), and bad min_sev_es_asid assignment if ciphertext hiding
isn't supported.
> We do need to make these basic checks, i.e., if the parameter has been specified and if ciphertext hiding feature is supported and enabled,
> before doing any further processing.
>
> Why should we even attempt to do any parameter comparison, parameter conversion or sanity checks if the parameter has not been specified and/or
> ciphertext hiding feature itself is not supported and enabled.
Agreed.
> I believe this function should be simple and understandable which it is.
Please take a look at the new diff below: I believe it's even simpler
and more understandable as it's less code, and now alerts the user if
they provide an empty "ciphertext_hiding_asids= ".
Thanks,
Kim
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;
}
- 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 >= 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 +3109,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,
> Ashish
>
>>> - 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");
>>> -
>>> - return true;
>>> + /* Do sanity check on user-defined ciphertext_hiding_asids */
>>> + if (kstrtoint(ciphertext_hiding_asids,
>>> sizeof(ciphertext_hiding_asids), &max_snp_asid) ||
>> The second parameter is supposed to be the base, this gets lucky because
>> you changed the size of the ciphertext_hiding_asids to 10.
>>
>>> + max_snp_asid >= min_sev_asid ||
>>> + !sev_is_snp_ciphertext_hiding_supported()) {
>>> + pr_warn("ciphertext_hiding not supported, or 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;
>>> }
>>>
>>> -invalid_parameter:
>>> - pr_warn("Module parameter ciphertext_hiding_asids (%s) invalid\n",
>>> - ciphertext_hiding_asids);
>>> - return false;
>>> + return true;
>>> }
>>>
>>> void __init sev_hardware_setup(void)
>>> @@ -3122,8 +3102,11 @@ 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;
>>> + min_sev_es_asid = max_snp_asid + 1;
>> If "max" was specified, but ciphertext hiding isn't enabled, you've now
>> changed min_sev_es_asid to an incorrect value and will be trying to enable
>> ciphertext hiding during initialization.
>>
>> Thanks,
>> Tom
>>
>>> + }
>>> 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