|WARNING| security: harden telemetry parameter parsing

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch: security: harden telemetry parameter parsing

### Summary
This patch improves input validation in the security telemetry handlers by replacing unsafe parsing with a validated helper function and removing thread-unsafe `strtok()`.

---

## Findings

### Errors

None.

### Warnings

**1. Missing upper bound validation on capa_id**

In `security_handle_cryptodev_crypto_caps()`, the parsed `capa_id` is validated only against `INT_MAX`, but there is no check that it is within the valid range of capabilities indices before it is used. The code later iterates through the capabilities array, but if `capa_id` exceeds the actual number of capabilities, this could lead to out-of-bounds access.

**Suggested fix:** Add a bounds check after parsing `capa_id` and before using it as an index, similar to how `dev_id` is validated against `RTE_CRYPTO_MAX_DEVS - 1`.

```c
capa_id = telemetry_parse_uint(end_param + 1, &end_param, INT_MAX);
if (capa_id < 0 || *end_param != '\0')
	return -EINVAL;

rc = security_capabilities_from_dev_id(dev_id, (void *)&capabilities);
if (rc < 0)
	return rc;

/* Validate capa_id is within bounds */
crypto_caps_n = 0;
while (capabilities[crypto_caps_n].action != RTE_SECURITY_ACTION_TYPE_NONE)
	crypto_caps_n++;
if (capa_id >= crypto_caps_n)
	return -EINVAL;
```

**2. Cast from unsigned long to int could theoretically overflow on exotic platforms**

In `telemetry_parse_uint()`, the function casts `val` (unsigned long) to int after checking `val > max`. While the function enforces that `max <= INT_MAX` in the documentation, and the callers pass appropriate max values, there is no compile-time or runtime assertion that `max <= INT_MAX` is respected. On a platform where `unsigned long` is wider than `int` (though this is not a realistic concern for current DPDK targets), a logic error in a future caller could pass `max > INT_MAX`, resulting in an overflow on the return cast.

**Suggested fix:** Add a compile-time or runtime assertion in `telemetry_parse_uint()`:

```c
static int
telemetry_parse_uint(const char *str, char **end, unsigned long max)
{
	unsigned long val;

	RTE_VERIFY(max <= INT_MAX);  /* or static_assert if in C11+ context */

	if (str == NULL || !isdigit((unsigned char)*str))
		return -EINVAL;

	errno = 0;
	val = strtoul(str, end, 0);
	if (errno != 0 || val > max)
		return -EINVAL;

	return (int)val;
}
```

This ensures that any future misuse of the function is caught at build time or early at runtime.

### Info

**1. The patch correctly fixes the unsigned char cast for isdigit()**

The original code called `isdigit(*params)` on a plain `char`, which is undefined behavior when the char value is negative (high-bit set). The patch correctly casts to `unsigned char` in the helper function: `isdigit((unsigned char)*str)`. This is the correct fix.

**2. The patch correctly replaces strtok() with manual parsing**

The original code used `strtok()` which is not thread-safe. The new code manually parses the comma separator by checking `*end_param != ','` and passing `end_param + 1` to the next parse call. This is thread-safe and correct.

**3. Error handling is improved**

The patch replaces silent truncation and log-and-continue behavior with explicit error returns, which is the correct approach for telemetry parameter validation.

---

## Positive Observations

- The patch eliminates unsafe `strtok()` usage
- Input validation is significantly strengthened
- The helper function `telemetry_parse_uint()` is reusable and well-structured
- Error handling is made more explicit and robust
- The `isdigit()` cast to `unsigned char` is correct and fixes undefined behavior

---

## Conclusion

The patch is a significant improvement in security and correctness. The two warnings above should be addressed:

1. Add bounds checking on `capa_id` after obtaining the actual capability array size
2. Add an assertion to `telemetry_parse_uint()` to enforce the `max <= INT_MAX` contract

With these changes, the patch would fully harden the telemetry parameter parsing as intended.