[EXT] [PATCH] crypto: fsl - Fix RNG generation for lengths greater than 16 bytes
Paweł Kochanowski
pkochanowski at sii.pl
Wed Apr 9 13:29:18 CEST 2025
Hi,
> From: Gaurav Jain <gaurav.jain at nxp.com>
> Sent: Wednesday, April 9, 2025 8:19 AM
>
> Hi Pawel
>
> > -----Original Message-----
> > From: Paweł Kochanowski <pkochanowski at sii.pl>
> > Sent: Tuesday, April 8, 2025 4:30 PM
> > To: Gaurav Jain <gaurav.jain at nxp.com>; u-boot at lists.denx.de
> > Cc: Priyanka Jain <priyanka.jain at nxp.com>; Gabriel Nesteruk
> > <gnesteruk at sii.pl>; Pankaj Gupta <pankaj.gupta at nxp.com>; Horia Geanta
> > <horia.geanta at nxp.com>
> > Subject: RE: [EXT] [PATCH] crypto: fsl - Fix RNG generation for
> > lengths greater than 16 bytes
> >
> > [You don't often get email from pkochanowski at sii.pl. Learn why this is
> > important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using the
> 'Report this email'
> > button
> >
> >
> > Hi Gaurav,
> >
> > What we see is that the jr_enqueue() called by run_descriptor_jr()
> > swaps the endianness of the descriptor in place (by modifying the data
> > pointed by
> > desc_add):
> >
> > /* The descriptor must be submitted to SEC block as per endianness
> > * of the SEC Block.
> > * So, if the endianness of Core and SEC block is different, each word
> > * of the descriptor will be byte-swapped.
> > */
> > for (i = 0; i < length; i++) {
> > desc_word = desc_addr[i];
> > sec_out32((uint32_t *)&desc_addr[i], desc_word);
> > }
> >
> > So the sequence looks like this:
> > 1. caam_rng_probe sets correct descriptor in ` caam_rng_priv *priv` 2.
> > caam_rng_read is called with 32B 3.
> > caam_rng_read_one->run_descriptor_jr() is called to generate 16B with
> > `priv->desc` containing valid descriptor 4. The descriptor is swapped
> > in jr_enqueue() before passing it to job ring
>
> I see you are right. In Linux, caam endianness is handled at the time of creating
> the descriptor.
> But In Uboot, caam endianness is not handled at the time of descriptor
> creation.
>
> 5. The loop in
> > caam_rng_read is called second time, this time the `priv->desc`
> > contain swapped data.
> >
> > Interesting thing is that the job still succeeds and that some data
> > are present in the buffers, but maybe swapped descriptor can also be a valid
> one?
> I agree that the descriptor should be reinitialized for each RNG job.
> I am also not sure how the swapped descriptor worked second time.
>
We do not see any error code (run_descriptor_jr returns 0) although the descriptor header is wrong so I assume that no real action should be done by the CAAM?
> > The effect that we see is that when leaving the command executing this
> > sequence we get an exception in "unrelated" place..
> After your change you stop seeing this exception? Are you sure it is related to
> rng descriptor?
>
> >
> > This is what we see when printing descriptor in the caam_rng_read_one
> > on
> > LS1046A:
> >
> > First iteration:
> > 0: B0800005
> > 1: 82500002
> > 2: 60340010
> > 3: 0
> > 4: FBC94DC0
> 4th word is seqoutptr: sgf pre ext (ews). Have you also modified the
> descriptor ?
>
No we did not change the descriptor although this log was from older code.. I have rerun this with code like this:
printf("caam_rng_read_one() priv->desc:\n");
int desc_size = desc_len(priv->desc);
for (int i = 0; i < desc_size; i++)
printf("%i: 0x%08x\n", i, priv->desc[i]);
...
printf("caam_rng_read_one() priv->data:\n");
for (int i = 0; i < CAAM_RNG_MAX_FIFO_STORE_SIZE; i++)
printf("%02x ", priv->data[i]);
First iteration:
caam_rng_read_one() priv->desc:
0: 0xb0800005
1: 0x82500002
2: 0x60340010
3: 0x00000000
4: 0xfbc6ec80
caam_rng_read_one() priv->data:
a9 82 98 2e 7a ef da eb bd 0d a3 f4 e8 db 68 43
Second iteration:
caam_rng_read_one() priv->desc:
0: 0x050080b0
1: 0x02005082
2: 0x10003460
3: 0x00000000
4: 0x80ecc6fb
5: 0x00000000
6: 0x00000000
7: 0x00000000
8: 0x00000000
9: 0x00000000
10: 0x00000000
11: 0x00000000
12: 0x00000000
13: 0x00000000
14: 0x00000000
15: 0x00000000
16: 0x2e9882a9
17: 0xebdaef7a
18: 0xf4a30dbd
19: 0x4368dbe8
20: 0x00000000
...
47: 0x00000000
caam_rng_read_one() priv->data:
2e 98 82 a9 eb da ef 7a f4 a3 0d bd 43 68 db e8
This explain why we see "new" random data generated on second time - it is simply swapped old result as it is now treated as part of the descriptor..
BR,
Pawel.
> Regards
> Gaurav
> >
> > Second iteration:
> > 0: 050080B0
> > 1: 02005082
> > 2: 10003460
> > 3: 0
> > 4: C04DC9FB
> >
> > BR,
> > Pawel.
> >
> > > From: Gaurav Jain <gaurav.jain at nxp.com>
> > > Sent: Tuesday, April 8, 2025 12:09 PM
> > >
> > > Can you explain more details about this issue. Where it is being
> > > modified in
> > > run_descriptor_jr() ?
> > > Regards
> > > Gaurav
> > >
> > > > -----Original Message-----
> > > > From: Pawel Kochanowski <pkochanowski at sii.pl>
> > > > Sent: Monday, April 7, 2025 6:17 PM
> > > > To: u-boot at lists.denx.de
> > > > Cc: Priyanka Jain <priyanka.jain at nxp.com>; Gaurav Jain
> > > > <gaurav.jain at nxp.com>; Gabriel Nesteruk <gnesteruk at sii.pl>; Paweł
> > > > Kochanowski <pkochanowski at sii.pl>
> > > > Subject: [EXT] [PATCH] crypto: fsl - Fix RNG generation for
> > > > lengths greater than 16 bytes
> > > >
> > > > [You don't often get email from pkochanowski at sii.pl. Learn why
> > > > this is important at https://aka.ms/LearnAboutSenderIdentification
> > > > ]
> > > >
> > > > Caution: This is an external email. Please take care when clicking
> > > > links or opening attachments. When in doubt, report the message
> > > > using the
> > > 'Report this email'
> > > > button
> > > >
> > > >
> > > > From: Gabriel Nesteruk <gnesteruk at sii.pl>
> > > >
> > > > Reinitialize the descriptor for each RNG job, as it may be
> > > > modified by run_descriptor_jr().
> > > > Failing to do so can result in memory corruption when
> > > > dm_rng_read() is called a second time on NXP devices with
> > > > CONFIG_SYS_FSL_SEC_BE enabled.
> > > >
> > > > Signed-off-by: Gabriel Nesteruk <gnesteruk at sii.pl>
> > > > Signed-off-by: Pawel Kochanowski <pkochanowski at sii.pl>
> > > > ---
> > > > drivers/crypto/fsl/rng.c | 14 ++++++--------
> > > > 1 file changed, 6 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/crypto/fsl/rng.c b/drivers/crypto/fsl/rng.c
> > > > index
> > > > 06364948052..440b26e3c94 100644
> > > > --- a/drivers/crypto/fsl/rng.c
> > > > +++ b/drivers/crypto/fsl/rng.c
> > > > @@ -27,8 +27,14 @@ struct caam_rng_priv { static int
> > > > caam_rng_read_one(struct caam_rng_priv *priv) {
> > > > int size = ALIGN(CAAM_RNG_MAX_FIFO_STORE_SIZE,
> > > > ARCH_DMA_MINALIGN);
> > > > + ulong rng_desc_size = ALIGN(CAAM_RNG_DESC_LEN,
> > > > + ARCH_DMA_MINALIGN);
> > > > int ret;
> > > >
> > > > + inline_cnstr_jobdesc_rng(priv->desc, priv->data,
> > > > + CAAM_RNG_MAX_FIFO_STORE_SIZE);
> > > > + flush_dcache_range((unsigned long)priv->desc,
> > > > + (unsigned long)priv->desc + rng_desc_size);
> > > > +
> > > > ret = run_descriptor_jr(priv->desc);
> > > > if (ret < 0)
> > > > return -EIO;
> > > > @@ -63,14 +69,6 @@ static int caam_rng_read(struct udevice *dev,
> > > > void *data, size_t len)
> > > >
> > > > static int caam_rng_probe(struct udevice *dev) {
> > > > - struct caam_rng_priv *priv = dev_get_priv(dev);
> > > > - ulong size = ALIGN(CAAM_RNG_DESC_LEN,
> ARCH_DMA_MINALIGN);
> > > > -
> > > > - inline_cnstr_jobdesc_rng(priv->desc, priv->data,
> > > > - CAAM_RNG_MAX_FIFO_STORE_SIZE);
> > > > - flush_dcache_range((unsigned long)priv->desc,
> > > > - (unsigned long)priv->desc + size);
> > > > -
> > > > return 0;
> > > > }
> > > >
> > > > --
> > > > 2.43.0
More information about the U-Boot
mailing list