On 11/5/2019 12:24 AM, Eric Richter wrote:

From: Nayna Jain <na...@linux.ibm.com>

The X.509 certificates trusted by the platform and required to secure boot
the OS kernel are wrapped in secure variables, which are controlled by
OPAL.

This patch adds firmware/kernel interface to read and write OPAL secure
variables based on the unique key.

I feel splitting this patch into smaller set of changes would make it easier to review. For instance roughly as below:

 1, opal-api.h which adds the #defines  OPAL_SECVAR_ and the API signature.
 2, secvar.h then adds secvar_operations struct
 3, powerpc/kernel for the Interface definitions
 4, powernv/opal-secvar.c for the API implementations
 5, powernv/opal-call.c for the API calls
 6, powernv/opal.c for the secvar init calls.

diff --git a/arch/powerpc/include/asm/opal-api.h 
b/arch/powerpc/include/asm/opal-api.h
index 378e3997845a..c1f25a760eb1 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -211,7 +211,10 @@
  #define OPAL_MPIPL_UPDATE                     173
  #define OPAL_MPIPL_REGISTER_TAG                       174
  #define OPAL_MPIPL_QUERY_TAG                  175
-#define OPAL_LAST                              175
+#define OPAL_SECVAR_GET                                176
+#define OPAL_SECVAR_GET_NEXT                   177
+#define OPAL_SECVAR_ENQUEUE_UPDATE             178
+#define OPAL_LAST                              178

Please fix the indentation for the #defines


+static int opal_get_variable(const char *key, uint64_t ksize,
+                            u8 *data, uint64_t *dsize)
+{
+       int rc;
+
+       if (!key || !dsize)
+               return -EINVAL;
+
+       *dsize = cpu_to_be64(*dsize);
+
+       rc = opal_secvar_get(key, ksize, data, dsize);
+
+       *dsize = be64_to_cpu(*dsize);

Should the return status (rc) from opal_secvar_get be checked before attempting to do the conversion (be64_to_cpu)?

+static int opal_get_next_variable(const char *key, uint64_t *keylen,
+                                 uint64_t keybufsize)
+{
+       int rc;
+
+       if (!key || !keylen)
+               return -EINVAL;
+
+       *keylen = cpu_to_be64(*keylen);
+
+       rc = opal_secvar_get_next(key, keylen, keybufsize);
+
+       *keylen = be64_to_cpu(*keylen);

Same comment as above - should rc be checke before attempting to convert?

+
+       return opal_status_to_err(rc);
+}
+
+static int opal_set_variable(const char *key, uint64_t ksize, u8 *data,
+                            uint64_t dsize)
+{
+       int rc;
+
+       if (!key || !data)
+               return -EINVAL;

Is the key and data received here from a trusted caller? If not, should there be some validation checks done here before enqueuing the data?

 -lakshmi

Reply via email to