linux-kernel - Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c17990ac-30b2-4bdc-b31a-811af6052782@amd.com>
Date: Mon, 18 Aug 2025 15:39:14 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Kim Phillips <kim.phillips@....com>,
 Herbert Xu <herbert@...dor.apana.org.au>
Cc: Neeraj.Upadhyay@....com, aik@....com, akpm@...ux-foundation.org,
 ardb@...nel.org, arnd@...db.de, bp@...en8.de, corbet@....net,
 dave.hansen@...ux.intel.com, davem@...emloft.net, hpa@...or.com,
 john.allen@....com, kvm@...r.kernel.org, linux-crypto@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 michael.roth@....com, mingo@...hat.com, nikunj@....com, paulmck@...nel.org,
 pbonzini@...hat.com, rostedt@...dmis.org, seanjc@...gle.com,
 tglx@...utronix.de, thomas.lendacky@....com, x86@...nel.org
Subject: Re: [PATCH v7 0/7] Add SEV-SNP CipherTextHiding feature support



On 8/18/2025 2:38 PM, Kim Phillips wrote:
> On 8/18/25 2:16 PM, Kalra, Ashish wrote:
>> On 8/16/2025 3:39 AM, Herbert Xu wrote:
>>> On Mon, Aug 11, 2025 at 08:30:25PM +0000, Ashish Kalra wrote:
>>>> Hi Herbert, can you please merge patches 1-5.
>>>>
>>>> Paolo/Sean/Herbert, i don't know how do you want handle cross-tree merging
>>>> for patches 6 & 7.
>>> These patches will be at the base of the cryptodev tree for 6.17
>>> so it could be pulled into another tree without any risks.
>>>
>>> Cheers,
>> Thanks Herbert for pulling in patches 1-5.
>>
>> Paolo, can you please merge patches 6 and 7 into the KVM tree.
> Hi Ashish,
> 
> I have pending comments on patch 7:
> 
> https://lore.kernel.org/kvm/e32a48dc-a8f7-4770-9e2f-1f3721872a63@amd.com/
> 
> If still not welcome, can you say why you think:
> 
> 1. The ciphertext_hiding_asid_nr variable is necessary

I prefer safe coding, and i don't want to update max_snp_asid, until i am sure there are no sanity 
check failures and that's why i prefer using a *temp* variable and then updating max_snp_asid when i
am sure all sanity checks have been done.

Otherwise, in your case you are updating max_snp_asid and then rolling it back in case of sanity check
failures, i don't like that. 

> 
> 2. The isdigit(ciphertext_hiding_asids[0])) check is needed when it's immediately followed by kstrtoint which effectively makes the open-coded isdigit check  redundant?

isdigit() is a MACRO compared to kstrtoint() call, it is more optimal to do an inline check and avoid
calling kstrtoint() if the parameter is not a number.

> 
> 3. Why the 'invalid_parameter:' label referenced by only one goto statement can't be folded and removed.

This is for understandable code flow :

1). Check module parameter is set by user.
2). Check ciphertext_hiding_feature enabled.
3). Check if parameter is numeric.
    Sanity checks on numeric parameter
    If checks fail goto invalid_parameter
4). Check if parameter is the string "max".
5). Set max_snp_asid and min_sev_es_asid. 
6). Fall-through to invalid parameter.
invalid_parameter: 

This is overall a more understandable code flow.

Again, this is just a module parameter checking function and not something which will affect runtime performance by eliminating a single temporary variable or jump label.

Thanks,
Ashish

> 
> Thanks,
> 
> Kim