|WARNING| crypto/armv8: validate xform auth key length

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Errors

### 1. Missing error check before memcpy

**File:** drivers/crypto/armv8/rte_armv8_pmd.c  
**Function:** auth_set_prerequisites

The patch adds validation for `xform->auth.key.data == NULL` and key length, but does not show the memcpy operation that this validation is protecting. Based on the commit message stating "the key memcpy to write out of bounds," there must be a memcpy operation using `xform->auth.key.data` and `xform->auth.key.length` after this validation.

**However, the critical issue is:** The validation rejects keys with `length > SHA_BLOCK_MAX`, but according to the commit message, this is meant to prevent buffer overflow in a memcpy. Without seeing the destination buffer declaration in the patch context, I cannot verify that `SHA_BLOCK_MAX` is the correct bound. The destination buffer size must match this check.

**Action required:** Verify that the destination buffer in the subsequent memcpy is sized to `SHA_BLOCK_MAX` bytes. If the buffer is smaller, this validation is insufficient. If larger, the constant should match the actual buffer size.

---

## Warnings

### 1. NULL check and length check ordering

**File:** drivers/crypto/armv8/rte_armv8_pmd.c  
**Line:** `if (xform->auth.key.data == NULL || xform->auth.key.length > SHA_BLOCK_MAX)`

The condition checks for NULL pointer first, which is good. However, the error message is generic and doesn't distinguish between a NULL key and an oversized key. Consider providing more specific error messages:

```c
if (xform->auth.key.data == NULL) {
    ARMV8_CRYPTO_LOG_ERR("Authentication key data is NULL.");
    return -1;
}
if (xform->auth.key.length > SHA_BLOCK_MAX) {
    ARMV8_CRYPTO_LOG_ERR("Authentication key length %u exceeds maximum %u.",
                         xform->auth.key.length, SHA_BLOCK_MAX);
    return -1;
}
```

This improves debuggability by telling the user exactly what went wrong.

### 2. Zero-length key not validated

**File:** drivers/crypto/armv8/rte_armv8_pmd.c  
**Line:** `xform->auth.key.length > SHA_BLOCK_MAX`

The validation checks that the key is not too large, but does not check if the key length is zero. HMAC operations require a key, and a zero-length key is meaningless. Consider adding:

```c
if (xform->auth.key.length == 0) {
    ARMV8_CRYPTO_LOG_ERR("Authentication key length cannot be zero.");
    return -1;
}
```

Or combine it with the upper bound check:

```c
if (xform->auth.key.length == 0 || xform->auth.key.length > SHA_BLOCK_MAX) {
    ARMV8_CRYPTO_LOG_ERR("Invalid authentication key length %u (valid range: 1-%u).",
                         xform->auth.key.length, SHA_BLOCK_MAX);
    return -1;
}
```

### 3. Return value should be specific error code

**File:** drivers/crypto/armv8/rte_armv8_pmd.c  
**Line:** `return -1;`

The function returns `-1` on error, but DPDK convention is to return negative errno values. Consider returning `-EINVAL` instead:

```c
return -EINVAL;
```

Check if other error paths in this function follow this pattern and maintain consistency.

---

## Info

### 1. Algorithm-specific key length validation

The validation uses `SHA_BLOCK_MAX` as a universal upper bound for all auth algorithms. Different HMAC algorithms have different block sizes:
- SHA-1: 64 bytes
- SHA-256: 64 bytes  
- SHA-384/SHA-512: 128 bytes

If `SHA_BLOCK_MAX` is 128, this is correct for all SHA variants. If it's 64, this may reject valid keys for SHA-384/SHA-512. Consider whether algorithm-specific validation would be more precise, though the current approach is acceptable if `SHA_BLOCK_MAX` is set to the maximum of all supported algorithms.

---

## Summary

The patch adds necessary input validation to prevent buffer overflow, which is a good defensive programming practice. The main concerns are:

1. **Error**: Cannot verify the buffer overflow protection is correct without seeing the destination buffer size
2. **Warning**: Error messages could be more specific
3. **Warning**: Zero-length keys are not rejected
4. **Warning**: Return value should use `-EINVAL` instead of `-1`

Review-Result: ERROR