|WARNING| [v2] crypto/openssl: fix use-after-free bug and cleanup

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/165210

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-09

# DPDK Patch Review: crypto/openssl: fix use-after-free bug and cleanup

## Summary

This patch fixes a use-after-free bug in the OpenSSL crypto PMD ML-KEM implementation and adds proper error path cleanup. The changes are generally correct, but there are several areas that need attention.

---

## Errors

### 1. Incorrect status code handling on success path

**File:** `drivers/crypto/openssl/rte_openssl_pmd.c`

**mlkem_encap_op_evp()**: The `cop->status` is set to `RTE_CRYPTO_OP_STATUS_SUCCESS` and `ret = 0` before the cleanup labels. If any of the cleanup functions were to fail (though they typically don't), the function would still return success. While this is unlikely to cause issues in practice since the cleanup functions are essentially no-ops on valid pointers, it's cleaner to set success status only after all cleanup is done.

```c
/* Current pattern (lines 3592-3594) */
op->encap.sk.length = keylen;
ret = 0;
cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;

err_encap:
	EVP_PKEY_CTX_free(cctx);
```

**Suggested fix**: Move success assignments after the label or use a separate success path:

```c
	op->encap.sk.length = keylen;
	cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
	ret = 0;
	goto cleanup;  /* or fall through to cleanup */

err_encap:
	EVP_PKEY_CTX_free(cctx);
err_pkey:
	EVP_PKEY_free(pkey);
err_params:
	OSSL_PARAM_free(params);
cleanup:
	return ret;
```

Or accept the current pattern since the cleanup functions don't fail.

---

## Warnings

### 1. Early cop->status assignment on error paths

**File:** `drivers/crypto/openssl/rte_openssl_pmd.c`

**Both functions** (encap line 3540, decap line 3649): Setting `cop->status = RTE_CRYPTO_OP_STATUS_ERROR` immediately after initial checks means the status is set even before any error occurs. While this ensures error paths don't need to set it, it's slightly confusing since the status is "error" during the entire successful execution until the very end.

```c
/* Line 3540 in encap, line 3649 in decap */
cop->status = RTE_CRYPTO_OP_STATUS_ERROR;

if (EVP_PKEY_fromdata_init(pctx) != 1)
	goto err_params;
```

**Suggested approach**: This is acceptable for simplicity, but consider documenting the pattern or setting status in each error path for clarity.

---

### 2. Inconsistent spacing around EVP_PKEY_encapsulate_init call

**File:** `drivers/crypto/openssl/rte_openssl_pmd.c`  
**Line:** 3556

There's an extra blank line between the `cctx` NULL check and the `EVP_PKEY_encapsulate_init` call:

```c
if (cctx == NULL)
	goto err_pkey;


if (EVP_PKEY_encapsulate_init(cctx, NULL) != 1)
```

**Suggested fix**: Remove one blank line to maintain consistency with the rest of the code style.

---

### 3. Missing release notes update

**Severity:** Warning

The patch fixes a use-after-free bug (a correctness issue) and should be documented in the release notes. The commit message indicates this is a bug fix with `Fixes:` tag and `Cc: stable at dpdk.org`, which suggests it's an important fix that users should be aware of.

**Suggested action**: Add an entry to `doc/guides/rel_notes/release_25_03.rst` (or the appropriate release version) under the "Fixed Issues" section:

```rst
* **crypto/openssl: Fixed use-after-free in ML-KEM operations.**

  Fixed a use-after-free bug where OSSL_PARAM was freed before being used
  by EVP_PKEY_decapsulate_init(). Added proper resource cleanup for all
  error paths in ML-KEM encapsulate and decapsulate operations.
```

---

### 4. .mailmap change unrelated to the fix

**File:** `.mailmap`  
**Line:** 1290

The addition of the author's mailmap entry is typically done in a separate patch or as part of the first contribution. While not incorrect, it's cleaner to separate administrative changes from functional fixes.

**Suggested action**: Consider submitting the `.mailmap` change as a separate patch, or note in the commit message that this is the contributor's first patch.

---

## Info

### 1. Label naming convention

The use of `err_params`, `err_pkey`, `err_encap`/`err_decap` is clear and follows a common pattern. An alternative approach used elsewhere in DPDK is to use numbered labels (`err1`, `err2`, `err3`) or functional labels (`cleanup_pkey`, `cleanup_params`). The current naming is good and self-documenting.

---

### 2. Pattern consistency

The refactoring maintains consistency between `mlkem_encap_op_evp()` and `mlkem_decap_op_evp()`, which is good for maintainability. Both functions now follow the same error handling pattern with similar label naming.

---

## Verification Checklist

- [x] Use-after-free bug correctly identified and fixed (params used after free)
- [x] All error paths now have proper cleanup (goto labels added)
- [x] Resource cleanup order is correct (cctx - pkey - params)
- [x] No new memory leaks introduced
- [x] No NULL pointer dereferences on error paths
- [x] Coding style mostly consistent with DPDK standards
- [ ] Release notes should be updated (Warning)
- [x] Fixes tag present
- [x] Cc: stable present

---

## Conclusion

The core fix is **correct**: the use-after-free bug is properly resolved, and the error path cleanup is now complete. The main issues are:

1. **Missing release notes** for a significant bug fix (Warning)
2. Minor style inconsistency (extra blank line)
3. Success status set before cleanup (acceptable but could be clearer)

The patch significantly improves the code's correctness and is a valuable fix for the stable branch.