|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

## Summary

This patch fixes buffer handling in out-of-place crypto operations in the ipsec_mb PMD. It removes a potentially dangerous memcpy in scatter-gather cases and adds missing buffer copies for non-scatter-gather out-of-place operations.

## Errors

### 1. Potential buffer overflow in NULL cipher out-of-place copy

**Location:** Lines 1843-1847

```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);
```

**Issue:** This copies `msg_len + offset` bytes starting from the beginning of `m_dst`, but should copy `msg_len` bytes starting at `offset` in the destination. The current code may:
1. Overflow the destination buffer if `offset + msg_len` exceeds the mbuf data size
2. Write to the wrong destination offset (always starts at byte 0 instead of at `offset`)

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

This matches the pattern from the removed code at line 1827-1829 (which correctly added the offset to both src and dst pointers).

### 2. Incorrect copy length for bit-granular ciphers

**Location:** Lines 1848-1851

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

**Issue:** For `SNOW3G_UEA2_BITLEN` and `KASUMI_UEA1_BITLEN`, this only copies the data **before** the cipher region (the offset converted to bytes). The cipher output itself is not copied. This is likely incorrect -- either:
- The cipher operation happens in-place in the src buffer and the entire buffer needs copying, or  
- Only the pre-cipher region should be copied and the cipher fills in its region directly in dst

The commit message states "missing buffer copy for out-of-place non-scatter-gather sessions" but this code does not copy the cipher output.

**Required clarification:** What is the correct behavior for these bit-granular ciphers in out-of-place mode? If the operation modifies `m_src` in-place and the result needs copying to `m_dst`, then the full buffer length should be copied, not just the offset.

**Suggested fix (assuming full buffer copy needed):**
```c
uint32_t total_len = (job->msg_len_to_cipher_in_bits +
    job->cipher_start_src_offset_in_bits + 7) >> 3;
memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *),
    rte_pktmbuf_mtod(m_src, uint8_t *),
    total_len);
```

### 3. Wrong copy length for non-NULL cipher

**Location:** Lines 1852-1855

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

**Issue:** Same as #2 -- this only copies the pre-cipher data, not the cipher output itself. For an out-of-place operation, the destination needs the entire buffer including the cipher result.

**Suggested fix:**
```c
uint32_t total_len = job->cipher_start_src_offset_in_bytes +
    job->msg_len_to_cipher_in_bytes;
memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *),
    rte_pktmbuf_mtod(m_src, uint8_t *),
    total_len);
```

Or copy in two parts (pre-cipher region + cipher region) if the cipher backend writes directly to `m_dst` for the cipher portion.

---

## Warnings

### 1. Missing bounds check on mbuf data room

**Location:** All three memcpy calls (lines 1843, 1849, 1853)

**Issue:** None of the new memcpy operations verify that the source and destination mbufs have sufficient `data_len` or `buf_len` for the copy. If `msg_len_to_cipher_in_bytes + cipher_start_src_offset_in_bytes` exceeds the mbuf data room, this will write past the end of the buffer.

**Suggested addition:** Before the memcpy, add:
```c
uint32_t copy_len = /* calculated length */;
if (unlikely(copy_len > rte_pktmbuf_data_len(m_src) ||
             copy_len > rte_pktmbuf_tailroom(m_dst))) {
    return -EINVAL;
}
```

### 2. Inconsistent copy semantics between removed and added code

**Removed code (line 1827-1829):**
- Used `job->dst` and `job->src` (which are `uint8_t *` pointers already adjusted)
- Added offset to both pointers
- Copied `msg_len` bytes

**Added code (lines 1843-1847):**
- Uses `rte_pktmbuf_mtod()` to get base pointers
- Does NOT add offset to destination
- Copies `msg_len + offset` bytes

This inconsistency suggests the new code does not replicate the old behavior correctly. The removed code was specific to the NULL cipher scatter-gather case and may have had `job->dst` pre-adjusted. The new code appears in a different execution path (non-scatter-gather) and needs different handling.

---

## Info

### 1. Consider extracting common copy logic

The three new memcpy blocks (lines 1843-1855) all follow a similar pattern with different length calculations. Consider extracting this into a helper function:

```c
static inline int
copy_oop_buffer(struct rte_mbuf *m_dst, struct rte_mbuf *m_src,
                uint32_t offset, uint32_t length)
{
    if (unlikely(offset + length > rte_pktmbuf_data_len(m_src)))
        return -EINVAL;
    if (unlikely(offset + length > rte_pktmbuf_tailroom(m_dst)))
        return -EINVAL;
    
    memcpy(rte_pktmbuf_mtod(m_dst, uint8_t *) + offset,
           rte_pktmbuf_mtod(m_src, uint8_t *) + offset,
           length);
    return 0;
}
```

### 2. Commit message could be more specific

The commit message states "unnecessary memcpy() may dereference an invalid destination buffer" but doesn't explain why the destination might be invalid in the scatter-gather case. Adding detail about when `job->dst` is invalid would help reviewers understand the fix.

---

## Questions for Patch Author

1. **For NULL cipher out-of-place non-SGL:** Should the destination offset be added to the memcpy destination pointer (matching the removed code's behavior)?

2. **For bit-granular ciphers:** Does the cipher operation write directly to `m_dst`, or does it modify `m_src` in-place? The current code only copies the pre-cipher region, not the cipher output.

3. **For non-NULL non-bit-granular ciphers:** Same question -- does the result need to be copied from `m_src` after the operation, or does the cipher write to `m_dst` directly?

4. **Testing:** Have these code paths been tested with varying `cipher_start_src_offset_in_bytes` values to verify the destination is written at the correct offset?