Re: [PATCH 3/4] crypto: caam - add in-kernel interface for blob generator

From: David Gstir <david@sigma-star.at>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: "Horia Geantă" <horia.geanta@nxp.com>,
	"Aymen Sghaier" <aymen.sghaier@nxp.com>,
	"Herbert Xu" <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	kernel@pengutronix.de, "James Bottomley" <jejb@linux.ibm.com>,
	"Jarkko Sakkinen" <jarkko@kernel.org>,
	"Mimi Zohar" <zohar@linux.ibm.com>,
	"David Howells" <dhowells@redhat.com>,
	"James Morris" <jmorris@namei.org>,
	"Eric Biggers" <ebiggers@kernel.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	"Udit Agarwal" <udit.agarwal@nxp.com>,
	"Jan Luebbe" <j.luebbe@pengutronix.de>,
	"Richard Weinberger" <richard@nod.at>,
	"Franck LENORMAND" <franck.lenormand@nxp.com>,
	"Sumit Garg" <sumit.garg@linaro.org>,
	linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH 3/4] crypto: caam - add in-kernel interface for blob generator
Date: Tue, 10 Aug 2021 11:29:04 +0000	[thread overview]
Message-ID: <796E18E6-1329-40D6-B12F-4CE6C90DD988@sigma-star.at> (raw)
In-Reply-To: <4078060ab2e44114af8204b4defea4f3d4b9e285.1626885907.git-series.a.fatoum@pengutronix.de>

Hi Ahmad,

> On 21.07.2021, at 18:48, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

[...]

> diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
> new file mode 100644
> index 000000000000..513d3f90e438
> --- /dev/null
> +++ b/drivers/crypto/caam/blob_gen.c
> @@ -0,0 +1,230 @@

[...]

> +
> +int caam_encap_blob(struct caam_blob_priv *priv, const char *keymod,
> +		    void *input, void *output, size_t length)
> +{
> +	u32 *desc;
> +	struct device *jrdev = &priv->jrdev;
> +	dma_addr_t dma_in, dma_out;
> +	struct caam_blob_job_result testres;
> +	size_t keymod_len = strlen(keymod);
> +	int ret;
> +
> +	if (length <= CAAM_BLOB_OVERHEAD || keymod_len > CAAM_BLOB_KEYMOD_LENGTH)

The docs for this function mention the length <= CAAM_BLOB_MAX_LEN
restriction. This is not checked here. Is this intended?

Since you already assert that MAX_BLOB_SIZE <= CAAM_BLOB_MAX_LEN
in security/keys/trusted-keys/trusted_caam.c, this will never
be an issue for CAAM-based trusted-keys though.

> +		return -EINVAL;
> +
> +	desc = caam_blob_alloc_desc(keymod_len);
> +	if (!desc) {
> +		dev_err(jrdev, "unable to allocate desc\n");
> +		return -ENOMEM;
> +	}
> +

[...]

> diff --git a/include/soc/fsl/caam-blob.h b/include/soc/fsl/caam-blob.h
> new file mode 100644
> index 000000000000..aebbc9335f64
> --- /dev/null
> +++ b/include/soc/fsl/caam-blob.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
> + */
> +
> +#ifndef __CAAM_BLOB_GEN
> +#define __CAAM_BLOB_GEN
> +
> +#include <linux/types.h>
> +
> +#define CAAM_BLOB_KEYMOD_LENGTH		16
> +#define CAAM_BLOB_OVERHEAD		(32 + 16)
> +#define CAAM_BLOB_MAX_LEN		4096
> +
> +struct caam_blob_priv;
> +
> +/** caam_blob_gen_init - initialize blob generation
> + *
> + * returns either pointer to new caam_blob_priv instance
> + * or error pointer
> + */
> +struct caam_blob_priv *caam_blob_gen_init(void);
> +
> +/** caam_blob_gen_init - free blob generation resources

s/init/exit/

> + *
> + * @priv: instance returned by caam_blob_gen_init
> + */
> +void caam_blob_gen_exit(struct caam_blob_priv *priv);

Except these minor things, I noticed no issues with this whole series:

Reviewed-by: David Gstir <david@sigma-star.at>