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

[Roland Hieber <r.hieber@pengutronix.de>]

Hi Sascha,

thanks for the feedback so far! Will fix things in v2.

On Mon, Nov 26, 2018 at 10:01:40AM +0100, Sascha Hauer wrote:
[...]
> > 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
> > + */
> > +
[...]
> > +/*
> > + * 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".

It also errors out when caam_era > 8 or rngrev >= 3. I'm not sure about
the implementation details here why this was done, but that's literally
what the NXP-authored code from U-Boot does.

[...]
> > +	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.

I thought about that too, but I didn't see a way to make memory_display
use pr_vdebug, or otherwise make its output depend on the debug level.

 - Roland

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

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