|WARNING| [v2,7/7] net/ixgbe: refactor IPsec code

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-19

# DPDK Patch Review

## Patch 1/7: net/ixgbe: add key length check in IPsec

### Errors
None.

### Warnings
None.

### Info
- Adds input validation for key length. This is a good security practice.

---

## Patch 2/7: net/ixgbe: remove unnecessary dev pointer

### Errors
None.

### Warnings
None.

### Info
- Clean refactoring that simplifies function signature by using existing `dev_data` pointer.

---

## Patch 3/7: net/ixgbe: harmonize return values in IPsec

### Errors
None.

### Warnings
None.

### Info
- Improves error code consistency by replacing `-1` with specific errno values (`-ENOSPC`, `-ENOENT`).

---

## Patch 4/7: net/ixgbe: get rid of unnecessary malloc in IPsec

### Errors

**Patch 4, `ixgbe_crypto_add_sa()` around line 97-214 (Error - key material not zeroed before free):**
The code allocates a temporary buffer for the key on the stack and copies the session key into it for writing to hardware. After writing to hardware, the key buffer is zeroed with `rte_memzero_explicit(key, sizeof(key))`. However, the `ic_session->key` buffer that was copied from still contains the key material in plaintext, and if the session is freed without proper zeroing, this creates a leak of key material.

**Suggested fix:**
Add `rte_memzero_explicit()` in the session removal path to zero `ic_session->key` before freeing the session structure. Verify that wherever `ic_session` is freed, the key field is wiped.

---

## Patch 5/7: net/ixgbe: use correct index variable in IPsec

### Errors
None.

### Warnings
None.

### Info
- Fixes variable naming inconsistency for better readability (no functional change).

---

## Patch 6/7: net/ixgbe: fix missing HW write in IPsec

### Errors
None.

### Warnings
None.

### Info
- Fixes missing hardware write macro invocation. This is a correctness fix.

---

## Patch 7/7: net/ixgbe: refactor IPsec code

### Errors

**Patch 7, struct assignment zeroing (Error - potential padding/uninitialized data leak):**
In multiple places, the patch uses compound literal assignment to zero structures:
```c
*rxsa = (struct ixgbe_crypto_rx_sa_table){0};
*txsa = (struct ixgbe_crypto_tx_sa_table){0};
*rxip = (struct ixgbe_crypto_rx_ip_table){0};
```

If these structures contain padding bytes (common in structures with alignment requirements), the compound literal initialization zeroes named members but may leave padding undefined. Since these tables may have previously held sensitive data (SPI values, IP addresses), this could leak stale data in padding.

**Suggested fix:**
Use `memset()` or `rte_memzero_explicit()` instead of compound literal assignment:
```c
memset(rxsa, 0, sizeof(*rxsa));
memset(txsa, 0, sizeof(*txsa));
memset(rxip, 0, sizeof(*rxip));
```

**Patch 7, SPI comparison (Error - incorrect comparison after type change):**
The patch changes the comparison from:
```c
if (priv->rx_sa_tbl[i].spi == rte_cpu_to_be_32(ic_session->spi))
```
to:
```c
if (priv->rx_sa_tbl[i].spi == ic_session->spi)
```

This is only correct if `rxsa->spi = ic_session->spi;` in the add path stores the SPI in the same byte order as `ic_session->spi`. Looking at the add path in patch 7, line ~196:
```c
rxsa->spi = ic_session->spi;
```

However, the original code (patch 3, pre-refactor) stored it as:
```c
priv->rx_sa_tbl[sa_index].spi = rte_cpu_to_be_32(ic_session->spi);
```

The patch removes the `rte_cpu_to_be_32()` conversion in the add path but also removes it in the remove path. If `ic_session->spi` is in host byte order, the table entry is now stored in host byte order (changed behavior), so the comparison without conversion is correct **only if** the add path was also changed. Verify that the add and remove paths use consistent byte ordering. If `ic_session->spi` is already in network byte order, document that; if it's in host byte order, the old code was correct and this is a regression.

**Suggested fix:**
Verify `ic_session->spi` byte order. If it is host byte order and the hardware expects network byte order (likely for a SPI field), restore `rte_cpu_to_be_32()` in the add path or document why it is omitted.

---

## Summary

**Patch 4:** Missing zeroing of `ic_session->key` before session free (key material leak).

**Patch 7:**
1. Struct zeroing via compound literal may leave padding uninitialized in structures that previously held sensitive data.
2. SPI byte order inconsistency between add and remove paths after refactor (missing `rte_cpu_to_be_32()` in add path changes storage format; verify this is intentional).