|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

## Summary
This patch series adds RSA-specific capability parameters to the cryptodev API, introduces RSA-OAEP and RSA-PSS support to the OpenSSL PMD, and provides comprehensive test coverage. The series includes ABI changes to support these new features.

---

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

### Errors

1. **Missing NULL checks on padding parameter access** (Correctness bug)
   - **File**: `lib/cryptodev/rte_cryptodev.h`
   - **Issue**: The new `rsa_capa` field is added to a union, but the documentation does not specify when each union member is valid. Code accessing `rsa_capa.pad_types`, `rsa_capa.mgf1_hash_algos`, etc., could read uninitialized memory if the wrong union member is accessed for a given xform type.
   - **Impact**: Reading the wrong union member is undefined behavior and could produce garbage values or crashes.
   - **Fix**: Add Doxygen comments clarifying that `rsa_capa` is only valid when `xform_type == RTE_CRYPTO_ASYM_XFORM_RSA`. Alternatively, consider making `rsa_capa` a separate field outside the union if it is always meant to be valid for RSA.

2. **Inconsistent zero-means-no-limit semantics** (Warning)
   - **File**: `lib/cryptodev/rte_cryptodev.h`, `struct rte_crypto_rsa_capa`
   - **Issue**: The comment states "A min, max, or increment value of 0 means no limit is imposed for that field and the PMD default applies." However, for `modlen.min = 0`, this is ambiguous: does it mean "minimum is 0 bytes (invalid RSA key)" or "no minimum specified"? This conflicts with the existing `modlen` field documentation which says "Value 0 means implementation default."
   - **Fix**: Change the comment to: "A value of 0 means the PMD default applies; non-zero values specify explicit limits." Or define a sentinel like `UINT32_MAX` for "no limit."

3. **Boolean field not explicitly zero-initialized** (Warning)
   - **File**: `lib/cryptodev/rte_cryptodev.h`, `struct rte_crypto_rsa_capa`
   - **Issue**: The `pss_explicit_salt` field is `bool` but the comment says "false (default)". In C, static/global structs are zero-initialized, but dynamically allocated ones are not unless explicitly zeroed. If a PMD allocates this struct on the stack or via `malloc()`, the field could be uninitialized.
   - **Fix**: Add a note in the Doxygen comment: "PMDs must zero-initialize this structure or explicitly set all fields." Or define a macro like `RTE_CRYPTO_RSA_CAPA_INIT` that zero-initializes the struct.

### Warnings

4. **Unclear zero-value semantics for bitmasks** (Warning)
   - **File**: `lib/cryptodev/rte_cryptodev.h`, `pad_types` and `mgf1_hash_algos` fields
   - **Issue**: The comment says "A value of 0 means ... capability is not reported and the PMD default may apply." But what is the PMD default if a PMD does not populate these fields? Should applications assume all padding types are supported, or none?
   - **Fix**: Clarify: "A value of 0 means the PMD does not advertise specific padding/hash support via this field; applications must assume only the capabilities implied by other fields (e.g., `hash_algos`) or use runtime trial-and-error."

5. **Release notes ABI change wording could be clearer** (Info)
   - **File**: `doc/guides/rel_notes/release_26_11.rst`
   - **Issue**: The ABI change note says "The struct ... is updated to include `rsa_capa`, a new ... field". This is accurate but does not mention that the struct size may change (depending on union padding).
   - **Fix**: Add: "This adds a new union member and may change the size of the containing structure on some platforms."

---

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

### Errors
None.

### Warnings
None.

---

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

### Errors
None.

### Warnings
None.

---

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

### Errors

6. **Conditional compilation for OAEP support not checked** (Correctness bug)
   - **File**: `drivers/crypto/openssl/rte_openssl_pmd_ops.c`
   - **Issue**: The patch advertises `RTE_CRYPTO_RSA_PADDING_OAEP` unconditionally in the capabilities array, even though the commit message says "For OpenSSL 3.0 and later, advertise OAEP padding ...". The code does not check the OpenSSL version or presence of OAEP functions at compile time.
   - **Impact**: If this PMD is compiled against OpenSSL 1.x (which does not support OAEP in the EVP API), advertising OAEP would cause runtime failures when an application tries to use it.
   - **Fix**: Wrap the OAEP capability in a preprocessor check:
     ```c
     #if OPENSSL_VERSION_NUMBER >= 0x30000000L
         (1 << RTE_CRYPTO_RSA_PADDING_OAEP) |
     #endif
     ```
     (Note: OpenSSL 3.0.0 is `0x30000000L`. Verify the exact version where OAEP EVP support was added.)

### Warnings
None.

---

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

### Errors

7. **Label memory leak on early error path** (Resource leak)
   - **File**: `drivers/crypto/openssl/rte_openssl_pmd_ops.c`, function `openssl_set_asym_session_parameters`, around line 1298
   - **Issue**: If `OPENSSL_zalloc(label_len)` succeeds but a subsequent step before `OSSL_PARAM_BLD_new()` fails (e.g., `BN_bin2bn` fails), the code jumps to `err_rsa`, which frees `asym_session->u.r.label` only if `ret != 0`. However, the label was allocated before `ret` was set to a non-zero value, so it leaks.
   - **Fix**: Move the `if (ret != 0)` label cleanup block to always free the label on error:
     ```c
     err_rsa:
         if (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);
         /* ... */
     ```
     Remove the `if (ret != 0)` condition around the free.

8. **OPENSSL_memdup result not checked for NULL** (Missing error check)
   - **File**: `drivers/crypto/openssl/rte_openssl_pmd.c`, function `openssl_rsa_set_oaep_params`, around line 2314
   - **Issue**: `OPENSSL_memdup()` can return NULL on allocation failure, but the code only checks for NULL after calling `EVP_PKEY_CTX_set0_rsa_oaep_label()`, which is too late. If `label` is NULL, passing it to OpenSSL could cause a crash or undefined behavior.
   - **Fix**: Check immediately after allocation:
     ```c
     void *label = OPENSSL_memdup(sess->u.r.label, sess->u.r.label_len);
     if (label == NULL)
         return -1;
     ```

9. **Confusing error message for OAEP on wrong operation type** (Warning)
   - **File**: `drivers/crypto/openssl/rte_openssl_pmd.c`, function `process_openssl_rsa_op_evp`, around line 2348
   - **Issue**: The error message says "OAEP supports encrypt/decrypt only", but the check also triggers for sign/verify. The message should clarify that OAEP is only valid for encrypt/decrypt, not sign/verify.
   - **Fix**: Change the message to: "OAEP padding is only valid for encrypt/decrypt operations, not sign/verify."

### Warnings

10. **Magic number `INT_MAX` used without comment** (Info)
    - **File**: `drivers/crypto/openssl/rte_openssl_pmd_ops.c`, around line 1285
    - **Issue**: The check `if (xform->rsa.padding.oaep_label.length > (size_t)INT_MAX)` guards against OpenSSL API limits (which take `int` length), but this is not obvious.
    - **Fix**: Add a comment: `/* OpenSSL label length is int; reject if too large */`.

---

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

### Errors

11. **Capability check logic inverted** (Correctness bug)
    - **File**: `app/test/test_cryptodev_asym.c`, function `rsa_oaep_supported`, around line 304
    - **Issue**: The check says:
      ```c
      if (capa->rsa_capa.pad_types != 0 &&
          (capa->rsa_capa.pad_types & (1 << RTE_CRYPTO_RSA_PADDING_OAEP)) == 0)
      ```
      This means "if pad_types is non-zero AND OAEP is not set, skip". But if `pad_types == 0`, the check passes and the test continues, which contradicts the intent. According to Patch 1, `pad_types == 0` means "PMD does not report padding capabilities."
    - **Fix**: Change to:
      ```c
      if (capa->rsa_capa.pad_types != 0 &&
          (capa->rsa_capa.pad_types & (1 << RTE_CRYPTO_RSA_PADDING_OAEP)) == 0) {
          RTE_LOG(INFO, USER1, "...");
          return 0;
      }
      if (capa->rsa_capa.pad_types == 0) {
          RTE_LOG(INFO, USER1, "Padding capabilities not reported; assuming unsupported\n");
          return 0;
      }
      ```
      Or invert the entire logic to check for support explicitly.

12. **Missing bounds check on `mgf1hash` before bitshift** (Correctness bug)
    - **File**: `app/test/test_cryptodev_asym.c`, function `rsa_oaep_supported`, around line 314
    - **Issue**: The line `(capa->rsa_capa.mgf1_hash_algos & RTE_BIT64(padding->mgf1hash)) == 0` does not validate that `padding->mgf1hash` is less than 64. If `mgf1hash >= 64`, `RTE_BIT64()` is undefined behavior (left shift of 64 or more on a 64-bit integer).
    - **Fix**: Add a check:
      ```c
      if (padding->mgf1hash >= 64) {
          RTE_LOG(ERR, USER1, "Invalid mgf1hash value %u\n", padding->mgf1hash);
          return 0;
      }
      ```

### Warnings

13. **Redundant `== NULL` check in test suite** (Info)
    - **File**: `app/test/test_cryptodev_asym.c`, multiple test functions
    - **Issue**: Lines like `if (rte_cryptodev_asym_capability_get(dev_id, &idx) == NULL)` are explicitly comparing against NULL, which is acceptable per DPDK style but could be simplified to `if (!rte_cryptodev_asym_capability_get(...))`.
    - **Note**: This is acceptable current style; not flagging as an error.

---

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

### Errors

14. **Signature mismatch returns wrong status in `openssl_rsa_verify_recover`** (Correctness bug)
    - **File**: `drivers/crypto/openssl/rte_openssl_pmd.c`, function `openssl_rsa_verify_recover`, around line 2491
    - **Issue**: The function returns `1` on signature mismatch, which the caller interprets as "set `cop->status = RTE_CRYPTO_OP_STATUS_ERROR`". However, earlier in the same function (around line 2490), there are error paths that also return `1` but log "RSA sign Verification failed". This conflates two different failure modes: signature mismatch (expected outcome) vs. setup/processing error (unexpected).
    - **Fix**: Distinguish between mismatch (return 1) and hard errors (return -1). Ensure all hard errors (malloc failure, EVP_PKEY_verify_recover_init failure) return -1, and only soft mismatches return 1.

15. **Potential double-free of `tmp` on error path** (Resource leak / double-free)
    - **File**: `drivers/crypto/openssl/rte_openssl_pmd.c`, function `openssl_rsa_verify_recover`, around line 2514
    - **Issue**: If `EVP_PKEY_verify_recover(ctx, tmp, ...)` fails, the code calls `OPENSSL_free(tmp)` and returns `1`. But if the caller of `openssl_rsa_verify_recover()` then jumps to an error path that attempts to free `tmp` again (if it was stored in a struct), this could be a double-free. However, reviewing the caller (`process_openssl_rsa_op_evp`), `tmp` is local to this function, so this is safe.
    - **Conclusion**: Not a bug; `tmp` is correctly freed before return.

16. **Missing EVP_PKEY_CTX reset between operations** (Potential bug)
    - **File**: `drivers/crypto/openssl/rte_openssl_pmd.c`, function `openssl_rsa_pss_verify`, around line 2424
    - **Issue**: `EVP_PKEY_verify()` is called on `ctx`, which is `sess->u.r.ctx`. If the same session is reused for multiple operations, does the context need to be reset between calls? OpenSSL's EVP API typically requires `_init()` to be called before each operation. The code calls `EVP_PKEY_verify_init()` at the start of the function, which should reset the context, so this is correct.
    - **Conclusion**: Not a bug; `_init()` resets the context.

### Warnings

17. **Magic number `94` in test suite without explanation** (Info)
    - **File**: `app/test/test_cryptodev_rsa_test_vectors.h`, around line 613
    - **Issue**: The comment says "maxSaltLen = emLen - hLen - 2 = 128 - 32 - 2 = 94", which is correct, but the value `94` is repeated in `rsa_pss_max_salt_xform.rsa.padding.pss_saltlen = 94` without a `#define` or inline comment.
    - **Fix**: Add a `#define RSA_PSS_MAX_SALTLEN 94` or a comment on the line itself.

---

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

### Errors

18. **Same capability check logic bug as Patch 6** (Correctness bug)
    - **File**: `app/test/test_cryptodev_asym.c`, function `rsa_pss_supported`, around line 643
    - **Issue**: Identical issue to finding #11: `pad_types == 0` allows the test to proceed when it should skip.
    - **Fix**: Same as finding #11.