|WARNING| crypto/ipsec_mb: fix out-of-place operation handling

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-16

# DPDK Patch Review

## Patch: crypto/ipsec_mb: fix out-of-place operation handling

---

## Summary

This patch modifies out-of-place (OOP) crypto operation handling in the ipsec_mb PMD. It removes a memcpy for NULL cipher OOP scatter-gather sessions and adds memcpy operations for OOP non-scatter-gather sessions.

---

## Errors

### 1. Redundant memcpy before cipher operation

**Issue:** The newly added memcpy for non-NULL cipher modes copies data that will be overwritten by the cipher operation itself.

```c
} else if (oop) {
    if (cipher_mode == IMB_CIPHER_NULL) {
        /* OK - NULL cipher needs explicit copy */
        memcpy(...);
    } else if (cipher_mode == IMB_CIPHER_SNOW3G_UEA2_BITLEN ||
            cipher_mode == IMB_CIPHER_KASUMI_UEA1_BITLEN) {
        /* ERROR - copies prefix, then cipher overwrites it */
        memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *),
            rte_pktmbuf_mtod(m_src, uint8_t *),
            job->cipher_start_src_offset_in_bits >> 3);
    } else {
        /* ERROR - copies prefix, then cipher overwrites it */
        memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *),
            rte_pktmbuf_mtod(m_src, uint8_t *),
            job->cipher_start_src_offset_in_bytes);
    }
}
```

**Why it matters:** For OOP operations with real cipher modes, the hardware/library writes to `m_dst` starting at `cipher_start_src_offset`. The memcpy copies the prefix (pre-cipher data), but then the cipher operation overwrites the entire destination buffer from offset 0, making the memcpy pointless and potentially incorrect. The correct pattern is: hardware ciphers the data into `m_dst`, then a manual copy of the prefix is done if needed.

**Suggested fix:**

For non-NULL cipher modes, the memcpy should happen **after** the cipher operation completes (in the post-processing path), or the copy should only cover the region **not** touched by the cipher (the prefix before `cipher_start_src_offset`). Verify what the Intel IPsec-MB library expects for OOP and whether the prefix copy is needed at all.

Alternative: if the library requires the entire source buffer to be present in `m_dst` before calling the cipher, then the fix is correct but needs verification against library behavior.

---

### 2. Missing bounds check before memcpy

**Issue:** The memcpy for NULL cipher OOP copies `msg_len_to_cipher + cipher_start_src_offset` bytes without verifying that `m_dst` has sufficient space.

```c
memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *),
    rte_pktmbuf_mtod(m_src, uint8_t *),
    job->msg_len_to_cipher_in_bytes +
    job->cipher_start_src_offset_in_bytes);
```

**Why it matters:** If `m_dst` data room is smaller than the copy size, this writes past the end of the buffer, causing memory corruption or a segfault.

**Suggested fix:**

```c
size_t copy_len = job->msg_len_to_cipher_in_bytes +
                  job->cipher_start_src_offset_in_bytes;
if (rte_pktmbuf_data_len(m_dst) < copy_len)
    return -EINVAL;
memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *), ...);
```

Or use `rte_pktmbuf_append()` to grow `m_dst` to the required size if not already large enough.

---

### 3. Potential NULL pointer dereference

**Issue:** `m_dst` could be NULL on the OOP code path if the operation was incorrectly configured.

```c
} else if (oop) {
    /* m_dst used here without NULL check */
    memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *), ...);
}
```

**Why it matters:** If `m_dst` is NULL (e.g., due to upstream bug or malformed operation), `rte_pktmbuf_mtod(m_dst, ...)` will dereference NULL.

**Suggested fix:**

At the top of `set_mb_job_params()`, verify:

```c
if (oop && m_dst == NULL)
    return -EINVAL;
```

---

## Warnings

### 1. Unclear cipher vs prefix copy semantics

**Issue:** The commit message states the fix adds "missing buffer copy for out-of-place non-scatter-gather sessions" but does not explain **why** the entire source buffer (or prefix) needs to be copied when a cipher operation will write to `m_dst`.

**Why it matters:** Without understanding the Intel IPsec-MB library's requirements for OOP buffers, it's unclear whether this copy is correct, redundant, or happening at the wrong time.

**Suggested fix:** Add a comment in the code explaining that the memcpy is needed because [reason based on IPsec-MB docs], or remove it if it's redundant.

---

### 2. Inconsistent handling of bit-length vs byte-length modes

**Issue:** The bit-length cipher modes (SNOW3G, KASUMI) do a byte-aligned copy (`>> 3`) but this may not be correct if `cipher_start_src_offset_in_bits` is not a multiple of 8.

```c
memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *),
    rte_pktmbuf_mtod(m_src, uint8_t *),
    job->cipher_start_src_offset_in_bits >> 3);
```

**Why it matters:** If the offset is not byte-aligned, the copy could be short by up to 7 bits, causing incorrect output.

**Suggested fix:**

Either verify that `cipher_start_src_offset_in_bits` is always byte-aligned for these modes, or use:

```c
(job->cipher_start_src_offset_in_bits + 7) >> 3
```

to round up.

---

### 3. Removal of scatter-gather OOP memcpy without justification

**Issue:** The removed code copied cipher output for NULL cipher OOP scatter-gather sessions:

```c
-if (cipher_mode == IMB_CIPHER_NULL && oop) {
-    memcpy(job->dst + job->cipher_start_src_offset_in_bytes,
-        job->src + job->cipher_start_src_offset_in_bytes,
-        job->msg_len_to_cipher_in_bytes);
-}
```

The commit message says this causes a segfault due to "invalid destination buffer" but does not explain under what conditions `job->dst` is invalid.

**Why it matters:** If this memcpy is actually needed for some NULL cipher OOP SG cases, removing it breaks those cases. If it's always wrong, the commit message should explain why `job->dst` is invalid (e.g., "job->dst points to a partial SG segment, not the full destination buffer").

**Suggested fix:** Clarify in the commit message or code comment why `job->dst` is unsafe to use here.

---

### 4. Missing release notes update

**Issue:** This patch fixes a segfault and adds missing functionality (OOP copy for multiple transform combinations). Changes that affect correctness or fix crashes should be documented in release notes.

**Suggested fix:** Add an entry to `doc/guides/rel_notes/release_XX_XX.rst` under "Fixed Issues" mentioning the crash fix and the added OOP support.

---

## Info

### 1. Consider using rte_memcpy for alignment optimization

In the hot path (non-scatter-gather OOP), the standard `memcpy()` is used. If this is performance-critical, consider `rte_memcpy()` which may be faster for packet data.

However, this is control-path setup code (per-operation, not per-packet-byte), so standard `memcpy()` is likely fine.

---

## Style Notes (Correct)

- Use of `else if` chain is appropriate here.
- Code formatting and indentation follow DPDK style.
- No forbidden tokens detected.

---

## Verdict

**This patch has correctness issues that must be addressed before merging.**

The removal of the scatter-gather NULL cipher memcpy and the addition of non-scatter-gather OOP memcpy appear to be in the right direction, but:

1. The newly added memcpy for non-NULL cipher modes (Error #1) is likely incorrect or misplaced.
2. Missing bounds checks (Error #2) could cause buffer overflows.
3. Missing NULL check on `m_dst` (Error #3).
4. The patch needs clearer justification in the commit message and possibly inline comments explaining the IPsec-MB library's OOP buffer requirements.