Hey Arek
> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusz...@intel.com>
> Sent: Thursday 20 March 2025 16:57
> To: dev@dpdk.org
> Cc: gak...@marvell.com; Ji, Kai <kai...@intel.com>; Dooley, Brian
> <brian.doo...@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusz...@intel.com>; sta...@dpdk.org
> Subject: [PATCH] crypto/qat: fix out-of-place header bytes in aead raw api
>
> This commit fixes a problem with overwriting data in the OOP header in RAW
> API crypto processing when using AEAD algorithms.
>
> Fixes: 85fec6fd9674 ("crypto/qat: unify raw data path functions")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusz...@intel.com>
> ---
> drivers/crypto/qat/dev/qat_crypto_pmd_gens.h | 134
> +++++++++++++++++++
> drivers/crypto/qat/dev/qat_sym_pmd_gen1.c | 13 +-
> 2 files changed, 142 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
> b/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
> index 35c1888082..c447f2cb45 100644
> --- a/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
> +++ b/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
> @@ -6,9 +6,12 @@
> #define _QAT_CRYPTO_PMD_GENS_H_
>
> #include <rte_cryptodev.h>
> +#include <rte_common.h>
> +#include <rte_branch_prediction.h>
> #include "qat_crypto.h"
> #include "qat_sym_session.h"
> #include "qat_sym.h"
> +#include "icp_qat_fw_la.h"
>
> #define AES_OR_3DES_MISALIGNED (ctx->qat_mode ==
> ICP_QAT_HW_CIPHER_CBC_MODE && \
> ((((ctx->qat_cipher_alg ==
> ICP_QAT_HW_CIPHER_ALGO_AES128) || \ @@ -146,6 +149,137 @@
> qat_cipher_is_len_in_bits(struct qat_sym_session *ctx,
> return 0;
> }
>
> +static inline
> +uint32_t qat_reqs_mid_set(int *error, struct icp_qat_fw_la_bulk_req *const
> req,
> + struct qat_sym_op_cookie *const cookie, const void *const opaque,
> + const struct rte_crypto_sgl *sgl_src, const struct rte_crypto_sgl
> *sgl_dst,
> + const union rte_crypto_sym_ofs ofs)
> +{
> + uint32_t src_tot_length = 0; /* Returned value */
> + uint32_t dst_tot_length = 0; /* Used only for input validity checks */
> + uint32_t src_length = 0;
> + uint32_t dst_length = 0;
> + uint64_t src_data_addr = 0;
> + uint64_t dst_data_addr = 0;
> + const struct rte_crypto_vec * const vec_src = sgl_src->vec;
> + const struct rte_crypto_vec * const vec_dst = sgl_dst->vec;
> + const uint32_t n_src = sgl_src->num;
> + const uint32_t n_dst = sgl_dst->num;
> + const uint16_t offset = RTE_MAX(ofs.ofs.cipher.head,
> ofs.ofs.auth.head);
> + const uint8_t is_flat = !(n_src > 1 || n_dst > 1); /* Flat buffer or the
> SGL */
> + const uint8_t is_in_place = !n_dst; /* In-place or out-of-place */
> +
> + *error = 0;
> + if (unlikely((n_src < 1 || n_src > QAT_SYM_SGL_MAX_NUMBER) ||
> + n_dst > QAT_SYM_SGL_MAX_NUMBER)) {
> + QAT_LOG(DEBUG,
> + "Invalid number of sgls, source no: %u, dst no: %u,
> opaque: %p",
> + n_src, n_dst, opaque);
> + *error = -1;
> + return 0;
> + }
> +
> + /* --- Flat buffer --- */
> + if (is_flat) {
> + src_data_addr = vec_src->iova;
> + dst_data_addr = vec_src->iova;
> + src_length = vec_src->len;
> + dst_length = vec_src->len;
> +
> + if (is_in_place)
> + goto done;
> + /* Out-of-place
> + * If OOP, we need to keep in mind that offset needs to
> + * start where the aead starts
> + */
> + dst_length = vec_dst->len;
> + /* Integer promotion here, but it does not bother this time */
> + if (unlikely(offset > src_length || offset > dst_length)) {
> + QAT_LOG(DEBUG,
> + "Invalid size of the vector parameters, source
> length: %u, dst length: %u, opaque: %p",
> + src_length, dst_length, opaque);
> + *error = -1;
> + return 0;
> + }
> + src_data_addr += offset;
> + dst_data_addr = vec_dst->iova + offset;
> + src_length -= offset;
> + dst_length -= offset;
> + src_tot_length = src_length;
> + dst_tot_length = dst_length;
> + goto check;
> + }
> +
> + /* --- Scatter-gather list --- */
> + struct qat_sgl * const qat_sgl_src = (struct qat_sgl *)&cookie-
> >qat_sgl_src;
> + uint16_t i;
> +
> + ICP_QAT_FW_COMN_PTR_TYPE_SET(req-
> >comn_hdr.comn_req_flags,
> + QAT_COMN_PTR_TYPE_SGL);
> + qat_sgl_src->num_bufs = n_src;
> + src_data_addr = cookie->qat_sgl_src_phys_addr;
> + /* Fill all the source buffers but the first one */
> + for (i = 1; i < n_src; i++) {
> + qat_sgl_src->buffers[i].len = (vec_src + i)->len;
> + qat_sgl_src->buffers[i].addr = (vec_src + i)->iova;
> + src_tot_length += qat_sgl_src->buffers[i].len;
> + }
> +
> + if (is_in_place) {
> + /* SGL source first entry, no OOP */
> + qat_sgl_src->buffers[0].len = vec_src->len;
> + qat_sgl_src->buffers[0].addr = vec_src->iova;
> + dst_data_addr = src_data_addr;
> + goto done;
> + }
> + /* Out-of-place */
> + struct qat_sgl * const qat_sgl_dst =
> + (struct qat_sgl *)&cookie->qat_sgl_dst;
> + /*
> + * Offset reaching outside of the first buffer is not supported (RAW
> api).
> + * Integer promotion here, but it does not bother this time
> + */
> + if (unlikely(offset > vec_src->len || offset > vec_dst->len)) {
> + QAT_LOG(DEBUG,
> + "Invalid size of the vector parameters, source length:
> %u, dst length: %u, opaque: %p",
> + vec_src->len, vec_dst->len, opaque);
> + *error = -1;
> + return 0;
> + }
> + /* SGL source first entry, adjusted to OOP offsets */
> + qat_sgl_src->buffers[0].addr = vec_src->iova + offset;
> + qat_sgl_src->buffers[0].len = vec_src->len - offset;
> + /* SGL destination first entry, adjusted to OOP offsets */
> + qat_sgl_dst->buffers[0].addr = vec_dst->iova + offset;
> + qat_sgl_dst->buffers[0].len = vec_dst->len - offset;
> + /* Fill the remaining destination buffers */
> + for (i = 1; i < n_dst; i++) {
> + qat_sgl_dst->buffers[i].len = (vec_dst + i)->len;
> + qat_sgl_dst->buffers[i].addr = (vec_dst + i)->iova;
> + dst_tot_length += qat_sgl_dst->buffers[i].len;
> + }
> + dst_tot_length += qat_sgl_dst->buffers[0].len;
> + qat_sgl_dst->num_bufs = n_dst;
> + dst_data_addr = cookie->qat_sgl_dst_phys_addr;
> +
> +check: /* If error, return directly. If success, jump to one of these
> labels */
> + if (src_tot_length != dst_tot_length) {
> + QAT_LOG(DEBUG,
> + "Source length is not equal to the destination length
> %u, dst no: %u, opaque: %p",
> + src_tot_length, dst_tot_length, opaque);
> + *error = -1;
> + return 0;
> + }
> +done:
> + req->comn_mid.opaque_data = (uintptr_t)opaque;
> + req->comn_mid.src_data_addr = src_data_addr;
> + req->comn_mid.dest_data_addr = dst_data_addr;
> + req->comn_mid.src_length = src_length;
> + req->comn_mid.dst_length = dst_length;
> +
> + return src_tot_length;
> +}
> +
> static __rte_always_inline int32_t
> qat_sym_build_req_set_data(struct icp_qat_fw_la_bulk_req *req,
> void *opaque, struct qat_sym_op_cookie *cookie, diff --git
> a/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
> b/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
> index 24e51a9318..3976d03179 100644
> --- a/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
> +++ b/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
> @@ -942,16 +942,19 @@ qat_sym_dp_enqueue_aead_jobs_gen1(void
> *qp_data, uint8_t *drv_ctx,
> for (i = 0; i < n; i++) {
> struct qat_sym_op_cookie *cookie =
> qp->op_cookies[tail >> tx_queue->trailz];
> + int error = 0;
>
> req = (struct icp_qat_fw_la_bulk_req *)(
> (uint8_t *)tx_queue->base_addr + tail);
> rte_mov128((uint8_t *)req, (const uint8_t *)&(ctx->fw_req));
>
> if (vec->dest_sgl) {
> - data_len = qat_sym_build_req_set_data(req,
> - user_data[i], cookie,
> - vec->src_sgl[i].vec, vec->src_sgl[i].num,
> - vec->dest_sgl[i].vec, vec->dest_sgl[i].num);
> + data_len = qat_reqs_mid_set(&error, req, cookie,
> user_data[i],
> + &vec->src_sgl[i], &vec->dest_sgl[i],
> ofs);
> + /* In oop there is no offset, src/dst addresses are
> moved
> + * to avoid overwriting the dst header
> + */
> + ofs.ofs.cipher.head = 0;
> } else {
> data_len = qat_sym_build_req_set_data(req,
> user_data[i], cookie,
> @@ -959,7 +962,7 @@ qat_sym_dp_enqueue_aead_jobs_gen1(void
> *qp_data, uint8_t *drv_ctx,
> vec->src_sgl[i].num, NULL, 0);
> }
>
> - if (unlikely(data_len < 0))
> + if (unlikely(data_len < 0) || error)
> break;
>
> enqueue_one_aead_job_gen1(ctx, req, &vec->iv[i],
> --
> 2.43.0
Acked-by: Brian Dooley <brian.doo...@intel.com>
