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

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

[David Marchand]

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.

Fixes: d61f70b4c918 ("crypto/libcrypto: add driver for OpenSSL library")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/crypto/openssl/rte_openssl_pmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 9657b70c7a..9f5f3cda7d 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2016-2017 Intel Corporation
  */
 
+#include <rte_byteorder.h>
 #include <rte_common.h>
 #include <rte_hexdump.h>
 #include <rte_cryptodev.h>
@@ -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);
 }
 
 /*
-- 
2.46.2

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

[Morten Brørup]

> 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.

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

[David Marchand]

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

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

[David Marchand]

On Thu, Oct 24, 2024 at 3:10 PM David Marchand
<david.marchand@redhat.com> wrote:
> > 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.

Like:

diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c
b/drivers/crypto/openssl/rte_openssl_pmd.c
index 9657b70c7a..8e193759b7 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -99,22 +99,6 @@ digest_name_get(enum rte_crypto_auth_algorithm algo)

 static int cryptodev_openssl_remove(struct rte_vdev_device *vdev);

-/*----------------------------------------------------------------------------*/
-
-/**
- * Increment counter by 1
- * Counter is 64 bit array, big-endian
- */
-static void
-ctr_inc(uint8_t *ctr)
-{
-       uint64_t *ctr64 = (uint64_t *)ctr;
-
-       *ctr64 = __builtin_bswap64(*ctr64);
-       (*ctr64)++;
-       *ctr64 = __builtin_bswap64(*ctr64);
-}
-
 /*
  *------------------------------------------------------------------------------
  * Session Prepare
@@ -1192,7 +1176,9 @@ static int
 process_openssl_cipher_des3ctr(struct rte_mbuf *mbuf_src, uint8_t *dst,
                int offset, uint8_t *iv, int srclen, EVP_CIPHER_CTX *ctx)
 {
-       uint8_t ebuf[8], ctr[8];
+       uint8_t ebuf[8];
+       uint64_t host_ctr;
+       uint64_t ctr;
        int unused, n;
        struct rte_mbuf *m;
        uint8_t *src;
@@ -1209,14 +1195,16 @@ process_openssl_cipher_des3ctr(struct rte_mbuf
*mbuf_src, uint8_t *dst,
        l = rte_pktmbuf_data_len(m) - offset;

        memcpy(ctr, iv, 8);
+       host_ctr = rte_be_64_to_cpu(ctr);

        for (n = 0; n < srclen; n++) {
                if (n % 8 == 0) {
+                       ctr = rte_cpu_to_be_64(host_ctr);
                        if (EVP_EncryptUpdate(ctx,
                                        (unsigned char *)&ebuf, &unused,
                                        (const unsigned char *)&ctr, 8) <= 0)
                                goto process_cipher_des3ctr_err;
-                       ctr_inc(ctr);
+                       host_ctr++;
                }
                dst[n] = *(src++) ^ ebuf[n % 8];


-- 
David Marchand

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

[David Marchand]

On Thu, Oct 24, 2024 at 3:17 PM David Marchand
<david.marchand@redhat.com> wrote:
> @@ -1209,14 +1195,16 @@ process_openssl_cipher_des3ctr(struct rte_mbuf
> *mbuf_src, uint8_t *dst,
>         l = rte_pktmbuf_data_len(m) - offset;
>
>         memcpy(ctr, iv, 8);
> +       host_ctr = rte_be_64_to_cpu(ctr);
>
>         for (n = 0; n < srclen; n++) {
>                 if (n % 8 == 0) {
> +                       ctr = rte_cpu_to_be_64(host_ctr);

Moving this here adds one uneeded extra conversion on the first iteration.
So I would keep the conversion around the host_ctr variable increment,
if you get the idea.


>                         if (EVP_EncryptUpdate(ctx,
>                                         (unsigned char *)&ebuf, &unused,
>                                         (const unsigned char *)&ctr, 8) <= 0)
>                                 goto process_cipher_des3ctr_err;
> -                       ctr_inc(ctr);
> +                       host_ctr++;
>                 }
>                 dst[n] = *(src++) ^ ebuf[n % 8];

-- 
David Marchand

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

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 24 October 2024 15.30
> 
> On Thu, Oct 24, 2024 at 3:17 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > @@ -1209,14 +1195,16 @@ process_openssl_cipher_des3ctr(struct
> rte_mbuf
> > *mbuf_src, uint8_t *dst,
> >         l = rte_pktmbuf_data_len(m) - offset;
> >
> >         memcpy(ctr, iv, 8);
> > +       host_ctr = rte_be_64_to_cpu(ctr);
> >
> >         for (n = 0; n < srclen; n++) {
> >                 if (n % 8 == 0) {
> > +                       ctr = rte_cpu_to_be_64(host_ctr);
> 
> Moving this here adds one uneeded extra conversion on the first
> iteration.
> So I would keep the conversion around the host_ctr variable increment,
> if you get the idea.
> 
> 
> >                         if (EVP_EncryptUpdate(ctx,
> >                                         (unsigned char *)&ebuf,
> &unused,
> >                                         (const unsigned char *)&ctr,
> 8) <= 0)
> >                                 goto process_cipher_des3ctr_err;
> > -                       ctr_inc(ctr);
> > +                       host_ctr++;
> >                 }
> >                 dst[n] = *(src++) ^ ebuf[n % 8];
> 
> --
> David Marchand

LGTM.
For the next version,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

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

[David Marchand]

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.

Besides, avoid unaligned accesses and remove the ctr_inc helper that is
not used anywhere else.

Fixes: d61f70b4c918 ("crypto/libcrypto: add driver for OpenSSL library")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
Changes since v1:
- fixed unaligned access,
- removed unneeded helper,

---
 drivers/crypto/openssl/rte_openssl_pmd.c | 28 ++++++++----------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 9657b70c7a..0616383921 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2016-2017 Intel Corporation
  */
 
+#include <rte_byteorder.h>
 #include <rte_common.h>
 #include <rte_hexdump.h>
 #include <rte_cryptodev.h>
@@ -99,22 +100,6 @@ digest_name_get(enum rte_crypto_auth_algorithm algo)
 
 static int cryptodev_openssl_remove(struct rte_vdev_device *vdev);
 
-/*----------------------------------------------------------------------------*/
-
-/**
- * Increment counter by 1
- * Counter is 64 bit array, big-endian
- */
-static void
-ctr_inc(uint8_t *ctr)
-{
-	uint64_t *ctr64 = (uint64_t *)ctr;
-
-	*ctr64 = __builtin_bswap64(*ctr64);
-	(*ctr64)++;
-	*ctr64 = __builtin_bswap64(*ctr64);
-}
-
 /*
  *------------------------------------------------------------------------------
  * Session Prepare
@@ -1192,7 +1177,8 @@ static int
 process_openssl_cipher_des3ctr(struct rte_mbuf *mbuf_src, uint8_t *dst,
 		int offset, uint8_t *iv, int srclen, EVP_CIPHER_CTX *ctx)
 {
-	uint8_t ebuf[8], ctr[8];
+	uint8_t ebuf[8];
+	uint64_t ctr;
 	int unused, n;
 	struct rte_mbuf *m;
 	uint8_t *src;
@@ -1208,15 +1194,19 @@ process_openssl_cipher_des3ctr(struct rte_mbuf *mbuf_src, uint8_t *dst,
 	src = rte_pktmbuf_mtod_offset(m, uint8_t *, offset);
 	l = rte_pktmbuf_data_len(m) - offset;
 
-	memcpy(ctr, iv, 8);
+	memcpy(&ctr, iv, 8);
 
 	for (n = 0; n < srclen; n++) {
 		if (n % 8 == 0) {
+			uint64_t cpu_ctr;
+
 			if (EVP_EncryptUpdate(ctx,
 					(unsigned char *)&ebuf, &unused,
 					(const unsigned char *)&ctr, 8) <= 0)
 				goto process_cipher_des3ctr_err;
-			ctr_inc(ctr);
+			cpu_ctr = rte_be_to_cpu_64(ctr);
+			cpu_ctr++;
+			ctr = rte_cpu_to_be_64(cpu_ctr);
 		}
 		dst[n] = *(src++) ^ ebuf[n % 8];
 
-- 
2.46.2