Re: [PATCH v6 1/4] powerpc/powernv: Add OPAL API interface to access secure variable

[Lakshmi Ramasubramanian]

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

> From: Nayna Jain <nayna@xxxxxxxxxxxxx>
>
> 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