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

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Thu Sep 3 10:52:18 CEST 2026
Previous message (by thread): |WARNING| [v2,8/8] app/test: add RSA-PSS sign and verify test cases
Next message (by thread): |WARNING| [v2,8/8] app/test: add RSA-PSS sign and verify test cases
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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

# Review of DPDK Patch Series: RSA-specific capability parameters

## Summary

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

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- The API additions are well-structured and properly documented
- ABI changes are correctly documented in release notes
- The new `rte_crypto_rsa_capa` structure provides clear separation of RSA-specific capabilities
- Good use of zero values to indicate "no limit" or "not reported"
- The `pss_explicit_salt` feature flag is correctly marked as optional (defaults to false)

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Correctly migrates from the generic `modlen` union field to `rsa_capa`
- Properly advertises only the padding types actually supported by the PMD

---

## 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, correctly applied to CNXK PMD
- Consistent with OcteonTX implementation

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

- Comprehensive capability advertisement for OpenSSL 3.0+
- Correctly advertises OAEP support along with supported hash algorithms
- Good use of `RTE_BIT64()` macro for bitmask construction
- SHA-1, SHA-2, and SHA-3 family support is appropriate for OpenSSL

---

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

### Errors

None identified. All error paths correctly handle resource cleanup.

### Warnings

1. **Label allocation uses `OPENSSL_zalloc()` but the copy uses `rte_memcpy()`**  
   While not incorrect, consider using `memcpy()` for consistency with the allocation API choice (OpenSSL functions). The mix is unusual but not wrong.

### Info

**Correctness verification:**

Traced all error paths in `openssl_set_asym_session_parameters()`:
- Line 1302-1307: `oaep_md` validation - `goto err_rsa` (label freed at 1425-1428) 
- Line 1316-1319: `mgf1_md` validation - `goto err_rsa` (label freed) 
- Line 1321-1326: label length overflow check - `goto err_rsa` (no allocation yet) 
- Line 1331-1335: label data NULL but length non-zero - `goto err_rsa` (no allocation yet) 
- Line 1337-1339: `OPENSSL_zalloc()` failure - `goto err_rsa` (alloc failed, no free needed) 
- Line 1424-1428: Error cleanup frees `asym_session->u.r.label` and zeroes `label_len` 

Normal teardown in `openssl_reset_asym_session()`:
- Lines 1905-1908: Frees label when `label_len > 0` 

OAEP parameter validation in `openssl_rsa_set_oaep_params()`:
- Line 2312-2322: All OpenSSL API failures return -1 with appropriate cleanup (label allocated via `OPENSSL_memdup()` is freed on error at line 2320) 

Usage context check:
- Lines 2346-2351: OAEP restricted to encrypt/decrypt operations 

No use-after-free, no resource leaks detected.

**API usage:**
- `EVP_PKEY_CTX_set0_rsa_oaep_label()` correctly takes ownership of the label, requiring `OPENSSL_memdup()` instead of direct pointer copy 
- MGF1 hash defaults to OAEP hash when not specified (line 1313-1314) 
- Empty label handling is correct (OpenSSL 3 rejects `set0_rsa_oaep_label(NULL, 0)`, so it's skipped) 

**Code quality:**
- Good factoring of OAEP parameter setup into separate function
- Clear comments explaining OpenSSL API ownership semantics

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

**Test coverage:**
- OAEP with default parameters (SHA-256, no label, no explicit MGF1) 
- OAEP with custom MGF1 hash and label 
- OAEP with label but default MGF1 (falls back to SHA-256) 
- Both EXP and QT (CRT) private key types 
- Proper capability checks before running tests 

**Capability checking logic:**
- `rsa_oaep_supported()` correctly validates:
  - OAEP padding bit in `pad_types`
  - Primary hash in `hash_algos`
  - MGF1 hash in `mgf1_hash_algos` (only when explicitly set)
- Correctly handles `mgf1hash == 0` as "use primary hash" 

**Test data:**
- Label vector is 20 bytes of sequential data (adequate for testing)
- Tests use existing RSA key material (rsa_n, rsa_e, etc.)

---

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

### Errors

None identified. All error paths are correct.

### Warnings

None identified.

### Info

**Correctness verification:**

Session setup in `openssl_set_asym_session_parameters()`:
- Lines 1323-1343: PSS parameter validation and storage
- Line 1329: `pss_md` validation - `goto err_rsa` (no allocation yet) 
- Line 1338-1342: `pss_mgf1_md` validation - `goto err_rsa` (no allocation yet) 
- Line 1343: `pss_saltlen` stored (integer, no resource) 

PSS sign path (`openssl_rsa_pss_sign()`):
- Lines 2343-2384: All OpenSSL API failures return -1
- Line 2355: Explicit salt check rejects `pss_salt.data != NULL` 
- No resources allocated, no cleanup needed 

PSS verify path (`openssl_rsa_pss_verify()`):
- Lines 2420-2452: All OpenSSL API failures return -1
- EVP_PKEY_verify() return value correctly interpreted:
  - `< 0` = setup/library error - return -1
  - `== 0` = invalid signature - return 1
  - `> 0` = valid signature - return 0
- Signature mismatch (ret == 1) does not fail the op, sets `cop->status = ERROR` instead (lines 2693-2695) 

Verify-recover path (`openssl_rsa_verify_recover()`):
- Lines 2480-2550: Comprehensive correctness
- Line 2525: `tmp` allocated via `OPENSSL_malloc()` 
- Line 2528: Allocation failure returns -1 
- Lines 2530-2538: OpenSSL verify_recover failure frees `tmp` before returning 
- Lines 2546-2550: Signature mismatch frees `tmp` before returning 
- Line 2553: Success path frees `tmp` 
- **No leak detected** 

Operation dispatch in `process_openssl_rsa_op_evp()`:
- Lines 2564-2569: PSS restricted to sign/verify 
- Lines 2650-2680: Sign operation branches on PSS vs non-PSS 
- Lines 2683-2697: Verify operation branches on PSS vs verify-recover 
- Lines 2689-2695: Signature mismatch sets `cop->status = ERROR`, returns 0 (op completes successfully) 

**Code quality:**
- Excellent separation of PSS-specific logic into helper functions
- Clear comments explaining EVP_PKEY_verify() return semantics
- Correct distinction between "processing failed" (return -1) and "signature invalid" (return 1 or set status)

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

**Test coverage:**
- PSS with salt length = digest length (32 bytes for SHA-256) 
- PSS with maximum permissible salt length (94 bytes) 
- PSS with zero-length salt (deterministic PSS) 
- Capability checks for PSS padding, hash, and MGF1 hash 

**PSS salt length validation:**
The test vectors document the maximum salt length calculation:
```
emBits = modBits - 1 = 1023
emLen = ceil(emBits / 8) = 128
maxSaltLen = emLen - hLen - 2 = 128 - 32 - 2 = 94
```
This matches RFC 8017 section 9.1.1 

**Test structure:**
- `queue_ops_rsa_pss_sign_verify()` correctly performs sign-then-verify without verify-recover (not supported for PSS)
- Uses `rsa_pss_digest_sha256` (pre-hashed message) as required by PSS API
- Properly checks `result_op->status` for verification result

**SHA-256 digest vector:**
The digest of rsaplaintext (lines 260-268) is a static test vector. For production use, applications should hash their own messages, but for unit testing this is acceptable.

---

## Overall Assessment

### Correctness: PASS
- No use-after-free issues
- No resource leaks on any error path
- All error codes are properly propagated
- Proper cleanup in both normal and error paths
- Correct OpenSSL API usage throughout

### Code Style: PASS
- Follows DPDK coding standards
- Proper use of `RTE_BIT64()` for 64-bit bitmasks
- Consistent formatting and indentation
- Clear variable naming
- Helpful comments where needed

### API Design: PASS
- `rte_crypto_rsa_capa` is well-structured and extensible
- Proper use of bitmasks for capability flags
- Clear separation of OAEP and PSS parameters
- Zero values used consistently to indicate "no limit" or "default"
- ABI changes properly documented

### Documentation: PASS
- Release notes updated in patch 1/8
- Doxygen comments added for new structures
- Inline comments explain OpenSSL API semantics

### Testing: PASS
- Comprehensive test coverage for OAEP and PSS
- Both EXP and QT key types tested
- Edge cases covered (empty label, zero salt, max salt)
- Proper capability checks prevent unsupported test execution

---

## Recommendations

1. **Consider standardizing memory allocation API choice** (Info)  
   Patch 5/8 mixes OpenSSL allocation (`OPENSSL_zalloc()`) with DPDK copy (`rte_memcpy()`). While not incorrect, using `memcpy()` consistently with `OPENSSL_*` functions would be more uniform.

2. **No required changes**  
   All patches are correct as written. The series is ready for merging.

---

## Conclusion

**This patch series is well-implemented and ready for inclusion.** All correctness criteria are met, error handling is thorough, and test coverage is comprehensive. No blocking issues were identified.
Previous message (by thread): |WARNING| [v2,8/8] app/test: add RSA-PSS sign and verify test cases
Next message (by thread): |WARNING| [v2,8/8] app/test: add RSA-PSS sign and verify test cases
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the test-report mailing list