|SUCCESS| crypto/openssl: fix use-after-free bug and cleanup

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164523

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-28

commit 5f761d7b60
Author: Pratik Senapati <psenapati at marvell.com>
crypto/openssl: fix use-after-free bug and cleanup

This patch fixes a use-after-free issue in the ML-KEM decapsulate
handler and adds proper resource cleanup for all error paths.

> diff --git a/.mailmap b/.mailmap
> index 4f93307aed..031becba8c 100644
> --- a/.mailmap
> +++ b/.mailmap

[ ... ]

> diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
> index 4f171f48cc..5bc51b8f0f 100644
> --- a/drivers/crypto/openssl/rte_openssl_pmd.c
> +++ b/drivers/crypto/openssl/rte_openssl_pmd.c
> @@ -3683,38 +3683,29 @@ mlkem_decap_op_evp(struct rte_crypto_op *cop,
>  	}
>  
>  	cctx = EVP_PKEY_CTX_new_from_pkey(NULL, pkey, NULL);
> -	if (cctx == NULL) {
> -		EVP_PKEY_free(pkey);
> -		cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> -		return -1;
> -	}
> +	if (cctx == NULL)
> +		goto err_pkey;

Does this change introduce a resource leak for the params variable? The
original code freed params before the conditional check, but after this
patch params appears to be allocated earlier in the function but not
freed on the err_pkey path.

>  
> -	if (EVP_PKEY_decapsulate_init(cctx, params) != 1) {
> +	if (EVP_PKEY_decapsulate_init(cctx, NULL) != 1) {
>  		cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> -		return -1;
> +		goto err_decap;
>  	}

The commit message states this fixes a use-after-free of params, but
can you confirm params was actually freed before this call in the
original code? The diff context doesn't show where params is freed
before EVP_PKEY_decapsulate_init(). If params is still allocated at
this point, does passing NULL instead potentially change the behavior
of the decapsulation operation?

>  
>  	if (EVP_PKEY_decapsulate(cctx, NULL, &keylen,
>  		op->decap.cipher.data, op->decap.cipher.length) != 1) {
>  		OPENSSL_LOG(ERR, "Failed to determine output length");
> -		EVP_PKEY_free(pkey);
> -		cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> -		return -1;
> +		goto err_decap;
>  	}
>  
>  	if (keylen > op->decap.sk.length) {
>  		OPENSSL_LOG(ERR, "Insufficient buffer for shared key");
> -		EVP_PKEY_free(pkey);
> -		cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> -		return -1;
> +		goto err_decap;
>  	}
>  
>  	if (EVP_PKEY_decapsulate(cctx, op->decap.sk.data, &keylen,
>  			op->decap.cipher.data, op->decap.cipher.length) != 1) {
>  		OPENSSL_LOG(ERR, "Failed to decapsulate");
> -		EVP_PKEY_free(pkey);
> -		cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> -		return -1;
> +		goto err_decap;
>  	}
>  
>  	op->decap.sk.length = keylen;
> @@ -3724,6 +3715,13 @@ mlkem_decap_op_evp(struct rte_crypto_op *cop,
>  	ret = 0;
>  	cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
>  	return ret;
> +
> +err_decap:
> +	EVP_PKEY_CTX_free(cctx);
> +err_pkey:
> +	EVP_PKEY_free(pkey);
> +	cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
> +	return -1;
>  }

Does this cleanup path leak params? If params was allocated earlier in
mlkem_decap_op_evp() and was previously freed before the
EVP_PKEY_decapsulate_init() call, does the new error path properly free
it? The err_pkey and err_decap labels don't show any params cleanup.