|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

This is a comprehensive 8-patch series adding RSA-specific capability reporting and RSA-OAEP/RSA-PSS support to DPDK cryptodev. The patches introduce new API structures, update PMDs to advertise capabilities, and add test coverage.

---

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

### Errors

None.

### Warnings

1. **ABI Changes Missing Deprecation Notice**

   The patch adds new fields to public API structures (`rte_cryptodev_asymmetric_xform_capability` and `rte_crypto_rsa_op_param`), which are ABI-breaking changes. These should have been announced in `doc/guides/rel_notes/deprecation.rst` in a prior release before merging.

   While the release notes correctly document the ABI changes, the deprecation notice requirement still applies for changes to installed structures.

2. **Union Member Clarification in Documentation**

   ```c
   union {
       struct rte_crypto_param_range modlen;
       /**< Range of modulus length supported by modulus based xform
        * such as MODEXP, MODINV, DH, and DSA.
        * Value 0 means implementation default.
        */
   ```

   The comment "such as MODEXP, MODINV, DH, and DSA" is now slightly misleading since RSA also uses modulus length, but via `rsa_capa.modlen` instead of this union member. Consider clarifying that this union member is for non-RSA modulus-based transforms.

### Info

1. **`pss_salt` Documentation Clarity**

   The `pss_salt` field documentation in `rte_crypto_rsa_op_param` is thorough, but could benefit from explicitly stating that the PMD will return `RTE_CRYPTO_OP_STATUS_INVALID_ARGS` when `pss_salt.data != NULL` and the PMD does not advertise `pss_explicit_salt == true`.

2. **Zero-Value Sentinel Semantics**

   Several fields use `0` to mean "not reported" or "no limit imposed":
   - `rsa_capa.modlen.{min,max,increment}`: "A min, max, or increment value of 0 means no limit is imposed for that field and the PMD default applies."
   - `rsa_capa.pad_types`: "A value of 0 means padding capability is not reported and the PMD default may apply."
   - `rsa_capa.mgf1_hash_algos`: "A value of 0 means MGF1 capability is not reported and the PMD default may apply."

   This is consistent with existing DPDK patterns, but note that it prevents distinguishing "PMD didn't fill this out" from "PMD explicitly says it has no limits." If a future PMD needs to report "supports no padding schemes" (unlikely but theoretically distinct from "didn't report"), the API cannot represent it.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch correctly uses the new `rsa_capa` union member and advertises only the padding schemes this PMD supports (NONE and PKCS1_5). No OAEP or PSS, which matches the PMD's known limitations.

---

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

### Errors

None.

### Warnings

None.

### Info

Identical pattern to patch 2, applied to the CNXK PMD. Correctly reports NONE and PKCS1_5 padding support only.

---

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

### Errors

None.

### Warnings

1. **Conditional Compilation for OpenSSL Version**

   The commit message states: "For OpenSSL 3.0 and later, advertise OAEP padding along with SHA-1, SHA-2, and SHA-3 digests for both primary hash and MGF1. For older OpenSSL versions, advertise only NONE and PKCS#1 v1.5 padding modes supported by the PMD."

   However, the code unconditionally advertises OAEP and all hash algorithms regardless of OpenSSL version:

   ```c
   .pad_types = ((1 << RTE_CRYPTO_RSA_PADDING_NONE) |
       (1 << RTE_CRYPTO_RSA_PADDING_PKCS1_5) |
       (1 << RTE_CRYPTO_RSA_PADDING_OAEP)),
   ```

   If older OpenSSL versions do not support OAEP, this capability reporting is incorrect. Either:
   - Add `#if OPENSSL_VERSION_NUMBER >= 0x30000000L` guards around the OAEP and hash capability bits, or
   - Remove the "For older OpenSSL versions" clause from the commit message if all supported DPDK OpenSSL versions include OAEP.

   The commit message and code must match.

---

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

### Errors

None.

### Warnings

1. **Error Path Resource Cleanup Asymmetry**

   In `openssl_set_asym_session_parameters()`:

   ```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);
       BN_clear_free(e);
       /* ... */
   ```

   The label cleanup is conditional on `ret != 0`, but the BIGNUM cleanup is unconditional. For consistency, consider whether the label cleanup should also be unconditional (i.e., always free on exit from this block, success or failure). As written, if `ret == 0` and the label was allocated, it's left in place (correct for success), but the asymmetry with BIGNUM cleanup is subtle.

   This is likely intentional (label is part of the session state on success, BIGNUMs are always temporary), but the difference is worth noting.

2. **`OPENSSL_memdup` and `OPENSSL_free` Pairing**

   In `openssl_rsa_set_oaep_params()`:

   ```c
   void *label = OPENSSL_memdup(sess->u.r.label, sess->u.r.label_len);
   if (label == NULL)
       return -1;
   if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, label, sess->u.r.label_len) <= 0) {
       OPENSSL_free(label);
       return -1;
   }
   ```

   The `set0` function takes ownership of `label` on success, so the error-path `OPENSSL_free` is correct. However, if `EVP_PKEY_CTX_set0_rsa_oaep_label` fails, it's not immediately clear from OpenSSL documentation whether it has already freed `label` or left it untouched. The code assumes the latter (which is correct for `set0` functions: they only take ownership on success), but a comment explaining this would improve clarity.

### Info

1. **`openssl_rsa_set_oaep_params` Return Value on Empty Label**

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

   This comment is helpful. It explains why the function doesn't call `set0_rsa_oaep_label` when `label_len == 0`, avoiding an OpenSSL API quirk.

---

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

### Errors

None.

### Warnings

None.

### Info

1. **Test Coverage**

   The tests cover:
   - Default OAEP parameters (hash == MGF1 hash)
   - Custom MGF1 hash different from primary hash
   - OAEP labels (both with explicit MGF1 and defaulted MGF1)
   - Both EXP and CRT private key types

   This is thorough. The capability checks (`is_rsa_oaep_supported`) correctly skip tests when the device doesn't support OAEP, the required hash, or the MGF1 hash.

2. **Capability Check Logic**

   ```c
   if (padding->mgf1hash != 0 &&
           (capa->rsa_capa.mgf1_hash_algos & RTE_BIT64(padding->mgf1hash)) == 0) {
       /* ... */
   ```

   This correctly implements the "if mgf1hash is left unconfigured (0), the PMD falls back to using hash for MGF1" behavior. When `mgf1hash == 0`, the check is skipped, and the PMD's fallback to `padding->hash` (which was already checked via `hash_algos`) is assumed.

---

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

### Errors

None.

### Warnings

1. **Verify-Recover Comment May Cause Confusion**

   In `openssl_rsa_verify_recover()`:

   ```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.
    */
   OPENSSL_free(tmp);
   OPENSSL_LOG(ERR, "RSA sign Verification failed");
   return 1;
   ```

   The function returns `1` (verification failed) but the comment says "let the op complete successfully." The function's return value is not the op completion status; it's a tri-state result (0 = valid, 1 = invalid, -1 = error). The caller (`process_openssl_rsa_op_evp`) then maps `ret > 0` to `RTE_CRYPTO_OP_STATUS_ERROR` and `ret == 0` to success. The comment is correct in intent but slightly misleading in wording. Consider: "...so return 1 (invalid signature) to allow the op to complete (with ERROR status)."

2. **PSS Signature Mismatch Logging Level**

   ```c
   OPENSSL_LOG(DEBUG, "RSA-PSS signature verification failed");
   return 1;
   ```

   A verification failure is logged at DEBUG level, which is appropriate (it's not an error in processing), but a genuine signature mismatch in `openssl_rsa_verify_recover` is logged at ERR level:

   ```c
   OPENSSL_LOG(ERR, "RSA sign Verification failed");
   return 1;
   ```

   For consistency, both should be DEBUG. A signature mismatch is a normal outcome, not a processing error.

### Info

1. **PSS Explicit Salt Not Supported**

   ```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;
   }
   ```

   The PMD correctly rejects explicit salt because it does not advertise `pss_explicit_salt == true` in patch 7. This is the expected behavior per the API contract.

2. **PSS Salt Length Validation**

   The `pss_saltlen` field in `rte_crypto_rsa_padding` is stored directly in `asym_session->u.r.pss_saltlen` without validation. OpenSSL will reject invalid salt lengths (e.g., exceeding `emLen - hLen - 2`), but that happens at sign/verify time, not at session creation. If a more user-friendly error is desired, consider adding a validation check at session creation (compute max salt length from modulus and hash, reject if `pss_saltlen` exceeds it). As-is, the error will be a generic OpenSSL failure during the operation.

---

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

### Errors

None.

### Warnings

None.

### Info

1. **Test Coverage for Salt Lengths**

   The tests cover three salt length scenarios:
   - `pss_saltlen = 32` (digest length)
   - `pss_saltlen = 94` (maximum for this key/hash)
   - `pss_saltlen = 0` (no salt, deterministic PSS)

   This is thorough and validates both normal and edge-case salt lengths.

2. **No Negative Test for Excessive Salt Length**

   The tests do not include a case where `pss_saltlen` exceeds the maximum (`emLen - hLen - 2 = 94`). If desired, a negative test could be added to verify that OpenSSL correctly rejects such a configuration at sign time.

3. **Digest Provided, Not Raw Message**

   ```c
   asym_op->rsa.message.data = rsa_pss_digest_sha256.data;
   asym_op->rsa.message.length = rsa_pss_digest_sha256.len;
   ```

   The test correctly uses a pre-computed SHA-256 digest as the "message" input, matching the API requirement that `RTE_CRYPTO_RSA_PADDING_PSS` expects a digest, not raw plaintext. The test vector `rsa_pss_digest_sha256` is the SHA-256 hash of `rsaplaintext`, clearly documented in patch 8.

---

## Series-Wide Issues

### Errors

None.

### Warnings

1. **ABI Stability Policy**

   This series introduces ABI-breaking changes (new fields in public structures). Per DPDK policy, such changes must be:
   - Announced in `doc/guides/rel_notes/deprecation.rst` at least one release in advance
   - Merged only in ABI-breaking releases (typically LTS+1, e.g., 21.11 - 22.03, 23.11 - 24.03)

   The patches correctly document the ABI changes in `doc/guides/rel_notes/release_26_11.rst`, but do not include a prior deprecation notice. If this is targeting 26.11 as an ABI-breaking release and the notice was in 26.07, this is acceptable. Otherwise, the series may need to wait for a future ABI-breaking window.

2. **OpenSSL Version Compatibility**

   Patch 4's commit message claims OpenSSL 3.0+ is required for OAEP, but the code advertises OAEP unconditionally. If DPDK still supports OpenSSL 1.1.1, and that version supports RSA-OAEP (which it does), the commit message should be corrected. If older OpenSSL versions are no longer supported, the commit message is accurate but the code should add version guards or a configure-time check.

### Info

1. **No Test Coverage for Explicit PSS Salt**

   The series does not implement or test `pss_explicit_salt == true` in any PMD. This is acceptable (it's an optional feature), but application developers should be aware that no current PMD supports application-supplied PSS salts. All PSS signatures are generated with internal RNG-based salts.

2. **No QAT or Other PMD Updates**

   The series updates OcteonTX, CNXK, and OpenSSL PMDs, but does not update Intel QAT, Marvell Nitrox, or other PMDs that may support RSA. Those PMDs will continue to report capabilities via the legacy `modlen` field only. This is acceptable (the API is backward-compatible), but PMD maintainers should be aware that they can adopt the new `rsa_capa` reporting when convenient.

---

## Summary

**Overall Assessment:** The series is well-structured and implements a useful feature (RSA-specific capability reporting and OAEP/PSS support). The code quality is high, with proper error handling, resource cleanup, and thorough test coverage. The main issues are:

1. **ABI deprecation notice** (Warning) - may need to be delayed if not properly announced.
2. **OpenSSL version conditional logic** (Warning in patch 4) -