linux-kernel - Re: [PATCH] soc: qcom: Add HWKM v1 support for wrapped keys

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] [day] [month] [year] [list]

Message-ID: <5dfd5c3b-08eb-e17a-69c2-68b5c1b6a4ed@oss.qualcomm.com>
Date: Mon, 27 Oct 2025 15:26:22 +0530
From: Neeraj Soni <neeraj.soni@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, andersson@...nel.org,
        konradybcio@...nel.org
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] soc: qcom: Add HWKM v1 support for wrapped keys

Hi Konrad,

On 10/27/2025 2:24 PM, Konrad Dybcio wrote:
> On 10/26/25 5:47 PM, Neeraj Soni wrote:
>> HWKM v1 and v2 differ slightly in wrapped key size and the bit fields for
>> certain status registers and operating mode (legacy or standard).
>>
>> Add support to select HWKM version based on the major and minor revisions.
>> Use this HWKM version to select wrapped key size and to configure the bit
>> fields in registers for operating modes and hardware status.
>>
>> Support for SCM calls for wrapped keys is being added in the TrustZone for
>> few SoCs with HWKM v1. Existing check of qcom_scm_has_wrapped_key_support()
>> API ensures that HWKM is used only if these SCM calls are supported in
>> TrustZone for that SoC.
>>
>> Signed-off-by: Neeraj Soni <neeraj.soni@....qualcomm.com>
>> ---
> 
> The subject must say "ice:"
Ok
> 
> [...]
> 
>>  static bool qcom_ice_check_supported(struct qcom_ice *ice)
>> @@ -114,9 +126,26 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
>>  		return false;
>>  	}
>>  
>> +	/* HWKM version v2 is present from ICE 3.2.1 onwards while version v1
>> +	 * is present only in ICE 3.2.0.
> 
> What about before v3.2.0?
Earlier ICE versions do not have HWKM. Will add this description in next revision.
> 
>> +	 */
>> +	if (major == 4 ||
>> +	   (major == 3 && (minor >= 3 || (minor == 2 && step >= 1))))
>> +		ice->hwkm_version = QCOM_ICE_HWKM_V2;
>> +	else if ((major == 3) && (minor == 2))
>> +		ice->hwkm_version = QCOM_ICE_HWKM_V1;
>> +	else
>> +		ice->hwkm_version = 0;
> 
> "if major > 3" is more future-proof than "== 4", unless you know for
> a fact major == 5 etc. will have an incompatible software interface
Thanks. Yes the software interfaces are compatible with versions having 
"major > 3". I will fix it in next revision.
> 
>>  	dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
>>  		 major, minor, step);
>>  
>> +	if (!ice->hwkm_version)
>> +		dev_info(dev, "QC Hard Ware Key Manager (HWKM) not supported\n");
> 
> "Hard Ware" looks *really* poor
I apologies to miss this. Will fix it in next revision.
> 
> [...]
> 
>> -	BUILD_BUG_ON(QCOM_ICE_HWKM_WRAPPED_KEY_SIZE >
>> +	BUILD_BUG_ON(QCOM_ICE_HWKM_WRAPPED_KEY_SIZE(ice->hwkm_version) >
>>  		     BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE);
> 
> This is not going to work how you want it to..
Thanks for pointing out. To fix it, i will define a maximum HWKM wrapped Key size.
Currently HWKM v1 supports wrapped key size of 100 bytes which is max. If this
changes in future HWKM versions then the size will be updated here.

#define	QCOM_ICE_HWKM_MAX_WRAPPED_KEY_SIZE 	100

> 
>>  	/*
>>  	 * When ICE is in HWKM mode, it only supports wrapped keys.
>> @@ -238,9 +266,18 @@ static void qcom_ice_hwkm_init(struct qcom_ice *ice)
>>  	 *
>>  	 * Put ICE in HWKM mode.  ICE defaults to legacy mode.
>>  	 */
>> -	regval = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
>> -	regval &= ~QCOM_ICE_LEGACY_MODE_ENABLED;
>> -	qcom_ice_writel(ice, regval, QCOM_ICE_REG_CONTROL);
>> +	if (ice->hwkm_version == QCOM_ICE_HWKM_V2) {
>> +		regval = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
>> +		regval &= ~QCOM_ICE_LEGACY_MODE_ENABLED;
>> +		qcom_ice_writel(ice, regval, QCOM_ICE_REG_CONTROL);
>> +	} else if (ice->hwkm_version == QCOM_ICE_HWKM_V1) {
>> +		regval = qcom_ice_readl(ice, QCOM_ICE_REG_HWKM_TZ_KM_CTL);
>> +		regval &= ~QCOM_ICE_HWKM_ICE_LEGACY_MODE_ENABLED;
>> +		qcom_ice_writel(ice, regval, QCOM_ICE_REG_HWKM_TZ_KM_CTL);
>> +	} else {
>> +		dev_err(ice->dev, "Invalid HWKM version\n");
>> +		return;
>> +	}
> 
> The else path is impossible to reach
. I will remove it in next revision.
> 
>>  
>>  	/* Disable CRC checks.  This HWKM feature is not used. */
>>  	qcom_ice_writel(ice, QCOM_ICE_HWKM_DISABLE_CRC_CHECKS_VAL,
>> @@ -298,7 +335,8 @@ EXPORT_SYMBOL_GPL(qcom_ice_suspend);
>>  
>>  static unsigned int translate_hwkm_slot(struct qcom_ice *ice, unsigned int slot)
>>  {
>> -	return slot * 2;
>> +	/* The slot offset depends upon HWKM version */
>> +	return (ice->hwkm_version == QCOM_ICE_HWKM_V1) ? (slot) : (slot * 2);
> 
> The parentheses are unnecessary
Ok. Will remove in next revision.
> 
> Konrad
> 
Thanks,
Neeraj