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

[Kim Phillips <kim.phillips@amd.com>]

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@amd.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@amd.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)
>>>