Re: [PATCH 1/2] drivers: caam: add RNG software self-test

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Roland,

On Sun, Nov 25, 2018 at 11:59:51PM +0100, Roland Hieber wrote:
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 9e62bd6fd6..14dfc7e827 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -19,10 +19,13 @@
>  #include "desc_constr.h"
>  #include "error.h"
>  #include "ctrl.h"
> +#include "rng_self_test.h"
>  
>  bool caam_little_end;
>  EXPORT_SYMBOL(caam_little_end);
>  
> +extern bool habv4_need_rng_software_self_test;

Put the declaration into include/hab.h

> +
>  /*
>   * Descriptor to instantiate RNG State Handle 0 in normal mode and
>   * load the JDKEK, TDKEK and TDSK registers
> @@ -570,6 +573,25 @@ static int caam_probe(struct device_d *dev)
>  
>  	cha_vid_ls = rd_reg32(&ctrl->perfmon.cha_id_ls);
>  
> +	/* habv4_need_rng_software_self_test is determined by habv4_get_status() */
> +	if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST) &&
> +	    habv4_need_rng_software_self_test) {
> +		u8 caam_era;
> +		u8 rngvid;
> +		u8 rngrev;
> +
> +		caam_era = (rd_reg32(&ctrl->perfmon.ccb_id) & CCBVID_ERA_MASK) >> CCBVID_ERA_SHIFT;
> +		rngvid = (cha_vid_ls & CHAVID_LS_RNGVID_MASK) >> CHAVID_LS_RNGVID_SHIFT;
> +		rngrev = (rd_reg32(&ctrl->perfmon.cha_rev_ls) & CRNR_LS_RNGRN_MASK) >> CRNR_LS_RNGRN_SHIFT;
> +
> +		pr_info("detected failure in CAAM RNG ROM self-test, running software self-test\n");

This looks unnecessary. You already printed the HAB failure event and
the rng_self_test() will output the result.

> +		ret = rng_self_test(ctrlpriv->jrpdev[0], caam_era, rngvid, rngrev);
> +		if (ret != 0) {
> +			caam_remove(dev);
> +			return ret;
> +		}
> +	}
> +
>  	/*
>  	 * If SEC has RNG version >= 4 and RNG state handle has not been
>  	 * already instantiated, do RNG instantiation
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 6c9d6d75a0..19e7d6d7e4 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -279,6 +279,8 @@ struct caam_perfmon {
>  
>  	/* CAAM Hardware Instantiation Parameters		fa0-fbf */
>  	u32 cha_rev_ms;		/* CRNR - CHA Rev No. Most significant half*/
> +#define CRNR_LS_RNGRN_SHIFT	16
> +#define CRNR_LS_RNGRN_MASK	(0xfull << CRNR_LS_RNGRN_SHIFT)
>  	u32 cha_rev_ls;		/* CRNR - CHA Rev No. Least significant half*/
>  #define CTPR_MS_QI_SHIFT	25
>  #define CTPR_MS_QI_MASK		(0x1ull << CTPR_MS_QI_SHIFT)
> @@ -311,6 +313,8 @@ struct caam_perfmon {
>  #define CCBVID_ERA_SHIFT	24
>  	u32 ccb_id;		/* CCBVID - CCB Version ID	*/
>  	u32 cha_id_ms;		/* CHAVID - CHA Version ID Most Significant*/
> +#define CHAVID_LS_RNGVID_SHIFT	16
> +#define CHAVID_LS_RNGVID_MASK	(0xfull << CHAVID_LS_RNGVID_SHIFT)
>  	u32 cha_id_ls;		/* CHAVID - CHA Version ID Least Significant*/
>  	u32 cha_num_ms;		/* CHANUM - CHA Number Most Significant	*/
>  	u32 cha_num_ls;		/* CHANUM - CHA Number Least Significant*/
> diff --git a/drivers/crypto/caam/rng_self_test.c b/drivers/crypto/caam/rng_self_test.c
> new file mode 100644
> index 0000000000..aff8f26cfd
> --- /dev/null
> +++ b/drivers/crypto/caam/rng_self_test.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2018 NXP
> + * Copyright (C) 2018 Pengutronix, Roland Hieber <r.hieber@pengutronix.de>
> + *
> + * CAAM RNG self-test -- based on the vendor patch for U-Boot:
> + * https://portland.source.codeaurora.org/patches/external/imxsupport/uboot-imx/imx_v2016.03_4.1.15_2.0.0_ga/HAB-238-Run-RNG-self-test-for-impacted-i.MX-chips.zip
> + *
> + * |    From: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> + * |    Subject: [PATCH] HAB-238 Run RNG self test for impacted i.MX chips
> + * |
> + * |    Patch is only applicable to imx_v2016.03_4.1.15_2.0.0_ga branch of u-boot.
> + * |    Please adapt the patch for your respective release version.
> + * |
> + * |    Background:
> + * |    Few i.MX chips which have HAB 4.2.3 or beyond, have oberserved following
> + * |    warning message generated by HAB due to incorrect implementation of drng
> + * |    self test in boot ROM.
> + * |
> + * |        Event       |0xdb|0x0024|0x42| SRCE Field: 69 30 e1 1d
> + * |                    |    |      |    |             STS = HAB_WARNING (0x69)
> + * |                    |    |      |    |             RSN = HAB_ENG_FAIL (0x30)
> + * |                    |    |      |    |             CTX = HAB_CTX_ENTRY (0xE1)
> + * |                    |    |      |    |             ENG = HAB_ENG_CAAM (0x1D)
> + * |                    |    |      |    | Evt Data (hex):
> + * |                    |    |      |    |  00 08 00 02 40 00 36 06 55 55 00 03 00 00 00 00
> + * |                    |    |      |    |  00 00 00 00 00 00 00 00 00 00 00 01
> + * |
> + * |    It is recommended to run this rng self test before any RNG related crypto
> + * |    implementations are done.
> + * [...]
> + * |
> + * |    Signed-off-by: Utkarsh Gupta <utkarsh.gupta@nxp.com>
> + *
> + * Known impacted chips:
> + *
> + * - i.MX6DQ+ silicon revision 1.1
> + * - i.MX6DQ silicon revision 1.6
> + * - i.MX6DLS silicon revision 1.4
> + * - i.MX6SX silicon revision 1.4
> + * - i.MX6UL silicon revision 1.2
> + * - i.MX67SD silicon revision 1.3
> + */
> +
> +#define pr_fmt(fmt) "rng_self_test: " fmt
> +
> +#include <dma.h>
> +#include <common.h>
> +#include "error.h"
> +#include "regs.h"
> +#include "jr.h"
> +
> +static inline u32 sec_in32(void *addr)
> +{
> +	return readl(addr);
> +}

This function is unused.

> +
> +#define DSC1SIZE 0x36
> +#define DSC2SIZE 0x3A
> +#define RESULT_SIZE 32
> +
> +static const u32 rng_dsc1[DSC1SIZE] = {
> +	0xb0800036, 0x04800010, 0x3c85a15b, 0x50a9d0b1,
> +	0x71a09fee, 0x2eecf20b, 0x02800020, 0xb267292e,
> +	0x85bf712d, 0xe85ff43a, 0xa716b7fb, 0xc40bb528,
> +	0x27b6f564, 0x8821cb5d, 0x9b5f6c26, 0x12a00020,
> +	0x0a20de17, 0x6529357e, 0x316277ab, 0x2846254e,
> +	0x34d23ba5, 0x6f5e9c32, 0x7abdc1bb, 0x0197a385,
> +	0x82500405, 0xa2000001, 0x10880004, 0x00000005,
> +	0x12820004, 0x00000020, 0x82500001, 0xa2000001,
> +	0x10880004, 0x40000045, 0x02800020, 0x8f389cc7,
> +	0xe7f7cbb0, 0x6bf2073d, 0xfc380b6d, 0xb22e9d1a,
> +	0xee64fcb7, 0xa2b48d49, 0xdf9bc3a4, 0x82500009,
> +	0xa2000001, 0x10880004, 0x00000005, 0x82500001,
> +	0x60340020, 0xFFFFFFFF, 0xa2000001, 0x10880004,
> +	0x00000005, 0x8250000d
> +};

Making this an array with undetermined size and using ARRAY_SIZE on the
array would make counting the elements unnecessary.

> +
> +static const u32 rng_result1[RESULT_SIZE] = {
> +	0x3a, 0xfe, 0x2c, 0x87, 0xcc, 0xb6, 0x44, 0x49,
> +	0x19, 0x16, 0x9a, 0x74, 0xa1, 0x31, 0x8b, 0xef,
> +	0xf4, 0x86, 0x0b, 0xb9, 0x5e, 0xee, 0xae, 0x91,
> +	0x92, 0xf4, 0xa9, 0x8f, 0xb0, 0x37, 0x18, 0xa4
> +};

same here.

> +
> +static const u32 rng_dsc2[DSC2SIZE] = {
> +	0xb080003a, 0x04800020, 0x27b73130, 0x30b4b10f,
> +	0x7c62b1ad, 0x77abe899, 0x67452301, 0xefcdab89,
> +	0x98badcfe, 0x10325476, 0x02800020, 0x63f757cf,
> +	0xb9165584, 0xc3c1b407, 0xcc4ce8ad, 0x1ffe8a58,
> +	0xfb4fa893, 0xbb5f4af0, 0x3fb946a1, 0x12a00020,
> +	0x56cbcaa5, 0xfff3adad, 0xe804dcbf, 0x9a900c71,
> +	0xa42017e3, 0x826948e2, 0xd0cfeb3e, 0xaf1a136a,
> +	0x82500405, 0xa2000001, 0x10880004, 0x00000005,
> +	0x12820004, 0x00000020, 0x82500001, 0xa2000001,
> +	0x10880004, 0x40000045, 0x02800020, 0x2e882f8a,
> +	0xe929943e, 0x8132c0a8, 0x12037f90, 0x809fbd66,
> +	0x8684ea04, 0x00cbafa7, 0x7b82d12a, 0x82500009,
> +	0xa2000001, 0x10880004, 0x00000005, 0x82500001,
> +	0x60340020, 0xFFFFFFFF, 0xa2000001, 0x10880004,
> +	0x00000005, 0x8250000d
> +};
> +
> +static const u32 rng_result2[RESULT_SIZE] = {
> +	0x76, 0x87, 0x66, 0x4e, 0xd8, 0x1d, 0x1f, 0x43,
> +	0x76, 0x50, 0x85, 0x5d, 0x1e, 0x1d, 0x9d, 0x0f,
> +	0x93, 0x75, 0x83, 0xff, 0x9a, 0x9b, 0x61, 0xa9,
> +	0xa5, 0xeb, 0xa3, 0x28, 0x2a, 0x15, 0xc1, 0x57
> +};

Why is this u32 when only the lower 8 bits are ever used? Changing this
to u8 would allow you to use memcmp to compare the result with the
expeceted one.

> +
> +/*
> + * construct_rng_self_test_jobdesc() - Implement destination address in RNG self test descriptors
> + * Returns zero on success, and negative on error.
> + */
> +static int construct_rng_self_test_jobdesc(u32 *desc, const u32 *rng_st_dsc, u8 *res_addr, int desc_size)
> +{
> +	int result_addr_idx = desc_size - 5;
> +	int i = 0;
> +
> +	if (!desc) {
> +		return -EINVAL;
> +	}

No need to check, this is already done in the caller.

> +
> +	for (i = 0; i < desc_size; i++) {
> +		desc[i] = rng_st_dsc[i];
> +	}
> +
> +	/* Replace destination address in the descriptor */
> +	desc[result_addr_idx] = (u32)res_addr;
> +
> +	pr_vdebug("RNG SELF TEST DESCRIPTOR:\n");
> +	for (i = 0; i < desc_size; i++) {
> +		pr_vdebug("0x%08X\n", desc[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +/* rng_self_test_done() - callback for caam_jr_enqueue */
> +static void rng_self_test_done(struct device_d *dev, u32 *desc, u32 err, void *arg)
> +{
> +	int * job_err = (int *)arg;

unnecessary cast.

> +	BUG_ON(!job_err);

This is unnecessary because you will get a nive crash dump in the next
line when you dereference the NULL pointer.

> +	*job_err = err;
> +}
> +
> +/*
> + * rng_self_test() - Perform RNG self test
> + * Returns zero on success, and negative on error.
> + */
> +int rng_self_test(struct device_d *dev, const u8 caam_era, const u8 rngvid, const u8 rngrev)
> +{

Please add a caam_ prefix.

> +	int ret, i, desc_size = 0, job_err = 0;
> +	const u32 *rng_st_dsc, *exp_result;
> +	u32 *desc = 0;

No need to initialize.

> +	u8 *result = 0;

ditto.

> +	if (caam_era < 8 && rngvid == 4 && rngrev < 3) {
> +		rng_st_dsc = rng_dsc1;
> +		desc_size = DSC1SIZE;
> +		exp_result = rng_result1;
> +	} else if (rngvid != 3) {
> +		rng_st_dsc = rng_dsc2;
> +		desc_size = DSC2SIZE;
> +		exp_result = rng_result2;
> +	} else {
> +		pr_err("Error: Invalid CAAM ERA (%d) or RNG Version ID (%d) or RNG revision (%d)\n",
> +				caam_era, rngvid, rngrev);
> +		return -EINVAL;

Is this test really correct? Basically it says "We accept everything
except rngvid == 3".

> +
> +	result = dma_alloc(sizeof(uint32_t) * RESULT_SIZE);
> +	if (!result) {
> +		pr_err("Not enough memory for result buffer allocation\n");
> +		return -ENOMEM;
> +	}

Please no messages for usual allocation failures.

"result" is never freed.

> +
> +	desc = dma_alloc(sizeof(uint32_t) * desc_size);
> +	if (!desc) {
> +		pr_err("Not enough memory for descriptor allocation\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = construct_rng_self_test_jobdesc(desc, rng_st_dsc, result, desc_size);
> +	if (ret) {
> +		pr_err("Error in Job Descriptor Construction: %d %s\n",
> +				ret, strerror(ret));
> +		goto err;
> +	} else {

Not necessary to put the below code into an else {} path when you error
out above.

> +		dma_sync_single_for_device((unsigned long)desc,
> +				desc_size * sizeof(uint32_t), DMA_TO_DEVICE);
> +		dma_sync_single_for_device((unsigned long)result,
> +				RESULT_SIZE * sizeof(uint32_t), DMA_FROM_DEVICE);
> +
> +		/* wait for job completion */
> +		ret = caam_jr_enqueue(dev, desc, rng_self_test_done, &job_err);
> +		if (ret) {
> +			pr_err("Error while running RNG self-test descriptor: %d %s\n",
> +					ret, strerror(ret));
> +			goto err;
> +		}
> +		if (job_err) {
> +			pr_err("Job Error:\n");
> +			caam_jr_strstatus(dev, job_err);
> +			goto err;
> +		}
> +	}
> +
> +	dma_sync_single_for_cpu((unsigned long)result, RESULT_SIZE * sizeof(uint32_t),
> +			DMA_FROM_DEVICE);
> +
> +	pr_vdebug("Result\n");
> +	for (i = 0; i < RESULT_SIZE; i++) {
> +		pr_vdebug("%02X\n", result[i]);
> +	}
> +
> +	pr_vdebug("Expected Result:\n");
> +	for (i = 0; i < RESULT_SIZE; i++) {
> +		pr_vdebug("%02X\n", exp_result[i]);
> +	}

Use memory_display to display this. Also this is only interesting if
both differ, so better print it in the failure path.

> +
> +	for (i = 0; i < RESULT_SIZE; i++) {
> +		if (result[i] != exp_result[i]) {
> +			pr_err("RNG self-test failed with unexpected result\n");
> +			ret = -ERANGE;
> +			goto err;
> +		}
> +	}

memcmp, as mentioned above.

> +	pr_notice("RNG software self-test passed\n");
> +	ret = 0;
> +
> +err:
> +	dma_free(desc);
> +	return ret;
> +}
> diff --git a/drivers/crypto/caam/rng_self_test.h b/drivers/crypto/caam/rng_self_test.h
> new file mode 100644
> index 0000000000..67356fe93b
> --- /dev/null
> +++ b/drivers/crypto/caam/rng_self_test.h
> @@ -0,0 +1,24 @@
> +/*
> + * CAAM RNG self test
> + *
> + * Copyright (C) 2018 Pengutronix, Roland Hieber <r.hieber@pengutronix.de>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef RNG_SELF_TEST_H
> +#define RNG_SELF_TEST_H
> +
> +int rng_self_test(struct device_d *dev, const u8 caam_era, const u8 rngvid, const u8 rngrev);
> +
> +#endif /* RNG_SELF_TEST_H */
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index 28fd42ecd7..e64f34870e 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -387,6 +387,27 @@ static void habv4_display_event(uint8_t *data, uint32_t len)
>  	habv4_display_event_record((struct hab_event_record *)data);
>  }
>  
> +/* Some chips with HAB >= 4.2.3 have an incorrect implementation of the RNG
> + * self-test in ROM code. In this case, an HAB event is generated, and a
> + * software self-test should be run. This variable is set to @c true by
> + * habv4_get_status() when this occurs. */
> +bool habv4_need_rng_software_self_test = false;
> +EXPORT_SYMBOL(habv4_need_rng_software_self_test);
> +
> +#define RNG_FAIL_EVENT_SIZE 36

ARRAY_SIZE is your friend.

> +static uint8_t habv4_known_rng_fail_events[][RNG_FAIL_EVENT_SIZE] = {
> +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> +	  0x00, 0x80, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x01 },
> +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> +	  0x00, 0x04, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> +	  0x00, 0x00, 0x00, 0x01 },
> +};
> +
>  static int habv4_get_status(const struct habv4_rvt *rvt)
>  {
>  	uint8_t data[256];
> @@ -413,10 +434,29 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
>  
>  	len = sizeof(data);
>  	while (rvt->report_event(HAB_STATUS_WARNING, index, data, &len) == HAB_STATUS_SUCCESS) {
> -		pr_err("-------- HAB warning Event %d --------\n", index);
> -		pr_err("event data:\n");
>  
> -		habv4_display_event(data, len);
> +		/* suppress RNG self-test fail events if they can be handled in software */
> +		bool is_rng_fail_event = false;
> +		if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_SELF_TEST)) {
> +			int i;
> +			for (i = 0; i < ARRAY_SIZE(habv4_known_rng_fail_events); i++) {
> +				if (memcmp(data, habv4_known_rng_fail_events[i],
> +					   min(len, (uint32_t)RNG_FAIL_EVENT_SIZE)) == 0) {
> +					is_rng_fail_event = true;
> +					break;
> +				}
> +			}
> +		}

You could put that into a separate function to make this loop body and
also the test better readable.

> +
> +		if (is_rng_fail_event) {
> +			pr_warning("RNG self-test failure detected, will run software self-test\n");

pr_warning is a bit harsh when we atually expect it. pr_info maybe?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox