|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: improve RSA private key validation

## Summary

This patch adds validation for RSA private key parameters in the CNXK crypto driver to prevent hardware limit violations and NULL pointer dereferences. The changes are mostly correct and address important validation gaps.

---

## Errors

**1. Key type validation placed after pointer dereferences**

The patch accesses `xfrm_rsa->n.data` and `xfrm_rsa->e.data` in the bounds check at line 154-156, but the new `key_type` validation that rejects unknown types is at line 195. If an unknown `key_type` is passed with a zero-length modulus, the earlier NULL check would evaluate the pointer, potentially dereferencing invalid memory if the union layout leaves `n.data` uninitialized.

**Fix:** Move the `key_type` validation immediately after the `rsa->key_type` assignment at line 160, before any other parameter access:

```c
/* Set private key type */
rsa->key_type = xfrm_rsa->key_type;

/* Reject unknown key_type instead of silently treating it as EXP. */
if (rsa->key_type != RTE_RSA_KEY_TYPE_QT && rsa->key_type != RTE_RSA_KEY_TYPE_EXP)
	return -EINVAL;

/* Bound n/e against CNXK's RSA capability and require valid data,
 * since they're used unconditionally below.
 */
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;
```

**2. Missing explicit zero-check on mod_len**

The comment at line 149 says "require valid data", but the bounds check does not explicitly reject `mod_len == 0`. Zero-length modulus should be an error (RSA without a modulus is meaningless), but the condition `mod_len > CNXK_AE_RSA_MAX_MODLEN` permits zero.

**Fix:** Add explicit zero check:

```c
if (mod_len == 0 || 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;
```

---

## Warnings

**1. Inconsistent integer constant style in modulus length check**

The new macro `CNXK_AE_RSA_MAX_MODLEN` is decimal `1024`. Existing DPDK code often uses `0x400` for powers of two, particularly for buffer sizes. Using decimal for 1024 is acceptable but mixing styles can reduce consistency.

**Suggestion:** Consider defining as `0x400` if that matches the style in `cnxk_cryptodev_ops.c` and `cnxk_cryptodev_capabilities.c`, or document why decimal is chosen.

**2. Comment refers to files outside this patch's scope**

The macro comment at line 22-26 claims the value "must match" constants in two other files, but this patch does not verify or update those files. If they define different values, the comment is misleading.

**Suggestion:** Either verify those files define 1024 (and mention in commit message), or weaken the comment to "should match" and note that discrepancies are a separate bug.

**3. New comment at line 181 refers to "above" when the clause is inline**

The comment says "which cnxk_ae_rsa_prep() uses without ever reading d" -- this context is helpful but slightly verbose for inline placement. Minor style point.

---

## Info

**1. Implicit boolean on pointer checks is acceptable here**

The condition `d.length != 0 && d.data == NULL` at line 192 follows the deprecated pattern of using `!= NULL` explicitly, which matches the new checks you added. This is correct per AGENTS.md (explicit comparisons required). No change needed.

**2. Rounding formula `(mod_len + 1) / 2` is correct**

For odd `mod_len`, this correctly rounds up. For even `mod_len`, it equals `mod_len / 2`. The math is sound for the stated purpose. (Note: `RTE_ALIGN_CEIL(mod_len, 2) / 2` would also work but is less clear.)

**3. Error propagation**

All error paths return `-EINVAL`. If the caller logs or handles different validation failures differently, more specific error codes (`-ENOTSUP` for unsupported key type, `-ERANGE` for out-of-bounds lengths) could be helpful, but `-EINVAL` is acceptable for all parameter validation errors.

---

## Style Notes

**1. Line length**

Line 154-156 is 98 characters (within 100-char guideline but long). Consider splitting the condition:

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

This groups the length checks separately from the NULL checks, which may also be clearer logically.

---

## Correctness: No Issues Found in Logic

- CRT component checks now validate both length and data pointer, preventing NULL dereference
- Upper bounds use `(mod_len + 1) / 2` correctly for odd moduli
- Total size calculation switched from `len = p.length * 5` to summing actual lengths, which is correct
- `d.length` bounds check correctly handles the optional-`d` case for public-only operations
- No use-after-free, resource leaks, or uninitialized reads

---

## Required Actions Before Merge

1. **Fix Error #1** (key_type validation order) -- move validation before pointer access
2. **Fix Error #2** (zero mod_len check) -- add explicit `mod_len == 0` rejection

## Recommended Improvements

1. Address Warning #1 or #2 if they apply to your codebase conventions
2. Consider Style Note #1 for readability