|WARNING| [v4, 8/8] test/crypto: 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/170056

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None identified.

### Warnings

None identified.

### Info

- **New ABI fields correctly documented** in release notes and ABI Changes section. Release notes describe the functional change appropriately.
- **API design follows TLV-like extensibility**: the new `rsa_capa` union member allows adding RSA-specific parameters without breaking ABI for other transforms. This is good practice.
- **Documentation in header comments is clear**: the Doxygen for `pss_explicit_salt` and `pss_salt` explains the contract and lifetime.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Correctly updates capability to use the new `rsa_capa` structure introduced in patch 1.
- Advertises padding support via `pad_types` bitmask as intended.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Same pattern as patch 2/8 for a different driver.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Advertises OAEP in addition to NONE/PKCS1_5.
- Reports hash and MGF1 hash capabilities via `hash_algos` and `mgf1_hash_algos` bitmasks.
- Correctly sets `pss_explicit_salt` to false (default) by omitting explicit initialization (the structure is zero-initialized by C).
- The comment "pss_explicit_salt not supported, defaults to false" in patch 7 confirms the OpenSSL PMD does not implement this feature, which is acceptable.

---

## Patch 5/8: crypto/openssl: support RSA-OAEP

### Errors

**Error 1**: Potential resource leak on error path in `openssl_set_asym_session_parameters()`.

**Problem**: If `OSSL_PARAM_BLD_new()` or any subsequent call fails after `asym_session->u.r.label` is allocated, the `label` is not freed because the cleanup label `err_rsa` only frees it when `ret != 0` but that flag is not set until after the allocation succeeds.

```c
asym_session->u.r.label = OPENSSL_zalloc(label_len);
if (asym_session->u.r.label == NULL)
    goto err_rsa;

memcpy(asym_session->u.r.label,
    xform->rsa.padding.oaep_label.data,
    label_len);
asym_session->u.r.label_len = label_len;

OSSL_PARAM_BLD * param_bld = OSSL_PARAM_BLD_new();
if (!param_bld) {
    OPENSSL_LOG(ERR, "failed to allocate resources");
    /* If goto err_rsa here, label is not freed because ret is still 0 */
    goto err_rsa;
}
```

**Why it matters**: Memory leak on session creation failure.

**Suggested fix**: Set `ret = -1` immediately after `OPENSSL_zalloc()` succeeds, so the `err_rsa` cleanup path knows to free it on any subsequent error. Alternatively, add a dedicated error label that unconditionally frees `label` before jumping to `err_rsa`.

```c
asym_session->u.r.label = OPENSSL_zalloc(label_len);
if (asym_session->u.r.label == NULL)
    goto err_rsa;
/* Mark that label was allocated so cleanup knows to free it */
ret = -1;

memcpy(asym_session->u.r.label,
    xform->rsa.padding.oaep_label.data,
    label_len);
asym_session->u.r.label_len = label_len;

OSSL_PARAM_BLD * param_bld = OSSL_PARAM_BLD_new();
if (!param_bld) {
    OPENSSL_LOG(ERR, "failed to allocate resources");
    /* Now err_rsa will free label because ret != 0 */
    goto err_rsa;
}
```

---

### Warnings

None identified.

### Info

- **Good practice**: the OAEP parameter configuration is split into a separate helper function (`openssl_rsa_set_oaep_params()`) that is called at operation time after the context is initialized. This keeps session setup separate from per-operation configuration.
- **Correct error handling**: the op-time OAEP validation (OAEP only valid for encrypt/decrypt) returns `ret` unmodified (still -1) and sets `cop->status`, which causes the operation to fail correctly without completing.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Test cases correctly check capabilities before running (via `is_rsa_oaep_supported()` helper).
- Tests use appropriate return codes (`TEST_SKIPPED` when capabilities are missing).
- Extends capability printing to show RSA padding/hash support, which is helpful for debugging.

---

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

### Errors

**Error 1**: Same potential label leak as in patch 5 if session creation fails after label allocation but before success is recorded.

**Problem**: Patch 7 adds PSS parameter setup in `openssl_set_asym_session_parameters()` after the OAEP label allocation. If PSS setup fails (e.g., `openssl_get_md()` returns NULL for PSS hash), the function jumps to `err_rsa`, which only frees `label` if `ret != 0`. But `ret` is still 0 at that point if OAEP succeeded and PSS failed.

**Why it matters**: Same as Error 1 in patch 5 -- memory leak on session creation failure.

**Suggested fix**: Same as patch 5. Set `ret = -1` immediately after `label` is allocated (or after any resource that needs cleanup on error is acquired).

---

### Warnings

**Warning 1**: `openssl_rsa_pss_verify()` and `openssl_rsa_verify_recover()` both return 1 to indicate signature mismatch, but only the PSS path logs it at DEBUG level. The recover path logs at ERR level.

**Why this may be misleading**: A signature mismatch is a normal outcome (the caller supplied a bad signature), not an internal error. Logging it as ERR may confuse operators.

**Suggested fix**: Consider using DEBUG or INFO level for mismatch messages in both paths, or document that ERR is used only for setup failures and mismatch is INFO.

---

### Info

- **Good design**: PSS verification uses `EVP_PKEY_verify()` directly instead of verify-recover, which is the correct approach for probabilistic schemes.
- **Good design**: the two verification paths (PSS vs PKCS1_5/none) are split into separate functions (`openssl_rsa_pss_verify()` and `openssl_rsa_verify_recover()`), making the code easier to follow.
- **Correct behavior**: a malformed signature causing `EVP_PKEY_verify_recover()` to fail is treated the same as a valid-but-mismatching signature (returns 1, not -1), which is correct per the comment in the code.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- PSS tests correctly use a pre-computed SHA-256 digest of the plaintext (`rsa_pss_digest_sha256`) instead of the raw message, matching the PSS padding requirement.
- Tests cover multiple salt length configurations (digest length, maximum, zero), which is good coverage.
- `is_rsa_pss_supported()` helper correctly checks both primary hash and MGF1 hash capabilities.

---

## Summary

### Critical Issues (Must Fix)

**Patch 5 and Patch 7**:
- **Error**: Resource leak on session creation failure after `asym_session->u.r.label` is allocated but before `ret` is set to non-zero. Any subsequent error (OSSL_PARAM_BLD_new failing, PSS hash lookup failing, etc.) will jump to `err_rsa` without freeing the label because the cleanup path checks `if (ret != 0)`.

**Fix**: Set `ret = -1` immediately after successfully allocating `label`, so the cleanup path knows to free it on any subsequent error.

### Non-Critical Issues

**Patch 7**:
- **Warning**: Inconsistent log level for signature mismatch (DEBUG in PSS path, ERR in recover path). Consider using DEBUG or INFO consistently since mismatch is a normal outcome.

### Positive Observations

- Good use of helper functions to separate session setup from per-operation configuration.
- Correct capability checking and test skipping when features are not supported.
- Clear documentation of new ABI fields and API behavior.
- Tests cover important edge cases (zero salt, maximum salt, custom MGF1 hash, labels).