|WARNING| [v1, 1/1] crypto/openssl: filter legacy ciphers from capability list

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Review of crypto/openssl: filter legacy ciphers from capability list

## ERRORS

### 1. Use-after-free via stale pointer in dev_info->capabilities

**File:** `drivers/crypto/openssl/rte_openssl_pmd_ops.c`  
**Function:** `openssl_pmd_info_get()`  
**Lines:** 1041-1044

```c
if (openssl_pmd_caps_active[0].op == RTE_CRYPTO_OP_TYPE_UNDEFINED)
    dev_info->capabilities = openssl_pmd_capabilities;
else
    dev_info->capabilities = openssl_pmd_caps_active;
```

**Issue:** `dev_info->capabilities` is assigned a pointer to the static array `openssl_pmd_caps_active`, which is overwritten on every call to `openssl_pmd_caps_init()`. If `openssl_pmd_caps_init()` is called again (e.g., by a second device init), all previously returned `dev_info->capabilities` pointers become stale and point to modified data.

**Why it matters:** Callers holding a reference to `dev_info->capabilities` from an earlier `info_get()` call will see the wrong capability list if a new device is initialized.

**Fix:** Initialize the capability list once at driver load time (in the constructor or module init), not per-device. Move `openssl_pmd_caps_init()` from `cryptodev_openssl_create()` to a one-time initialization path, or use a static boolean guard:

```c
void
openssl_pmd_caps_init(void)
{
    static bool initialized = false;
    if (initialized)
        return;
    initialized = true;
    /* ... existing init code ... */
}
```

---

### 2. Resource leak: EVP_CIPHER not freed on continue path

**File:** `drivers/crypto/openssl/rte_openssl_pmd_ops.c`  
**Function:** `openssl_pmd_caps_init()`  
**Lines:** 960-962

```c
EVP_CIPHER *des_cbc = EVP_CIPHER_fetch(NULL, "DES-CBC", "provider=legacy");
if (des_cbc == NULL)
    continue;
EVP_CIPHER_free(des_cbc);
```

**Issue:** If `des_cbc` is non-NULL (DES is available), the cipher is freed. But if `des_cbc == NULL` (DES unavailable), the loop `continue`s without freeing -- which is correct here since there's nothing to free. However, the logic is checking DES-CBC availability for *both* `RTE_CRYPTO_CIPHER_DES_CBC` and `RTE_CRYPTO_CIPHER_DES_DOCSISBPI` with the same fetch call. This means:
- When processing the `DES_DOCSISBPI` capability, a second `EVP_CIPHER_fetch("DES-CBC", ...)` is performed
- If DES-CBC is available, both capabilities pass the check and are included, which is correct
- But the code fetches and frees the cipher handle twice -- once per DES algorithm

This is not a leak (both handles are freed), but it is inefficient. More importantly, the check does not verify that `DES_DOCSISBPI` is actually supported -- it only checks `DES-CBC`.

**Fix:** Fetch the cipher once before the loop and reuse the result:

```c
void
openssl_pmd_caps_init(void)
{
    uint32_t i, j = 0;
    EVP_CIPHER *des_cbc = EVP_CIPHER_fetch(NULL, "DES-CBC", "provider=legacy");
    bool des_available = (des_cbc != NULL);
    EVP_CIPHER_free(des_cbc);

    for (i = 0; openssl_pmd_capabilities[i].op != RTE_CRYPTO_OP_TYPE_UNDEFINED; i++) {
        const struct rte_cryptodev_capabilities *cap = &openssl_pmd_capabilities[i];

        if (cap->op == RTE_CRYPTO_OP_TYPE_SYMMETRIC &&
            cap->sym.xform_type == RTE_CRYPTO_SYM_XFORM_CIPHER &&
            (cap->sym.cipher.algo == RTE_CRYPTO_CIPHER_DES_CBC ||
             cap->sym.cipher.algo == RTE_CRYPTO_CIPHER_DES_DOCSISBPI)) {
            if (!des_available)
                continue;
        }
        openssl_pmd_caps_active[j++] = *cap;
    }
    openssl_pmd_caps_active[j] = openssl_pmd_capabilities[i];
}
```

---

### 3. Logic error: uninitialized capabilities may be returned

**File:** `drivers/crypto/openssl/rte_openssl_pmd_ops.c`  
**Function:** `openssl_pmd_info_get()`  
**Lines:** 1041-1044

```c
if (openssl_pmd_caps_active[0].op == RTE_CRYPTO_OP_TYPE_UNDEFINED)
    dev_info->capabilities = openssl_pmd_capabilities;
else
    dev_info->capabilities = openssl_pmd_caps_active;
```

**Issue:** The static array `openssl_pmd_caps_active` is zero-initialized at load time. If `openssl_pmd_info_get()` is called before `openssl_pmd_caps_init()` (e.g., via `rte_cryptodev_info_get()` before the device is created), the first element's `.op` is zero, which equals `RTE_CRYPTO_OP_TYPE_UNDEFINED`. The code interprets this as "caps not initialized" and returns the full capability list -- but this is coincidental. After `openssl_pmd_caps_init()` is called but produces an empty filtered list (all capabilities filtered out), the first element is explicitly set to `UNDEFINED` by the terminator copy, and the same branch is taken. The logic is correct by accident but fragile.

**Why it matters:** The intent is unclear, and future changes could break the assumption that zero-init equals uninitialized.

**Fix:** Use an explicit initialization flag:

```c
static bool openssl_caps_initialized = false;

void
openssl_pmd_caps_init(void)
{
    if (openssl_caps_initialized)
        return;
    openssl_caps_initialized = true;
    /* ... existing init code ... */
}

static void
openssl_pmd_info_get(struct rte_cryptodev *dev, struct rte_cryptodev_info *dev_info)
{
    /* ... */
    if (dev_info != NULL) {
        dev_info->driver_id = dev->driver_id;
        dev_info->feature_flags = dev->feature_flags;
        dev_info->capabilities = openssl_caps_initialized ?
                                 openssl_pmd_caps_active :
                                 openssl_pmd_capabilities;
        /* ... */
    }
}
```

---

## WARNINGS

### 1. Missing warning when DES algorithms are filtered

**File:** `drivers/crypto/openssl/rte_openssl_pmd_ops.c`  
**Function:** `openssl_pmd_caps_init()`  
**Lines:** 954-967

The commit message states: "Add a warning when DES algorithms are unavailable."  
No such warning is emitted in the code. When `des_cbc == NULL`, the capability is silently dropped.

**Fix:** Add a log message when DES is unavailable:

```c
EVP_CIPHER *des_cbc = EVP_CIPHER_fetch(NULL, "DES-CBC", "provider=legacy");
if (des_cbc == NULL) {
    OPENSSL_LOG(WARNING, "DES algorithms unavailable (legacy provider not loaded)");
}
bool des_available = (des_cbc != NULL);
EVP_CIPHER_free(des_cbc);
```

Place this before the loop, so the warning is emitted once rather than twice (once per DES algorithm).

---

### 2. Array size assumption may break if capabilities are added

**File:** `drivers/crypto/openssl/rte_openssl_pmd_ops.c`  
**Line:** 932

```c
static struct rte_cryptodev_capabilities openssl_pmd_caps_active[RTE_DIM(openssl_pmd_capabilities)];
```

This is dimensioned to match `openssl_pmd_capabilities`, which is correct today. If `openssl_pmd_capabilities` grows, `openssl_pmd_caps_active` grows with it. However, the array is never shrunk if filtering removes many entries. This is not a bug (over-allocation is safe), but consider documenting the size choice or using a dynamic allocation if the list becomes large.

This is Info-level feedback, not a Warning -- the current approach is acceptable.

---

## INFO

### 1. Inconsistent log level for legacy provider load failure

**File:** `drivers/crypto/openssl/rte_openssl_pmd.c`  
**Line:** 40

```c
OPENSSL_LOG(WARNING, "Failed to load Legacy provider");
```

Changed from `ERR` to `WARNING`. This is appropriate -- the PMD continues to function with a reduced capability set -- but consider adding a message explaining the consequence:

```c
OPENSSL_LOG(WARNING, "Failed to load Legacy provider; DES ciphers will be unavailable");
```

---

### 2. Consider warning when default provider load fails

**File:** `drivers/crypto/openssl/rte_openssl_pmd.c`  
**Lines:** 46-48 (not shown in patch, surrounding context)

The patch changes the log level for legacy provider failure but does not touch the default provider path. If the default provider load also fails, that remains at `ERR` level. Verify this is intentional and that the default provider failure still justifies an error (likely yes, as modern algorithms would be unavailable).

---

## FINAL CHECK

**Pass 1 (Correctness):** Three correctness bugs identified:
1. Stale pointer when `openssl_pmd_caps_init()` is called multiple times
2. Inefficient repeated fetch (not a leak, but logic flaw in checking DOCSISBPI support)
3. Fragile uninitialized-check logic relying on zero-init coincidence

**Pass 2 (Style/Process):** One Warning: missing log message promised in commit message.

**Pass 3 (Evidence):** All findings reference lines in the patch or describe logic inferable from the added code.

---

Review-Result: ERROR