[EXT] [PATCH] crypto: fsl - Fix RNG generation for lengths greater than 16 bytes

[Paweł Kochanowski]

Hi Horia and Gaurav,

I would like to come back to this topic and check how we can proceed.
In my opinion the simple patch I have sent at the beginning is solving a real memory corruption bug, so I would propose to merge it as is.

Please let me know if you have other ideas how to solve it (potentially without full rewrite of the CAAM drivers).

BR,
Pawel.

> From: Paweł Kochanowski
> Sent: Friday, April 11, 2025 5:11 PM
> 
> Hi Horia,
> 
> > From: Horia Geanta <horia.geanta at nxp.com>
> > Sent: Wednesday, April 9, 2025 6:47 PM
> >
> > On 4/9/2025 9:19 AM, Gaurav Jain wrote:
> > > Hi Pawel
> > >
> > >> From: Paweł Kochanowski <pkochanowski at sii.pl>
> > >>
> > >> 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.
> > >
> > IMO the U-Boot approach is broken.
> > The handling of endianness must be done at creation time, only then we
> > know how to interpret the data and perform adequate byteswaps.
> > U-Boot makes the assumption that everything should be 4-byte swapped,
> > but there are cases when the swap has to be done 8-byte wide (for
> > example address pointers or 8-byte immediate data).
> >
> 
> I think you are right, although it might not be strictly necessary in U-boot as
> there seems to be no case where the 8-byte long data are used.
> As I see the append_ptr() is already prepared for that as the `struct
> ptr_addr_t` definition is different based on CAAM endianness (so words are
> swapped).
> The other place that could potentially cause a problem is append_u64() but I
> can not find anyplace that it is used in and I would propose to remove it.
> Do you see other places that I missed?
> 
> I also crosschecked with TF-A repository and it is done in very similar way
> there (per word swap on usage and not on creation).
> 
> BR,
> Pawel.
> 
> > > 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 would advise against this, considering what I said above.
> > Handling endianness should be done only once.
> >
> > Regards,
> > Horia