Re: [PATCH 2/3] powerpc/secvar: Expose secvars relevant to the key management mode

[Srish Srinivasan <ssrish@linux.ibm.com>]

On 5/5/25 12:53 PM, Andrew Donnellan wrote:
> On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
>> The PLPKS enabled PowerVM LPAR sysfs exposes all of the secure boot
>> secvars irrespective of the key management mode.
>>
>> The PowerVM LPAR supports static and dynamic key management for
>> secure
>> boot. The key management option can be updated in the management
>> console. Only in the dynamic key mode can the user modify the secure
>> boot secvars db, dbx, grubdb, grubdbx, and sbat, which are exposed
>> via
>> the sysfs interface. But the sysfs interface exposes these secvars
>> even
>> in the static key mode. This could lead to errors when reading them
>> or
>> writing to them in the static key mode.
>>
>> Expose only PK, trustedcadb, and moduledb in the static key mode to
>> enable loading of signed third-party kernel modules.
>>
>> Co-developed-by: Souradeep <soura@imap.linux.ibm.com>
>> Signed-off-by: Souradeep <soura@imap.linux.ibm.com>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> I'm assuming it's been determined that there's no value in letting
> userspace see db/dbx/etc in a read-only way in static mode?
>
> With one comment below:
>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Hi Andrew,
Thanks a lot for your feedback.

Yes, that is correct.
>> ---
>>   Documentation/ABI/testing/sysfs-secvar        |  9 ++++--
>>   arch/powerpc/platforms/pseries/plpks-secvar.c | 28 ++++++++++++++++-
>> --
>>   2 files changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-secvar
>> b/Documentation/ABI/testing/sysfs-secvar
>> index 857cf12b0904..2bdc7d9c0c10 100644
>> --- a/Documentation/ABI/testing/sysfs-secvar
>> +++ b/Documentation/ABI/testing/sysfs-secvar
>> @@ -22,9 +22,12 @@ Description:	A string indicating which backend is
>> in use by the firmware.
>>   		and is expected to be "ibm,edk2-compat-v1".
>>   
>>   		On pseries/PLPKS, this is generated by the kernel
>> based on the
>> -		version number in the SB_VERSION variable in the
>> keystore, and
>> -		has the form "ibm,plpks-sb-v<version>", or
>> -		"ibm,plpks-sb-unknown" if there is no SB_VERSION
>> variable.
>> +		existence of the SB_VERSION property in firmware.
>> This string
>> +		takes the form "ibm,plpks-sb-v1" in the presence of
>> SB_VERSION,
>> +		indicating the key management mode is dynamic.
>> Otherwise it
>> +		takes the form "ibm,plpks-sb-v0" in the static key
>> management
>> +		mode. Only secvars relevant to the key management
>> mode are
>> +		exposed.
> Everything except the last sentence here is relevant to the previous
> patch in the series (noting my comments on the previous patch about the
> string).
>
> The last sentence is more related to the <variable name> entry than the
> format entry, and perhaps worth including a list of what variables are
> applicable to each mode.
Sure, will fix this.

Thanks and Regards,
Srish
>
>>   
>>   What:		/sys/firmware/secvar/vars/<variable name>
>>   Date:		August 2019
>> diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c
>> b/arch/powerpc/platforms/pseries/plpks-secvar.c
>> index d57067a733ab..cbcb2c356f2a 100644
>> --- a/arch/powerpc/platforms/pseries/plpks-secvar.c
>> +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
>> @@ -59,7 +59,14 @@ static u32 get_policy(const char *name)
>>   		return PLPKS_SIGNEDUPDATE;
>>   }
>>   
>> -static const char * const plpks_var_names[] = {
>> +static const char * const plpks_var_names_static[] = {
>> +	"PK",
>> +	"moduledb",
>> +	"trustedcadb",
>> +	NULL,
>> +};
>> +
>> +static const char * const plpks_var_names_dynamic[] = {
>>   	"PK",
>>   	"KEK",
>>   	"db",
>> @@ -207,21 +214,34 @@ static int plpks_max_size(u64 *max_size)
>>   	return 0;
>>   }
>>   
>> +static const struct secvar_operations plpks_secvar_ops_static = {
>> +	.get = plpks_get_variable,
>> +	.set = plpks_set_variable,
>> +	.format = plpks_secvar_format,
>> +	.max_size = plpks_max_size,
>> +	.config_attrs = config_attrs,
>> +	.var_names = plpks_var_names_static,
>> +};
>>   
>> -static const struct secvar_operations plpks_secvar_ops = {
>> +static const struct secvar_operations plpks_secvar_ops_dynamic = {
>>   	.get = plpks_get_variable,
>>   	.set = plpks_set_variable,
>>   	.format = plpks_secvar_format,
>>   	.max_size = plpks_max_size,
>>   	.config_attrs = config_attrs,
>> -	.var_names = plpks_var_names,
>> +	.var_names = plpks_var_names_dynamic,
>>   };
>>   
>>   static int plpks_secvar_init(void)
>>   {
>> +	u8 mode;
>> +
>>   	if (!plpks_is_available())
>>   		return -ENODEV;
>>   
>> -	return set_secvar_ops(&plpks_secvar_ops);
>> +	mode = plpks_get_sb_keymgmt_mode();
>> +	if (mode)
>> +		return set_secvar_ops(&plpks_secvar_ops_dynamic);
>> +	return set_secvar_ops(&plpks_secvar_ops_static);
>>   }
>>   machine_device_initcall(pseries, plpks_secvar_init);