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

[Tom Lendacky <thomas.lendacky@amd.com>]

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;
>      }
> 
> -    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)
>