|WARNING| [v2,8/8] app/test: add RSA-PSS sign and verify test cases

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Overview
This patch series adds RSA-specific capability parameters to DPDK's cryptodev API, including support for OAEP and PSS padding schemes. The series includes API additions, PMD updates, and comprehensive test coverage.

---

## Patch 1/8: crypto: add RSA-specific capability parameters

### Errors

None.

### Warnings

1. **Missing capability check in PMD adapter function**:
   In typical capability-query patterns, when a PMD reports `pad_types = 0` or `mgf1_hash_algos = 0`, applications need clear guidance on what that means. The documentation states "PMD default may apply" but does not specify that applications must still attempt the operation to discover support. Consider adding a note that `0` means "not reported, try the operation and handle -ENOTSUP".

2. **ABI change documentation could be more explicit**:
   The ABI notes mention the new fields but don't explicitly state that this is an ABI break requiring a major version bump. Consider adding: "This is an ABI-breaking change; applications compiled against older DPDK versions will see different structure layouts."

### Info

None.

---

## Patch 2/8: crypto/octeontx: advertise RSA PKCS#1 v1.5 padding support

### Errors

None.

### Warnings

None.

### Info

None.

---

## Patch 3/8: crypto/cnxk: advertise RSA PKCS#1 v1.5 padding support

### Errors

None.

### Warnings

None.

### Info

None.

---

## Patch 4/8: crypto/openssl: advertise RSA padding and hash capabilities

### Errors

None.

### Warnings

1. **Conditional compilation directives missing**:
   The patch advertises OAEP/PSS for OpenSSL 3.0+ but does not wrap the capability advertisement in `#if` guards checking OpenSSL version. If compiled against OpenSSL 1.1.1, the PMD will advertise OAEP/PSS in capabilities but the processing code (added in later patches) will not handle it correctly. Either:
   - Wrap the OAEP/PSS capability fields in `#if OPENSSL_VERSION_NUMBER >= 0x30000000L`, or
   - Add a comment explaining that the code is OpenSSL-3.0-only and will be protected in later patches.

   The commit message says "For OpenSSL 3.0 and later" but the code does not enforce this at compile time in the capability structure initialization.

---

## Patch 5/8: crypto/openssl: add RSA-OAEP support for OpenSSL PMD

### Errors

None.

### Warnings

1. **Potential resource leak on error path**:
   In `openssl_set_asym_session_parameters()`, if `OSSL_PARAM_BLD_new()` fails after `asym_session->u.r.label` has been allocated, the error path `goto err_rsa` does not free the label. The `err_rsa` label only frees the label `if (ret != 0)`, but `ret` is not set to non-zero until after the `OSSL_PARAM_BLD_new()` call. This means if `param_bld` allocation fails, the label leaks.

   Suggested fix: set `ret = -1` immediately before `goto err_rsa` when `param_bld` allocation fails, or restructure the error handling to always free the label on any failure after it is allocated.

2. **Size_t to uint32_t cast could truncate**:
   ```c
   label_len = (uint32_t)xform->rsa.padding.oaep_label.length;
   ```
   The check `xform->rsa.padding.oaep_label.length > (size_t)INT_MAX` prevents labels larger than INT_MAX, but the cast to `uint32_t` could still truncate on a 64-bit platform if `size_t` is larger than UINT32_MAX. While the INT_MAX check makes this unlikely in practice, the cast is unnecessary--just use `size_t` for `label_len` or check against UINT32_MAX instead of INT_MAX.

---

## Patch 6/8: app/test: add RSA OAEP asymmetric test cases

### Errors

None.

### Warnings

None.

### Info

1. **Test coverage note**:
   The tests cover default MGF1, custom MGF1, and labeled OAEP, which is good. Consider adding a negative test case for an invalid OAEP label length (e.g., label longer than supported) to validate error handling, but this is optional.

---

## Patch 7/8: crypto/openssl: add RSA-PSS support for RSA operations

### Errors

None.

### Warnings

1. **Inconsistent return semantics in verify path**:
   `openssl_rsa_pss_verify()` returns 0 for valid, 1 for invalid, -1 for error. The caller in `process_openssl_rsa_op_evp()` handles this correctly. However, `openssl_rsa_verify_recover()` returns the same 0/1/-1 semantics but logs "RSA sign Verification failed" for both the `ret <= 0` case (which includes processing errors) and the memcmp mismatch case (which is a normal invalid-signature outcome). The logging is misleading--it should distinguish between "signature invalid" (expected) and "operation failed" (unexpected). Not an error, but worth clarifying the log messages.

2. **Potential double-free on label in session reset**:
   In `openssl_reset_asym_session()`, if `sess->u.r.label_len > 0`, the label is freed. However, there is no check to prevent double-free if `openssl_reset_asym_session()` is called twice on the same session (e.g., if session teardown is called multiple times by mistake). Consider setting `sess->u.r.label = NULL` after freeing to make the operation idempotent.

---

## Patch 8/8: app/test: add RSA-PSS sign and verify test cases

### Errors

None.

### Warnings

None.

### Info

1. **Test vector note**:
   The tests use internally-generated PSS salt (via OpenSSL's RNG), so they do not validate against fixed CAVP/ACVP SigGen test vectors. This is expected given that the OpenSSL PMD does not advertise `pss_explicit_salt` support, but worth noting if PMD vendors later add explicit-salt support and need to add additional test vectors.

---

## General Observations

1. **Documentation quality**: The additions to `rte_cryptodev.h` and `rte_crypto_asym.h` are well-documented with clear Doxygen comments. The field descriptions explain when fields are valid and what `0` means.

2. **Test coverage**: The series includes comprehensive test cases covering OAEP (default, labeled, custom MGF1) and PSS (digest-length salt, max salt, zero salt). This is excellent.

3. **ABI change process**: The series correctly notes the ABI changes in the release notes and documents them in the ABI section. Ensure this targets a major release (26.11) where ABI breaks are permitted.

4. **Error handling**: Generally good, but the label leak and potential double-free in the OpenSSL PMD should be addressed.

---

## Summary

**Overall assessment**: The patch series is well-structured and implements a useful feature with good test coverage. The main issues are:

- **1 potential resource leak** (label not freed if `OSSL_PARAM_BLD_new()` fails after label allocation)
- **1 potential double-free** (label in `openssl_reset_asym_session()` not nulled after free)
- **1 capability advertisement issue** (OAEP/PSS capabilities not guarded by OpenSSL version check)
- **Minor inconsistencies** in logging and type casts

None of these are show-stoppers, but they should be addressed before merging.