|WARNING| [v3] crypto/qat: require IPsec MB for HMAC precomputes

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-22

# Review of QAT Crypto PMD IPsec MB Dependency Patch

## Summary
This patch transitions the QAT crypto PMD from optional IPsec MB to mandatory IPsec MB for HMAC precomputes, removing the OpenSSL fallback for that functionality. OpenSSL remains optional for DOCSIS BPI cipher operations only.

---

## Errors

### 1. Resource leak in legacy provider load failure path
**File:** `drivers/crypto/qat/qat_sym_session.c`
**Lines:** 637-646

The error path after `ossl_legacy_provider_load()` fails does not unload the provider on the success path before returning. If `qat_sym_session_set_parameters()` succeeds, the providers remain loaded.

**Fix:** The unload should happen in both success and failure paths:
```c
#ifdef RTE_QAT_OPENSSL
	ret = ossl_legacy_provider_load();
	if (ret != 0)
		return ret;
#endif
	ret = qat_sym_session_set_parameters(dev, xform,
			CRYPTODEV_GET_SYM_SESS_PRIV(sess),
			CRYPTODEV_GET_SYM_SESS_PRIV_IOVA(sess));
+#ifdef RTE_QAT_OPENSSL
+	ossl_legacy_provider_unload();
+#endif
	if (ret != 0) {
		QAT_LOG(ERR,
		    "Crypto QAT PMD: failed to configure session parameters");
-#ifdef RTE_QAT_OPENSSL
-		ossl_legacy_provider_unload();
-#endif
		return ret;
	}

-#ifdef RTE_QAT_OPENSSL
-	ossl_legacy_provider_unload();
-#endif
	return 0;
```

**Why it matters:** Providers loaded but not unloaded leak resources and may interfere with subsequent operations.

---

### 2. Missing NULL check before OSSL_PROVIDER_unload
**File:** `drivers/crypto/qat/qat_sym_session.c`
**Lines:** 48-56

In `ossl_legacy_provider_load()`, if `OSSL_PROVIDER_load(NULL, "legacy")` returns NULL, the code attempts to load the default provider. If that also fails, `legacy_lib` (which is NULL) is passed to `OSSL_PROVIDER_unload()`.

**Current code:**
```c
legacy_lib = OSSL_PROVIDER_load(NULL, "legacy");
if (legacy_lib == NULL)
	return -EINVAL;

default_lib = OSSL_PROVIDER_load(NULL, "default");
if (default_lib == NULL) {
	OSSL_PROVIDER_unload(legacy_lib);  // <-- legacy_lib is NULL here if first load failed
	legacy_lib = NULL;
	return -EINVAL;
}
```

Wait, re-reading: if `legacy_lib == NULL`, the function returns immediately, so this path is unreachable. The logic is actually correct -- the NULL check for `legacy_lib` returns early, so the unload only happens when `legacy_lib` is valid.

**Correction:** On closer inspection, this is not a bug. The early return on `legacy_lib == NULL` prevents reaching the unload. The patch correctly adds `legacy_lib = NULL` after unload in the error path.

---

### 3. Missing NULL initialization of static provider pointers
**File:** `drivers/crypto/qat/qat_sym_session.c`
**Lines:** 36-37

The static provider pointers are declared but not initialized:
```c
static OSSL_PROVIDER * legacy_lib;
static OSSL_PROVIDER *default_lib;
```

In C, static variables are zero-initialized, so these are implicitly NULL. However, if `ossl_legacy_provider_load()` is called multiple times (e.g., configuring multiple sessions), the pointers may already be non-NULL from a previous call, causing a leak.

**Fix:** Initialize explicitly and check before loading:
```c
static OSSL_PROVIDER *legacy_lib = NULL;
static OSSL_PROVIDER *default_lib = NULL;

static int ossl_legacy_provider_load(void)
{
	if (legacy_lib != NULL)
		return 0;  /* Already loaded */
	
	legacy_lib = OSSL_PROVIDER_load(NULL, "legacy");
	if (legacy_lib == NULL)
		return -EINVAL;
	/* ... */
}
```

**Why it matters:** Multiple session configurations would leak provider handles if not guarded. The current code lacks reference counting or singleton protection.

---

## Warnings

### 1. Inconsistent error message format
**File:** `drivers/common/qat/meson.build`
**Line:** 56

Error message uses `@0@` format specifier but context uses both `@0@` and `.format()` in different ways:
```python
'IPSec_MB version >= @0@ is required, found version @1@'.format(
    IMB_required_ver, imb_ver))
```

For consistency with DPDK style, use:
```python
'missing required dependency, libIPSec_MB >= @0@'.format(IMB_required_ver)
```

---

### 2. OpenSSL optional but not truly optional on ARM
**File:** `drivers/common/qat/meson.build`
**Lines:** 88-98

The code treats OpenSSL as optional on ARM but the documentation states it's required for DOCSIS on ARM. If OpenSSL is missing on ARM, only a warning is printed but `qat_crypto` remains enabled, potentially leading to runtime failures when DOCSIS is attempted.

**Suggested fix:** On ARM, if DOCSIS support is critical, consider making OpenSSL mandatory:
```python
if arch_subdir == 'arm'
    if not libcrypto.found()
        qat_crypto = false
        dpdk_drvs_disabled += qat_crypto_path
        set_variable('drv_' + qat_crypto_path.underscorify() + '_disable_reason',
            'ARM requires OpenSSL >= @0@ for DOCSIS support'.format(openssl_required_ver))
    endif
else
    # x86: OpenSSL is optional
    if libcrypto.found()
        message('QAT: OpenSSL @0@ available for DOCSIS fallback'.format(libcrypto.version()))
    endif
endif
```

Alternatively, if optional is intended, the documentation should clarify that DOCSIS will be unavailable at runtime on ARM without OpenSSL.

---

### 3. OpenSSL OPENSSL_API_COMPAT set to 3.0.0
**File:** `drivers/crypto/qat/qat_sym_session.c`
**Line:** 15

```c
#define OPENSSL_API_COMPAT 0x30000000L
```

This defines the minimum API version to 3.0.0, but the meson.build allows any OpenSSL >= 3.0.0. If future versions deprecate APIs used here (EVP functions), compilation warnings will occur. Consider:
```c
#define OPENSSL_API_COMPAT 0x30200000L  /* 3.2.0 - current LTS */
```
Or leave at 3.0.0 if testing has only been done against that version.

---

### 4. Release note entry location
**File:** `doc/guides/rel_notes/release_26_07.rst`
**Lines:** 158-164

The release note is placed under "New Features" but this is not a feature -- it's a dependency change. It should be under a "Dependency Changes" or "Updated Drivers" section, or at minimum clearly marked as a build requirement change rather than a user-facing feature.

**Suggested revision:**
Move to an "Updated Drivers" or "Known Issues / Dependency Changes" section if one exists, or reword:
```rst
* **Updated QAT crypto PMD build dependencies.**

  The QAT crypto PMD now requires IPsec MB library (v1.4.0+) for HMAC precomputes.
  OpenSSL 3.0+ is optional and used only for DOCSIS BPI cipher fallback.
  Previously, QAT could build with OpenSSL-only on x86; this is no longer supported.
```

---

### 5. Documentation: "both IPsec MB and OpenSSL are required"
**File:** `doc/guides/cryptodevs/qat.rst`
**Line:** 371

```rst
On ARM, both IPsec MB and OpenSSL are required for full functionality.
```

This statement is ambiguous. Does "full functionality" mean DOCSIS only, or does something else break? Be explicit:
```rst
On ARM, IPsec MB is required for HMAC algorithms, and OpenSSL (3.0+) is required
for DOCSIS BPI cipher algorithms. Without OpenSSL, DOCSIS will be unavailable.
```

---

## Info

### 1. Consider rte_free_sensitive() for sensitive data
**File:** `drivers/crypto/qat/qat_sym_session.c`
**Multiple locations in removed code**

The removed OpenSSL code used `rte_free_sensitive()` and `rte_memzero_explicit()` for key material. Verify that the IPsec MB path (`qat_sym_do_precomputes_ipsec_mb()`) also properly clears sensitive buffers before freeing. A quick scan shows `memset_fn()` is used but should be reviewed for completeness.

---

### 2. Removed large block of dead code
**File:** `drivers/crypto/qat/qat_sym_session.c`
**Lines:** 1418-1752 (deleted)

The patch removes ~330 lines of OpenSSL-based HMAC precompute code. This is appropriate given the stated goal (OpenSSL 3.0 removed `SHA*_Transform` APIs), but ensure:
- IPsec MB covers all removed algorithms (SHA1, SHA224, SHA256, SHA384, SHA512, MD5, AES-XCBC, AES-CMAC, GCM)
- Test coverage exists for these paths

The remaining code shows IPsec MB is used for all these, so this is likely fine.

---

### 3. Version check message clarity
**File:** `drivers/common/qat/meson.build`
**Line:** 85

The error message when OpenSSL is not found on x86:
```python
message('QAT: OpenSSL >= @0@ not found - DOCSIS will use IPsec MB only'.format(openssl_required_ver))
```

This is misleading. IPsec MB does not implement DOCSIS BPI cipher (CFB-one-byte). The correct behavior is that DOCSIS will be **unavailable**, not "use IPsec MB only". Fix:
```python
message('QAT: OpenSSL >= @0@ not found - DOCSIS BPI cipher support will be unavailable'.format(openssl_required_ver))
```

---

## FINAL CHECK

### Correctness bugs verified:
-  Checked error paths for resource leaks - found provider unload missing in success path
-  Checked for use-after-free - none found
-  Checked shared variable access - no threading issues in this patch
-  Traced static provider pointer lifecycle - found potential leak on multiple calls

### Style/process false positives removed:
-  Removed initial concern about NULL unload (logic is correct on re-read)
-  Copyright/SPDX not flagged (not subject to AI review)
-  Commit message format not flagged (handled by checkpatch)

---

## Recommendation

**Fix the Error-level issue (provider leak in success path) before merging.** The Warning-level items (OpenSSL optional on ARM, release note placement, message clarity) should be addressed for quality but are not blockers. The potential issue with multiple `ossl_legacy_provider_load()` calls should also be resolved by adding singleton protection or reference counting.