| 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
| ||
|
Message-ID: <3fe59767-954b-4c2c-a602-9801f1c1080a@linux.ibm.com>
Date: Tue, 6 May 2025 15:27:34 -0400
From: Nayna Jain <nayna@...ux.ibm.com>
To: Andrew Donnellan <ajd@...ux.ibm.com>,
Srish Srinivasan <ssrish@...ux.ibm.com>,
linux-integrity@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Cc: maddy@...ux.ibm.com, mpe@...erman.id.au, npiggin@...il.com,
christophe.leroy@...roup.eu, naveen@...nel.org, zohar@...ux.ibm.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] powerpc/pseries: Correct secvar format representation
for static key management
On 5/5/25 4:36 AM, Andrew Donnellan wrote:
> On Wed, 2025-04-30 at 14:33 +0530, Srish Srinivasan wrote:
>> On a PLPKS enabled PowerVM LPAR, the secvar format property for
>> static
>> key management is misrepresented as "ibm,plpks-sb-unknown", creating
>> reason for confusion.
>>
>> Static key management mode uses fixed, built-in keys. Dynamic key
>> management mode allows keys to be updated in production to handle
>> security updates without firmware rebuilds.
>>
>> Define a function named plpks_get_sb_keymgmt_mode() to retrieve the
>> key management mode based on the existence of the SB_VERSION property
>> in the firmware.
>>
>> Set the secvar format property to either "ibm,plpks-sb-v1" or
>> "ibm,plpks-sb-v0" based on the key management mode, and return the
>> length of the secvar format property.
>>
>> Co-developed-by: Souradeep <soura@...p.linux.ibm.com>
>> Signed-off-by: Souradeep <soura@...p.linux.ibm.com>
>> Signed-off-by: Srish Srinivasan <ssrish@...ux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@...ux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@...ux.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/plpks-secvar.c | 70 +++++++++++------
>> --
>> 1 file changed, 40 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c
>> b/arch/powerpc/platforms/pseries/plpks-secvar.c
>> index 257fd1f8bc19..d57067a733ab 100644
>> --- a/arch/powerpc/platforms/pseries/plpks-secvar.c
>> +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c
>> @@ -152,39 +152,49 @@ static int plpks_set_variable(const char *key,
>> u64 key_len, u8 *data,
>> return rc;
>> }
>>
>> -// PLPKS dynamic secure boot doesn't give us a format string in the
>> same way OPAL does.
>> -// Instead, report the format using the SB_VERSION variable in the
>> keystore.
>> -// The string is made up by us, and takes the form "ibm,plpks-sb-
>> v<n>" (or "ibm,plpks-sb-unknown"
>> -// if the SB_VERSION variable doesn't exist). Hypervisor defines the
>> SB_VERSION variable as a
>> -// "1 byte unsigned integer value".
>> -static ssize_t plpks_secvar_format(char *buf, size_t bufsize)
>> +/*
>> + * Return the key management mode.
>> + *
>> + * SB_VERSION is defined as a "1 byte unsigned integer value". It is
>> owned by
>> + * the Partition Firmware and its presence indicates that the key
>> management
>> + * mode is dynamic. Only signed variables have null bytes in their
>> names.
>> + * SB_VERSION does not.
>> + *
>> + * Return 1 to indicate that the key management mode is dynamic.
>> Otherwise
>> + * return 0, indicating that the key management mode is static.
>> + */
> This description isn't accurate.
>
> For dynamic mode, it doesn't return 1, it returns whatever version is
> defined in SB_VERSION, which could be 1, or could at some later point be
> a higher version.
>
> Which makes the function name a bit of a misnomer too - it is returning
> the version number, just the version number can now be zero.
>
>> +static u8 plpks_get_sb_keymgmt_mode(void)
>> {
>> - struct plpks_var var = {0};
>> - ssize_t ret;
>> - u8 version;
>> -
>> - var.component = NULL;
>> - // Only the signed variables have null bytes in their names,
>> this one doesn't
>> - var.name = "SB_VERSION";
>> - var.namelen = strlen(var.name);
>> - var.datalen = 1;
>> - var.data = &version;
>> -
>> - // Unlike the other vars, SB_VERSION is owned by firmware
>> instead of the OS
>> - ret = plpks_read_fw_var(&var);
>> - if (ret) {
>> - if (ret == -ENOENT) {
>> - ret = snprintf(buf, bufsize, "ibm,plpks-sb-
>> unknown");
>> - } else {
>> - pr_err("Error %ld reading SB_VERSION from
>> firmware\n", ret);
>> - ret = -EIO;
>> - }
>> - goto err;
>> + u8 mode;
>> + ssize_t rc;
>> + struct plpks_var var = {
>> + .component = NULL,
>> + .name = "SB_VERSION",
>> + .namelen = 10,
>> + .datalen = 1,
>> + .data = &mode,
>> + };
>> +
>> + rc = plpks_read_fw_var(&var);
>> + if (rc) {
>> + pr_info("Error %ld reading SB_VERSION from
>> firmware\n", rc);
> We need to check for -ENOENT, otherwise this message is going to be
> printed every time you boot a machine in static mode.
>
> I think you should handle this as the existing code does: if it's
> ENOENT, return 0, and for other codes print an error and return -EIO.
>
>> + mode = 0;
>> }
>> + return mode;
>> +}
>>
>> - ret = snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", version);
>> -err:
>> - return ret;
>> +// PLPKS dynamic secure boot doesn't give us a format string in the
>> same way
>> +// OPAL does. Instead, report the format using the SB_VERSION
>> variable in the
>> +// keystore. The string, made up by us, takes the form "ibm,plpks-
>> sb-v<n>".Set
>> +// the secvar format property to either "ibm,plpks-sb-v1" or
>> "ibm,plpks-sb-v0",
>> +// based on the key management mode, and return the length of the
>> secvar format
>> +// property.
>> +static ssize_t plpks_secvar_format(char *buf, size_t bufsize)
>> +{
>> + u8 mode;
>> +
>> + mode = plpks_get_sb_keymgmt_mode();
>> + return snprintf(buf, bufsize, "ibm,plpks-sb-v%hhu", mode);
> It might be better to use something like "ibm,plpks-sb-static" in place
> of "ibm,plpks-sb-v0" to make it instantly clear that static mode
> doesn't use the same version numbering scheme as dynamic mode.
Yes, "ibm,plpks-sb-static" is more clear compared to "ibm,plpks-sb-v0".
However, I am not sure why "static mode doesn't use the same version
numbering scheme as dynamic mode". Infact, as per my understanding, it
is part of same versioning system. "0 represent static, 1 represent
dynamic and anything beyond 1 would mean dynamic with additional features".
Also, wouldn't having "ibm,pkpks-sb-static" and then "ibm,pkpk-sb-v1"
for dynamic would be bit confusing? I mean being static is clear, but
what they relate v1 to? Or did you mean to have "ibm,plpks-sb-static"
and "ibm,plpks-sb-dynamic" for the two modes?
Thanks & Regards,
- Nayna
Powered by blists - more mailing lists