Comments inline.
Reviewed-by: Greg Joyce <gjo...@linux.vnet.ibm.com>
________________________________
From: Nayna Jain <na...@linux.ibm.com>
Sent: Tuesday, July 12, 2022 7:59 PM
To: linuxppc-dev@lists.ozlabs.org <linuxppc-dev@lists.ozlabs.org>
Cc: Michael Ellerman <m...@ellerman.id.au>; Benjamin Herrenschmidt
<b...@kernel.crashing.org>; Paul Mackerras <pau...@samba.org>; George Wilson
<gcwil...@linux.ibm.com>; Gregory Joyce <gjo...@ibm.com>; Nayna Jain
<na...@linux.ibm.com>
Subject: [PATCH 1/2] powerpc/pseries: define driver for Platform KeyStore
PowerVM provides an isolated Platform Keystore(PKS) storage allocation
for each LPAR with individually managed access controls to store
sensitive information securely. It provides a new set of hypervisor
calls for Linux kernel to access PKS storage.
Define PLPKS driver using H_CALL interface to access PKS storage.
Signed-off-by: Nayna Jain <na...@linux.ibm.com>
---
+
+static int construct_auth(u8 consumer, struct plpks_auth **auth)
+{
+ pr_debug("max password size is %u\n", config->maxpwsize);
Are the pr_debugs in this function still needed?
+
+ if (!auth || consumer > 3)
+ return -EINVAL;
+
+ *auth = kmalloc(struct_size(*auth, password, config->maxpwsize),
+ GFP_KERNEL);
+ if (!*auth)
+ return -ENOMEM;
+
+ (*auth)->version = 1;
+ (*auth)->consumer = consumer;
+ (*auth)->rsvd0 = 0;
+ (*auth)->rsvd1 = 0;
+ if (consumer == PKS_FW_OWNER || consumer == PKS_BOOTLOADER_OWNER) {
+ pr_debug("consumer is bootloader or firmware\n");
+ (*auth)->passwordlength = 0;
+ return 0;
+ }
+
+ (*auth)->passwordlength = (__force __be16)ospasswordlength;
+
+ memcpy((*auth)->password, ospassword,
+ flex_array_size(*auth, password,
+ (__force u16)((*auth)->passwordlength)));
+ (*auth)->passwordlength = cpu_to_be16((__force
u16)((*auth)->passwordlength));
+
+ return 0;
+}
+
+/**
+ * Label is combination of label attributes + name.
+ * Label attributes are used internally by kernel and not exposed to the user.
+ */
+static int construct_label(char *component, u8 varos, u8 *name, u16 namelen,
u8 **label)
+{
+ int varlen;
+ int len = 0;
+ int llen = 0;
+ int i;
+ int rc = 0;
+ u8 labellength = MAX_LABEL_ATTR_SIZE;
+
+ if (!label)
+ return -EINVAL;
+
+ varlen = namelen + sizeof(struct label_attr);
+ *label = kzalloc(varlen, GFP_KERNEL);
+
+ if (!*label)
+ return -ENOMEM;
+
+ if (component) {
+ len = strlen(component);
+ memcpy(*label, component, len);
+ }
+ llen = len;
+
I guess the 8, 1, and 5 are field lengths. Could they be a define or sizeof?
+ if (component)
+ len = 8 - strlen(component);
+ else
+ len = 8;
+
+ memset(*label + llen, 0, len);
+ llen = llen + len;
+
+ ((*label)[llen]) = 0;
+ llen = llen + 1;
+
+ memcpy(*label + llen, &varos, 1);
+ llen = llen + 1;
+
+ memcpy(*label + llen, &labellength, 1);
+ llen = llen + 1;
+
+ memset(*label + llen, 0, 5);
+ llen = llen + 5;
+
+ memcpy(*label + llen, name, namelen);
+ llen = llen + namelen;
+
+ for (i = 0; i < llen; i++)
+ pr_debug("%c", (*label)[i]);
+
+ rc = llen;
+ return rc;
+}
+
+static int _plpks_get_config(void)
+{
+ unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = {0};
+ int rc;
+ size_t size = sizeof(struct plpks_config);
+
+ config = kzalloc(size, GFP_KERNEL);
+ if (!config)
+ return -ENOMEM;
+
+ rc = plpar_hcall(H_PKS_GET_CONFIG,
+ retbuf,
+ virt_to_phys(config),
+ size);
+
+ if (rc != H_SUCCESS)
Free config before returning the error.
+ return pseries_status_to_err(rc);
+
+ config->rsvd0 = be32_to_cpu((__force __be32)config->rsvd0);
+ config->maxpwsize = be16_to_cpu((__force __be16)config->maxpwsize);
+ config->maxobjlabelsize = be16_to_cpu((__force
__be16)config->maxobjlabelsize);
+ config->maxobjsize = be16_to_cpu((__force __be16)config->maxobjsize);
+ config->totalsize = be32_to_cpu((__force __be32)config->totalsize);
+ config->usedspace = be32_to_cpu((__force __be32)config->usedspace);
+ config->supportedpolicies = be32_to_cpu((__force
__be32)config->supportedpolicies);
+ config->rsvd1 = be64_to_cpu((__force __be64)config->rsvd1);
+
+ configset = true;
+
+ return 0;
+}
+
+static int plpks_confirm_object_flushed(u8 *label, u16 labellen)
+{
+ unsigned long retbuf[PLPAR_HCALL_BUFSIZE] = {0};
+ int rc;
+ u64 timeout = 0;
+ struct plpks_auth *auth;
+ u8 status;
+ int i;
+
Deleted pr_debugs? I guess this is a general question for all pr_debugs in this
file.
+ rc = construct_auth(PKS_OS_OWNER, &auth);
+ if (rc)
+ return rc;
+
+ for (i = 0; i < labellen; i++)
+ pr_debug("%02x ", label[i]);
+
+ do {
+ rc = plpar_hcall(H_PKS_CONFIRM_OBJECT_FLUSHED,
+ retbuf,
+ virt_to_phys(auth),
+ virt_to_phys(label),
+ labellen);
+
+ status = retbuf[0];
+ if (rc) {
+ pr_info("rc is %d, status is %d\n", rc, status);
+ if (rc == H_NOT_FOUND && status == 1)
+ rc = 0;
+ break;
+ }
+
+ pr_debug("rc is %d, status is %d\n", rc, status);
+
+ if (!rc && status == 1)
+ break;
+
+ usleep_range(PKS_FLUSH_SLEEP, PKS_FLUSH_SLEEP +
PKS_FLUSH_SLEEP_RANGE);
+ timeout = timeout + PKS_FLUSH_SLEEP;
+ pr_debug("timeout is %llu\n", timeout);
+
+ } while (timeout < PKS_FLUSH_MAX_TIMEOUT);
+
+ rc = pseries_status_to_err(rc);
+
+ kfree(auth);
+
+ return rc;
+}
+
+EXPORT_SYMBOL(plpks_remove_var);
XPORT_SYMBOL(plpks_get_config);
+
+static __init int pseries_plpks_init(void)
+{
+ int rc = 0;
+
+ rc = _plpks_get_config();
+
+ if (rc) {
I think this pr_err would be better if provided more info like the pr_info
below.
+ pr_err("Error initializing plpks\n");
+ return rc;
+ }
+
+ rc = plpks_gen_password();
+ if (rc) {
+ if (rc == H_IN_USE) {
+ rc = 0;
+ } else {
I think this pr_err would be better if provided more info like the pr_info
below.
+ pr_err("Failed setting password %d\n", rc);
+ rc = pseries_status_to_err(rc);
+ return rc;
+ }
+ }
+
+ pr_info("POWER LPAR Platform Keystore initialized successfully\n");
+
+ return rc;
+}
+arch_initcall(pseries_plpks_init);
--
2.27.0
