Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 08:46:52 +0100 (CET)
From:   Nikolaus Voss <nv@vosn.de>
To:     Ahmad Fatoum <a.fatoum@pengutronix.de>
cc:     Horia Geanta <horia.geanta@nxp.com>,
        Pankaj Gupta <pankaj.gupta@nxp.com>,
        Gaurav Jain <gaurav.jain@nxp.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        David Gstir <david@sigma-star.at>,
        Steffen Trumtrar <s.trumtrar@pengutronix.de>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] crypto: caam: blob_gen.c: warn if key is insecure
In-Reply-To: <b1d90113-c69a-108c-7142-6a47db001a96@pengutronix.de>
Message-ID: <c4e399c0-ac28-767d-e0ee-f95c573c3fa7@vosn.de>
References: <20221019144108.48E1B1B0E@mail.steuer-voss.de> <b1d90113-c69a-108c-7142-6a47db001a96@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Precedence: bulk

On Fri, 4 Nov 2022, Ahmad Fatoum wrote:

> Hello Nikolaus,
>
> On 19.10.22 14:44, Nikolaus Voss wrote:
>> If CAAM is not in "trusted" or "secure" state, a fixed non-volatile key
>> is used instead of the unique device key. This is the default mode of
>> operation without secure boot (HAB). In this scenario, CAAM encrypted
>> blobs should be used only for testing but not in a production
>> environment, so issue a warning.
>
> Thanks for your patch.
>
>> Signed-off-by: Nikolaus Voss <nikolaus.voss@haag-streit.com>
>> ---
>>  drivers/crypto/caam/blob_gen.c | 8 ++++++++
>>  drivers/crypto/caam/regs.h     | 3 +++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
>> index 6345c7269eb0..f3e741393f65 100644
>> --- a/drivers/crypto/caam/blob_gen.c
>> +++ b/drivers/crypto/caam/blob_gen.c
>> @@ -6,6 +6,7 @@
>>
>>  #define pr_fmt(fmt) "caam blob_gen: " fmt
>>
>> +#include <linux/bitfield.h>
>>  #include <linux/device.h>
>>  #include <soc/fsl/caam-blob.h>
>>
>> @@ -62,11 +63,13 @@ int caam_process_blob(struct caam_blob_priv *priv,
>>  		      struct caam_blob_info *info, bool encap)
>
> I agree with Herbert that this may not be the best place. I think
> a single warning during caam_blob_gen_init() would suffice.
>
>>  {
>>  	struct caam_blob_job_result testres;
>> +	const struct caam_drv_private *ctrlpriv;
>>  	struct device *jrdev = &priv->jrdev;
>>  	dma_addr_t dma_in, dma_out;
>>  	int op = OP_PCLID_BLOB;
>>  	size_t output_len;
>>  	u32 *desc;
>> +	u32 moo;
>>  	int ret;
>>
>>  	if (info->key_mod_len > CAAM_BLOB_KEYMOD_LENGTH)
>> @@ -100,6 +103,11 @@ int caam_process_blob(struct caam_blob_priv *priv,
>>  		goto out_unmap_in;
>>  	}
>>
>> +	ctrlpriv = dev_get_drvdata(jrdev->parent);
>> +	moo = FIELD_GET(CSTA_MOO, ctrlpriv->ctrl->perfmon.status);
>> +	if (moo != CSTA_MOO_SECURE && moo != CSTA_MOO_TRUSTED)
>> +		dev_warn(jrdev, "using insecure test key!\n");
>
> I'd make the warning a bit more verbose, e.g.
>
>  "device not configured for trusted/secure mode: using insecure test key!"

I agree.

>
>> +
>>  	/*
>>  	 * A data blob is encrypted using a blob key (BK); a random number.
>>  	 * The BK is used as an AES-CCM key. The initial block (B0) and the
>> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
>> index 66d6dad841bb..b7de5fb5e056 100644
>> --- a/drivers/crypto/caam/regs.h
>> +++ b/drivers/crypto/caam/regs.h
>> @@ -426,6 +426,9 @@ struct caam_perfmon {
>>  	u32 rsvd2;
>>  #define CSTA_PLEND		BIT(10)
>>  #define CSTA_ALT_PLEND		BIT(18)
>> +#define CSTA_MOO		GENMASK(9, 8)
>> +#define CSTA_MOO_SECURE	1
>> +#define CSTA_MOO_TRUSTED	3
>
> I just checked the i.MX6 and LS1046 security reference manuals and both
> have Trusted as 2 (10b). 3 is fail. Does you SoC differ? Either way, please
> note what SoC you were testing on in the commit message.

You're right, I will correct that.

Niko