|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: crypto/cnxk RSA private key validation

## Summary
This patch adds validation checks for RSA key parameters in the CNXK crypto driver. The changes focus on bounds checking and NULL pointer validation before using RSA key components.

---

## Errors

### 1. Incorrect error path resource leak in `cnxk_ae_fill_rsa_params`

**Issue:** Memory allocated via `rte_zmalloc_socket("cnxk_ae_rsa", total_size, ...)` at line ~161 (not shown but referenced by later pointer arithmetic) is not freed when the function returns -EINVAL on any of the new validation failures.

The function adds multiple early `return -EINVAL` paths after `total_size` is calculated and the buffer is allocated, but none of these new error paths free the allocated memory.

**Why it matters:** Resource leak on every rejected RSA key configuration.

**Fix:** Add cleanup path or move validation before allocation:

```c
/* Move all validation BEFORE the rte_zmalloc_socket call */
/* Bound n/e against CNXK's RSA capability... */
if (mod_len > CNXK_AE_RSA_MAX_MODLEN || exp_len == 0 || ...)
	return -EINVAL;

if (rsa->key_type == RTE_RSA_KEY_TYPE_QT) {
	/* ... all QT validation ... */
} else if (rsa->key_type == RTE_RSA_KEY_TYPE_EXP) {
	/* ... all EXP validation ... */
} else {
	return -EINVAL;
}

/* Calculate total_size */
total_size = /* ... */;

/* NOW allocate */
rsa->n.data = rte_zmalloc_socket(...);
if (rsa->n.data == NULL)
	return -ENOMEM;

/* Copy data */
```

Or add a cleanup label:
```c
	/* ... validation ... */
	if (error_condition)
		goto cleanup;

	/* ... copy operations ... */
	return 0;

cleanup:
	rte_free(rsa->n.data);
	rsa->n.data = NULL;
	return -EINVAL;
```

---

### 2. Missing bounds check in old code path (pre-existing but now more critical)

**Issue:** The `total_size` calculation at line ~201 adds `mod_len + exp_len + len` but does not verify that this sum fits within the hardware's maximum scratch buffer size referenced in the comment (CNXK_CPT_MAX_ASYM_OP_NUM_PARAMS * CNXK_CPT_MAX_ASYM_OP_MOD_LEN = presumably 5 * 1024 = 5120 bytes based on the comment).

While individual components are now bounded, the sum could theoretically exceed the per-operation buffer size if all components are at maximum. For example, if mod_len=1024, exp_len=1024, and len=2560 (5 * 512-byte quintuple components), total_size = 4608 bytes, which is likely fine. However, the code does not explicitly verify this assumption.

**Why it matters:** If the sum exceeds the scratch buffer, subsequent operations will write out of bounds.

**Fix:** Add explicit check after calculating `total_size`:

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

/* Verify total does not exceed hardware scratch buffer capacity */
if (total_size > CNXK_CPT_MAX_ASYM_OP_NUM_PARAMS * CNXK_CPT_MAX_ASYM_OP_MOD_LEN)
	return -EINVAL;
```

Note: This assumes `CNXK_CPT_MAX_ASYM_OP_NUM_PARAMS` is defined; if not, use the numeric constant from the referenced file with a comment.

---

## Warnings

### 1. `exp_len == 0` check is redundant with later NULL check

**Issue:** Line ~155 checks `exp_len == 0 || exp_len > CNXK_AE_RSA_MAX_MODLEN || xfrm_rsa->n.data == NULL || xfrm_rsa->e.data == NULL`.

However, if `exp_len == 0`, the subsequent `memcpy(rsa->e.data, xfrm_rsa->e.data, exp_len)` copies zero bytes, which is harmless. The real issue is if `xfrm_rsa->e.data == NULL`, which is already checked.

The comment says "require valid data" but zero-length exponent is a nonsensical RSA key rather than a NULL pointer dereference risk.

**Suggested improvement:** Either keep the check with a clarifying comment, or remove it if the hardware/spec allows zero-length exponents (unlikely for RSA):

```c
/* Public exponent must be present and within HW limits.
 * Zero-length exponent is cryptographically invalid for RSA.
 */
if (exp_len == 0 || exp_len > CNXK_AE_RSA_MAX_MODLEN ||
    xfrm_rsa->e.data == NULL)
	return -EINVAL;

/* Modulus must be present and within HW limits */
if (mod_len == 0 || mod_len > CNXK_AE_RSA_MAX_MODLEN ||
    xfrm_rsa->n.data == NULL)
	return -EINVAL;
```

This separates the concerns and makes the zero-length check's purpose clear.

---

### 2. Missing `mod_len == 0` check

**Issue:** Line ~155 checks `mod_len > CNXK_AE_RSA_MAX_MODLEN` but does not check `mod_len == 0`. A zero-length modulus is as invalid as a zero-length exponent.

**Fix:** Add the check:

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

---

### 3. Release notes missing

**Issue:** This patch fixes multiple potential NULL dereferences and adds hardware limit enforcement, which are correctness bugs. According to the guidelines, bug fixes should be documented in the release notes.

**Fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or the appropriate current release file):

```rst
* **crypto/cnxk: Fixed RSA key validation**

  Added validation of RSA modulus and exponent lengths against hardware limits.
  Fixed potential NULL pointer dereferences when CRT key components or private
  exponent are NULL. Corrected CRT key size calculation to use actual component
  lengths instead of assuming modulus/2.
```

---

### 4. Comment accuracy: "independently caller-controlled"

**Issue:** Comment at lines ~173-175 says "Each quintuple component... is independently caller-controlled" but this is somewhat misleading. The components come from the `xfrm_rsa` transform structure passed in, which is itself controlled by the application via the RTE API. The phrasing could be clearer.

**Suggested rewording:**

```c
/*
 * Each quintuple component (q, dQ, p, dP, qInv) has its own length
 * and data pointer supplied by the caller via the transform structure.
 * Validate each individually against the half-modulus upper bound,
 * since relying solely on qt.p.length is insufficient.
 */
```

---

## Info

### 1. Potential integer overflow in `max_len` calculation (extremely unlikely)

**Observation:** Line ~167: `max_len = (mod_len + 1) / 2;`

If `mod_len` is close to `SIZE_MAX`, adding 1 could overflow. However, given that `mod_len` is already bounded by `CNXK_AE_RSA_MAX_MODLEN` (1024), this cannot happen in practice.

**No action needed** -- just noting for completeness.

---

### 2. Consider extracting validation into helper functions

**Observation:** The validation logic is now quite complex. Consider extracting it into separate helper functions for readability:

```c
static int
validate_rsa_qt_components(const struct rte_crypto_rsa_priv_key_qt *qt,
                           size_t mod_len)
{
	size_t max_len = (mod_len + 1) / 2;
	
	if (qt->p.length == 0 || qt->p.length > max_len || ...)
		return -EINVAL;
	
	/* ... */
	return 0;
}
```

This is a style suggestion, not a requirement.

---

## Conclusion

The patch addresses important correctness issues (NULL checks, bounds validation). However, it introduces a resource leak on the error paths and is missing a total-size bounds check. These must be fixed before merge.

The release notes should document these bug fixes per DPDK policy.