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

[Paweł Kochanowski]

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