Re: [PATCH 4/6] crypto/openssl: fix 3DES-CTR with big endian CPUs

[David Marchand <david.marchand@redhat.com>]

On Thu, Oct 24, 2024 at 2:55 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 24 October 2024 14.06
> >
> > Caught by code review.
> >
> > Don't byte swap unconditionally (assuming that CPU is little endian is
> > wrong). Instead, convert from big endian to cpu and vice versa.
>
> Yes looks like a bug.
> I wonder if this PMD has more similar bugs...
> grep bswap drivers/crypto/openssl/* says no.
>
> > @@ -110,9 +111,9 @@ ctr_inc(uint8_t *ctr)
> >  {
> >       uint64_t *ctr64 = (uint64_t *)ctr;
> >
> > -     *ctr64 = __builtin_bswap64(*ctr64);
> > +     *ctr64 = rte_be_to_cpu_64(*ctr64);
> >       (*ctr64)++;
> > -     *ctr64 = __builtin_bswap64(*ctr64);
> > +     *ctr64 = rte_cpu_to_be_64(*ctr64);
> >  }
>
> But that's not all.
>
> There may be an alignment bug too; the way it is used in process_openssl_cipher_des3ctr(), "ctr" is not guaranteed to be uint64_t aligned.
>
> How about this instead:
>
> ctr_inc(void *ctr)
> {
>         uint64_t ctr64 = rte_be_to_cpu_64(*(unaligned_uint64_t *)ctr);
>         ctr64++;
>         *(unaligned_uint64_t *)ctr = rte_cpu_to_be_64(ctr64);
> }
>
> Or this:
>
> ctr_inc(void *ctr)
> {
>         uint64_t ctr64;
>
>         memcpy(&ctr64, ctr, sizeof(uint64_t));
>         ctr64 = rte_be_to_cpu_64(ctr64);
>         ctr64++;
>         ctr64 = rte_cpu_to_be_64(ctr64);
>         memcpy(ctr, &ctr64, sizeof(uint64_t));
> }
>
> Or use a union in process_openssl_cipher_des3ctr() to ensure it's uint64_t aligned.

Or declare ctr as a uint64_t in process_openssl_cipher_des3ctr
directly, and remove this casting.


-- 
David Marchand