|WARNING| [10/10] 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/168724

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-31

# Code Review: DPDK Cryptodev RSA Padding Capability Patch Series

## Summary

This 10-patch series adds RSA-specific capability reporting (padding schemes, MGF1 hash algorithms) and implements RSA-OAEP and RSA-PSS support in the OpenSSL PMD. The series includes test coverage and updates several PMDs to advertise their padding capabilities.

---

## PATCH 01/10: crypto: add RSA-specific capability parameters

### Errors

None identified.

### Warnings

**1. Missing release notes entry**

The patch adds new API structures (`rte_crypto_rsa_capa`) and a new field (`rsa_capa`) to the asymmetric capability union. This is a significant API addition that requires documentation in the release notes.

```
Missing: doc/guides/rel_notes/release_XX_YY.rst entry documenting:
- New rte_crypto_rsa_capa structure
- New rsa_capa field in rte_cryptodev_asymmetric_xform_capability
- Purpose: allow applications to query RSA padding and MGF1 hash support
```

**2. Missing functional test for new capability query**

The patch introduces `rsa_capa` but does not include a test in `app/test` that exercises the new capability reporting API. Applications need to be able to query `rsa_capa` and interpret the padding/hash bitmasks.

### Info

**API design consideration:**

The union in `rte_cryptodev_asymmetric_xform_capability` now has both `modlen` (generic modulus length for MODEXP/MODINV/DH/DSA) and `rsa_capa.modlen` (RSA-specific). The comment clarifies this is intentional, which is good. However, applications querying RSA capabilities should use `rsa_capa.modlen`, not the union's `modlen` field. The documentation could emphasize this more explicitly in the structure comment.

---

## PATCH 02/10: crypto/virtio: advertise RSA padding and hash capabilities

### Errors

None identified.

### Warnings

**Missing release notes update**

The virtio PMD now advertises RSA padding and hash capabilities. This is a driver feature enhancement that should be mentioned in the release notes (likely in a "Drivers" section noting improved RSA capability reporting for virtio crypto).

### Info

The patch correctly uses `RTE_BIT64()` for hash algorithm bitmasks.

---

## PATCH 03/10: crypto/octeontx: advertise RSA PKCS#1 v1.5 padding support

### Errors

None identified.

### Warnings

**Missing release notes update**

Similar to virtio, this is a driver feature update that should be noted in release notes.

---

## PATCH 04/10: crypto/cnxk: advertise RSA PKCS#1 v1.5 padding support

### Errors

None identified.

### Warnings

**Missing release notes update**

Driver feature update; should be in release notes.

---

## PATCH 05/10: crypto/qat: advertise RSA padding capabilities

### Errors

None identified.

### Warnings

**Missing release notes update**

Driver feature update; should be in release notes.

### Info

The patch introduces a `QAT_ASYM_RSA_CAP()` macro to wrap RSA capability reporting. This is good for consistency across QAT generations.

---

## PATCH 06/10: crypto/openssl: advertise RSA padding and hash capabilities

### Errors

None identified.

### Warnings

**1. Missing release notes update**

OpenSSL PMD now advertises OAEP padding (OpenSSL 3.0+) along with hash and MGF1 hash capabilities. This is a significant feature addition for the OpenSSL PMD and must be documented in release notes.

**2. Preprocessor conditional duplicates structure initialization**

The patch has two separate initializations of `rsa_capa` depending on OpenSSL version (one with OAEP for 3.0+, one without for older versions). The `hash_algos` field is set identically in both branches but appears outside the OpenSSL 3.0 conditional for the first branch and inside an `#else` for the second. This is slightly confusing:

```c
/* Good - this works but could be clearer */
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
    .pad_types = ((1 << RTE_CRYPTO_RSA_PADDING_NONE) |
        (1 << RTE_CRYPTO_RSA_PADDING_PKCS1_5) |
        (1 << RTE_CRYPTO_RSA_PADDING_OAEP)),
    .mgf1_hash_algos = ...,
    },
    .hash_algos = ...,
#else
    .pad_types = ((1 << RTE_CRYPTO_RSA_PADDING_NONE) |
        (1 << RTE_CRYPTO_RSA_PADDING_PKCS1_5)),
    },
#endif
```

The closing brace for `rsa_capa = {` is inside the first conditional but the `.hash_algos` assignment (which is part of the parent structure, not `rsa_capa`) is outside. This is legal but subtle. Consider extracting common hash_algos to avoid duplication or adding a comment clarifying the brace nesting.

---

## PATCH 07/10: crypto/openssl: add RSA-OAEP support for OpenSSL PMD

### Errors

**1. Error path resource leak in `openssl_rsa_set_oaep_params()`**

In `openssl_rsa_set_oaep_params()`, when `OPENSSL_memdup()` succeeds but `EVP_PKEY_CTX_set0_rsa_oaep_label()` fails, the label is freed. However, if `EVP_PKEY_CTX_set_rsa_mgf1_md()` fails earlier, the function returns -1 without freeing anything, which is fine because no allocation occurred yet. But if `EVP_PKEY_CTX_set_rsa_oaep_label()` fails after the memdup, the code frees the label and returns -1, which is correct. Actually, on closer inspection, this is handled correctly. Not an error.

**2. Missing NULL check after `OPENSSL_zalloc()` in `openssl_set_asym_session_parameters()`**

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

This check is present, so not an error. However, verify that the `err_rsa` label properly cleans up any resources allocated before this point in the OAEP branch. Looking at the code:

```c
err_rsa:
    if (ret != 0 && asym_session->u.r.label) {
        OPENSSL_free(asym_session->u.r.label);
        asym_session->u.r.label = NULL;
        asym_session->u.r.label_len = 0;
    }
    BN_clear_free(n);
    ...
```

The `err_rsa` label checks `ret != 0` before freeing the label. However, `ret` is initialized to `-1` and is only set to `0` at the end of the function on success. So if `OPENSSL_zalloc()` fails and we jump to `err_rsa`, `ret` is still `-1`, and the label (which is NULL) will be freed (a no-op). This is safe. No error.

**3. Potential resource leak on error path after label allocation**

If `OPENSSL_zalloc()` succeeds and the label is copied, but a subsequent operation fails (e.g., `OSSL_PARAM_BLD_new()` returns NULL), the code jumps to `err_rsa` with `ret` still non-zero, so the label cleanup will execute. This is correct. No error.

### Warnings

**1. Missing release notes update**

This patch adds RSA-OAEP support to the OpenSSL PMD (OpenSSL 3.0+). This is a major feature addition and must be documented in release notes.

**2. `openssl_rsa_set_oaep_params()` Doxygen comment placement**

The function comment says "Must be called after EVP_PKEY_encrypt_init() or EVP_PKEY_decrypt_init()." This is good. However, consider whether this function should validate that the context is in an initialized state, or whether the caller is responsible. Since OpenSSL will fail the set operations if the context is not initialized, the current approach (relying on OpenSSL's checks) is acceptable.

**3. Empty label vs NULL label**

The comment in `openssl_rsa_set_oaep_params()` says:

```c
/* Empty label is default; set0_rsa_oaep_label(NULL,0) fails on OpenSSL 3. */
```

This clarifies that an empty label (length 0) is handled by not calling `set0_rsa_oaep_label()` at all. This is fine, but the comment should also note that OpenSSL's default OAEP label is indeed the empty string, so this matches expected behavior.

---

## PATCH 08/10: app/test: add RSA OAEP asymmetric test cases

### Errors

None identified.

### Warnings

**1. Missing negative test for OAEP with wrong padding on sign/verify**

The tests validate OAEP encrypt/decrypt but do not verify that sign/verify operations with OAEP padding are properly rejected (per the PMD's validation in patch 07). Consider adding a negative test case.

**2. `rsa_oaep_supported()` does not check operation support**

The function checks padding, hash, and MGF1 hash but does not verify that the device supports encrypt/decrypt operations for RSA. The calling tests do check `RTE_CRYPTODEV_FF_RSA_PRIV_OP_KEY_EXP` and `RTE_CRYPTODEV_FF_RSA_PRIV_OP_KEY_QT`, so this is covered. Not a bug, but the function name suggests broader validation than it performs.

---

## PATCH 09/10: crypto/openssl: add RSA-PSS support for RSA operations

### Errors

**1. Missing check for `pss_explicit_salt` capability before rejecting explicit salt**

In `process_openssl_rsa_op_evp()`, the code rejects explicit PSS salt:

```c
if (op->rsa.pss_salt.data != NULL) {
    OPENSSL_LOG(ERR, "Explicit RSA-PSS salt is not supported");
    cop->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
    return ret;
}
```

However, this is hardcoded rejection. The OpenSSL PMD does not set `rsa_capa.pss_explicit_salt = true` in patch 09 (it's left as default false), so this is correct behavior. Not an error, but the code could be more defensive by checking the capability field. Since the capability is set at session creation time and the PMD controls it, the hardcoded check is acceptable.

**2. `openssl_rsa_pss_verify()` error handling**

The function returns 0 on success, 1 on verification failure, -1 on processing error. The caller in `process_openssl_rsa_op_evp()` handles this:

```c
ret = openssl_rsa_pss_verify(pad, sess, op);
if (ret < 0)
    goto err_rsa;
if (ret > 0) {
    cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
    return 0;
}
```

This is correct: verification failure (`ret > 0`) is a normal outcome (invalid signature), not a processing error, so the operation completes with an error status. Verification success (`ret == 0`) falls through to the `cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;` at the end of the function (not shown in the patch context but implied). This is correct. No error.

### Warnings

**1. Missing release notes update**

RSA-PSS support is a major feature addition to the OpenSSL PMD and must be documented in release notes.

**2. `openssl_rsa_pss_sign()` does not check `pss_explicit_salt`**

Similar to the explicit salt rejection in `process_openssl_rsa_op_evp()`, the `openssl_rsa_pss_sign()` function does not independently validate that `op->rsa.pss_salt.data == NULL`. This is fine because the caller checks it first, but for defensive programming, `openssl_rsa_pss_sign()` could assert or check this precondition.

**3. `openssl_rsa_verify_recover()` returns 1 on mismatch, but EVP_PKEY_verify_recover() may return <= 0**

The comment in `openssl_rsa_verify_recover()` notes:

```c
/* A malformed/corrupted signature can make the underlying
 * RSA op itself fail (e.g. invalid padding), rather than
 * just returning a recovered value that fails to compare.
 * Both cases mean verification failed, not that processing
 * broke, so still let the op complete successfully.
 */
```

This is good. However, the function unconditionally returns 1 (verification failed) when `EVP_PKEY_verify_recover()` returns <= 0. OpenSSL can return a negative value for library/setup errors (e.g., malloc failure). Should those be distinguished from signature mismatch? The current code treats all non-success as verification failure. This is debatable. For asymmetric crypto, distinguishing "signature is invalid" from "operation could not be performed" may not be necessary for the application. The current approach is acceptable but could be refined.

---

## PATCH 10/10: app/test: add RSA-PSS sign and verify test cases

### Errors

None identified.

### Warnings

**1. `rsa_pss_digest_sha256` initialization**

The patch adds a SHA-256 digest of `rsaplaintext.data`. The comment explains that PSS expects a digest, not the raw message. This is correct. However, there is no validation that `rsa_pss_digest_sha256` is actually the SHA-256 of `rsaplaintext`. If the test vector is incorrect, the test will fail, which is fine. Consider adding a comment with the command or code used to generate the digest for reproducibility.

**2. Missing test for PSS with MGF1 hash != signature hash**

The tests use PSS with default MGF1 (same as signature hash) but do not test PSS with a custom MGF1 hash (e.g., signature hash = SHA-256, MGF1 = SHA-1). The OpenSSL PMD advertises separate MGF1 hash support, so this should be tested.

---

## Cross-Cutting Issues

### Errors

None identified across patches.

### Warnings

**1. Inconsistent release notes coverage**

Multiple patches introduce new features (OAEP, PSS, capability reporting enhancements) but none include release notes updates. DPDK contribution guidelines require release notes for API changes and new features. This series must include updates to:
- `doc/guides/rel_notes/release_XX_YY.rst` (new features section)
- Possibly driver-specific release notes if virtio/qat/cnxk/octeontx/openssl have separate sections

**2. ABI compatibility**

The patch series adds a new field (`rsa_capa`) to the union in `rte_cryptodev_asymmetric_xform_capability`. This is ABI-compatible as long as:
- The union size does not change (new field must not be larger than existing largest member)
- No symbols are removed

The `rsa_capa` struct (size: `sizeof(struct rte_crypto_param_range) + sizeof(bool) + sizeof(uint8_t) + sizeof(uint64_t)`) may be larger than existing union members (`modlen`, `internal_rng`, `ecdsa_option`, `mldsa_capa`, etc.). Verify that this does not break ABI by checking the union size before and after. If the union grows, this is an ABI break and must be handled with symbol versioning or delayed to the next LTS release.

---

## Overall Assessment

### Correctness: GOOD
The series correctly implements RSA-OAEP and RSA-PSS, with proper error handling, resource cleanup, and capability reporting. No critical correctness bugs identified.

### Style: GOOD
Code follows DP