|WARNING| crypto/cnxk: improve RSA private key validation

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170968

_AI Code Review_

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

# DPDK Patch Review

## Patch: crypto/cnxk: improve RSA private key validation

---

## Errors

### 1. Logic error in key type handling (Correctness Bug)

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Lines 193-197

The patch rejects unknown `key_type` values as an error, but then falls through to code that attempts to handle `RTE_RSA_KEY_TYPE_EXP` without this check being effective. The original code structure had implicit fallthrough handling that would process EXP-type operations even if the key type wasn't explicitly EXP. The new `else` block at line 195 makes the intent clearer, but the subsequent code at lines 230-235 assumes `key_type == RTE_RSA_KEY_TYPE_EXP` when it's not QT, which is now more restrictive.

However, on closer inspection, this is actually **correct** - the patch properly validates and rejects unknown types before reaching the d.data copy block. The code flow is:
- QT path (lines 162-173)
- EXP path (lines 174-191) with validation
- Rejection of unknown types (line 195-197)
- QT setup (lines 217-228)
- EXP setup (lines 230-235) protected by the earlier type validation

**Resolution:** No issue - the logic correctly validates key_type before use.

### 2. Integer overflow in total_size calculation (Correctness Bug)

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Line 200

```c
total_size = mod_len + exp_len + len;
```

All three operands (`mod_len`, `exp_len`, `len`) are `size_t`. If their sum exceeds `SIZE_MAX`, this produces undefined behavior due to unsigned overflow. While `SIZE_MAX` overflow is less likely with the 1024-byte cap, the addition should be checked.

**Suggested fix:**
```c
/* Check for overflow in total size calculation */
if (len > SIZE_MAX - mod_len - exp_len)
	return -ENOMEM;
total_size = mod_len + exp_len + len;
```

Alternatively, since `CNXK_AE_RSA_MAX_MODLEN` is 1024, the maximum theoretical `total_size` is `1024 + 1024 + (5*512) = 4608` bytes, which cannot overflow `size_t` on any DPDK-supported platform. If this analysis is deemed sufficient, a comment explaining why overflow is impossible would be appropriate.

---

## Warnings

### 1. Incomplete bounds validation on public exponent

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Line 155

The patch validates `exp_len > CNXK_AE_RSA_MAX_MODLEN`, but does not validate `exp_len == 0`. While line 155 checks `exp_len == 0`, the public exponent is unconditionally copied at line 215-216 after the buffer is allocated. If `exp_len` were somehow zero despite the check, `rsa->e.data` would be allocated but point to uninitialized memory or the start of the next field.

**Current code (line 155):**
```c
if (mod_len > CNXK_AE_RSA_MAX_MODLEN || exp_len == 0 || exp_len > CNXK_AE_RSA_MAX_MODLEN ||
```

This check is present and correct - the `exp_len == 0` case is properly rejected. **No issue.**

### 2. Missing validation of modulus length lower bound

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Line 155

The patch adds an upper bound (`mod_len > CNXK_AE_RSA_MAX_MODLEN`) but does not validate a minimum. RSA security standards typically require a minimum modulus size (e.g., 512 bits / 64 bytes for legacy, 2048 bits / 256 bytes for modern use). A zero-length or extremely short modulus should be rejected.

**Suggested addition:**
```c
/* Reject moduli below minimum security threshold (e.g., 64 bytes = 512 bits) */
#define CNXK_AE_RSA_MIN_MODLEN 64

if (mod_len < CNXK_AE_RSA_MIN_MODLEN || mod_len > CNXK_AE_RSA_MAX_MODLEN || ...)
```

This depends on CNXK hardware capabilities and security policy - if the hardware accepts arbitrarily small moduli, the PMD may choose to allow them and let the application enforce policy.

---

## Info

### 1. Redundant NULL check removal is correct

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Lines 162, 220

The patch removes:
```c
if (qt.p.length != 0 && qt.p.data == NULL)
```
and replaces it with a comprehensive bounds check that includes `qt.p.data == NULL`. Similarly, the `if (qt.p.data != NULL)` guard before `memcpy` at line 220 is removed because the earlier validation (line 172) guarantees `qt.p.data` is non-NULL.

This is correct - the validation moved earlier in the function makes the later checks redundant.

### 2. Style: line length

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Lines 155, 172

Lines 155 and 172 exceed 100 characters. Consider breaking them for readability:

```c
if (mod_len > CNXK_AE_RSA_MAX_MODLEN || exp_len == 0 ||
    exp_len > CNXK_AE_RSA_MAX_MODLEN ||
    xfrm_rsa->n.data == NULL || xfrm_rsa->e.data == NULL)
	return -EINVAL;

if (qt.p.length == 0 || qt.p.length > max_len || qt.p.data == NULL ||
    qt.q.length == 0 || qt.q.length > max_len || qt.q.data == NULL ||
    qt.dP.length == 0 || qt.dP.length > max_len || qt.dP.data == NULL ||
    qt.dQ.length == 0 || qt.dQ.length > max_len || qt.dQ.data == NULL ||
    qt.qInv.length == 0 || qt.qInv.length > max_len ||
    qt.qInv.data == NULL)
	return -EINVAL;
```

### 3. Comment precision

**File:** `drivers/crypto/cnxk/cnxk_ae.h`  
**Location:** Line 181

The comment states "Total size is the actual sum of the quintuple component lengths" but the calculation includes `mod_len + exp_len` as well (line 200). The comment could be more precise: "Total size includes modulus, public exponent, and quintuple component lengths."

---

## Summary

**Must Fix (Errors):**
1. Verify integer overflow safety in `total_size` calculation (line 200) or add overflow check / explanatory comment

**Should Fix (Warnings):**
1. Consider adding minimum modulus length validation (security / hardware policy)

**Consider (Info):**
1. Break long lines for readability (lines 155, 172)
2. Clarify comment at line 181 to mention modulus and public exponent

**Overall Assessment:**  
The patch significantly improves RSA key validation by adding comprehensive bounds checks, preventing NULL pointer dereferences, and correctly calculating CRT key sizes. The primary concern is ensuring the `total_size` calculation cannot overflow (likely benign given the 1024-byte cap, but should be verified or documented).