|WARNING| [v2,2/2] net/nfp: move socket from /tmp to runtime dir

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch series moves Unix socket files from world-writable `/tmp` to the EAL runtime directory for improved isolation and security in the cnxk and nfp drivers.

---

## Patch 1/2: net/cnxk: move socket from /tmp to runtime dir

### Errors

**Use-after-free in `cnxk_eswitch_dev_remove()`**

In `drivers/net/cnxk/cnxk_eswitch.c`, after calling `cnxk_eswitch_ctrl_msg_sock_addr(&sun)`, the function calls `close(sock_fd)` on error, then returns, **but `sock_fd` was already closed on the previous error path and may be closed again by the caller or later code**.

Wait, re-reading the code:

```c
rc = cnxk_eswitch_ctrl_msg_sock_addr(&sun);
if (rc) {
    plt_err("Control message socket path too long");
    close(sock_fd);
    return rc;
}
```

This is not a use-after-free -- the `sock_fd` is only closed once on this path. However, **there is a resource leak**: the original code after line 140 opens `sock_fd` and on error closes it, but the **`sun` variable is uninitialized** when passed to `connect()` on the success path of the original code.

Actually, looking more carefully:

**Original code** (before patch):
- Opens socket
- Sets `sun.sun_family = AF_UNIX`
- Calls `strncpy(sun.sun_path, ...)`
- Calls `connect(sock_fd, (struct sockaddr *)&sun, sunlen)`

**New code** (after patch):
- Opens socket
- Calls `cnxk_eswitch_ctrl_msg_sock_addr(&sun)` which calls `memset(&sun, 0, sizeof(*sun))` and sets `sun_family` and `sun_path`
- If that returns error, closes socket and returns error
- Otherwise calls `connect(sock_fd, (struct sockaddr *)&sun, sunlen)`

This looks correct. The `sunlen` is set before the function call, so there's no issue there.

Let me re-check `drivers/net/cnxk/cnxk_rep_msg.c`:

In `open_socket_ctrl_channel()`, the new code calls `cnxk_eswitch_ctrl_msg_sock_addr(&un)` which already does `memset(&un, 0, sizeof(*un))`, but then **the old code after that had another `memset(&un, 0, sizeof(struct sockaddr_un))`** which is now removed. The patch correctly removes the redundant second `memset`.

Actually wait, looking at the patch again:

```diff
-	memset(&un, 0, sizeof(un));
-	un.sun_family = AF_UNIX;
-
-	if (strlen(CNXK_ESWITCH_CTRL_MSG_SOCK_PATH) > sizeof(un.sun_path) - 1) {
-		plt_err("Server socket path too long: %s", CNXK_ESWITCH_CTRL_MSG_SOCK_PATH);
+	/* Set unix socket path under the runtime dir and bind */
+	ret = cnxk_eswitch_ctrl_msg_sock_addr(&un);
+	if (ret) {
+		plt_err("Server socket path too long");
 		close(sock_fd);
-		return -E2BIG;
+		return ret;
 	}
 
-	if (remove(CNXK_ESWITCH_CTRL_MSG_SOCK_PATH) == -1 && errno != ENOENT) {
-		plt_err("remove-%s", CNXK_ESWITCH_CTRL_MSG_SOCK_PATH);
+	if (remove(un.sun_path) == -1 && errno != ENOENT) {
+		plt_err("remove-%s", un.sun_path);
 		close(sock_fd);
 		return -errno;
 	}
 
-	memset(&un, 0, sizeof(struct sockaddr_un));
-	un.sun_family = AF_UNIX;
-	strncpy(un.sun_path, CNXK_ESWITCH_CTRL_MSG_SOCK_PATH, sizeof(un.sun_path) - 1);
-
 	if (bind(sock_fd, (struct sockaddr *)&un, sizeof(un)) < 0) {
```

The second `memset` (line 140 in the original) is removed. This is correct because `cnxk_eswitch_ctrl_msg_sock_addr()` already initialized `un` fully. No correctness issue here.

Re-checking `close_socket()`:

```c
static void
close_socket(int fd)
{
	struct sockaddr_un un;

	close(fd);
	if (cnxk_eswitch_ctrl_msg_sock_addr(&un) == 0)
		unlink(un.sun_path);
}
```

This closes the socket first, then unlinks the file. That's the correct order (close before unlink). No issue.

**Missing error check on `remove()` (Warning level, not Error):**

In `open_socket_ctrl_channel()`, the `remove(un.sun_path)` call checks `errno != ENOENT` but does not validate the path returned by `cnxk_eswitch_ctrl_msg_sock_addr()` was successfully constructed. However, the function **does** check the return value of `cnxk_eswitch_ctrl_msg_sock_addr(&un)` just before the `remove()`, so if the path construction failed, the function already returned early. This is correct.

Actually, I don't see any correctness bugs in patch 1/2 now. Let me double-check the style issues.

### Warnings

**`snprintf` return value check pattern**

```c
ret = snprintf(un->sun_path, sizeof(un->sun_path), "%s/%s",
               rte_eal_get_runtime_dir(), CNXK_ESWITCH_CTRL_MSG_SOCK_NAME);
if (ret < 0 || (size_t)ret >= sizeof(un->sun_path))
    return -ENAMETOOLONG;
```

The check correctly validates both negative return (encoding error) and truncation (`ret >= size`). This is good practice.

**Error code consistency**

In `open_socket_ctrl_channel()`, the original code returned `-E2BIG` for path-too-long. The new code returns the error from `cnxk_eswitch_ctrl_msg_sock_addr()`, which is `-ENAMETOOLONG`. This is a more appropriate error code for the condition (path truncation), so this is an improvement.

---

## Patch 2/2: net/nfp: move socket from /tmp to runtime dir

### Errors

**Missing `close(sockfd)` on `snprintf` failure**

In `nfp_cpp_bridge_service_func()`:

```c
unlink(address.sun_path);
sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
if (sockfd < 0) {
    PMD_CPP_LOG(ERR, "Socket creation error. Service failed.");
    return -errno;
}
```

The socket is opened **before** the path is constructed. Then:

```c
ret = snprintf(address.sun_path, sizeof(address.sun_path), "%s/nfp_%s",
        rte_eal_get_runtime_dir(), pci_name);
if (ret < 0 || (size_t)ret >= sizeof(address.sun_path)) {
    PMD_CPP_LOG(ERR, "Socket path too long. Service failed.");
    return -EINVAL;
}
```

Wait, re-reading the patch:

```diff
-	snprintf(socket_handle, sizeof(socket_handle), "/tmp/%s", pci_name);
+	ret = snprintf(address.sun_path, sizeof(address.sun_path), "%s/nfp_%s",
+			rte_eal_get_runtime_dir(), pci_name);
+	if (ret < 0 || (size_t)ret >= sizeof(address.sun_path)) {
+		PMD_CPP_LOG(ERR, "Socket path too long. Service failed.");
+		return -EINVAL;
+	}

-	unlink(socket_handle);
+	unlink(address.sun_path);
 	sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
```

The `snprintf` and its error check happen **before** `socket()` is called, so there's no leak on the `snprintf` error path. The code is correct.

Actually wait, looking at the actual patch context lines more carefully:

>From the patch:
```c
pci_name = strchr(pf_dev->pci_dev->name, ':') + 1;

+	ret = snprintf(address.sun_path, sizeof(address.sun_path), "%s/nfp_%s",
+			rte_eal_get_runtime_dir(), pci_name);
+	if (ret < 0 || (size_t)ret >= sizeof(address.sun_path)) {
+		PMD_CPP_LOG(ERR, "Socket path too long. Service failed.");
+		return -EINVAL;
+	}
+
+	unlink(address.sun_path);
 	sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
```

Yes, the socket is created **after** the snprintf, so no leak on that error path.

**Potential NULL pointer dereference**

```c
pci_name = strchr(pf_dev->pci_dev->name, ':') + 1;
```

If `strchr()` returns `NULL` (no colon in the PCI device name), this will dereference `NULL + 1` - undefined behavior.

This should check the return value:

```c
pci_name = strchr(pf_dev->pci_dev->name, ':');
if (pci_name == NULL) {
    PMD_CPP_LOG(ERR, "Invalid PCI device name format");
    return -EINVAL;
}
pci_name++; /* skip the ':' */
```

**However**, this line of code **exists in the original version** and is not introduced by this patch. The patch does not make it worse. According to the guidelines, we review the patch as submitted, not the entire file. But this is a **latent bug exposed by the patch** because the new path format uses `pci_name` in a safety-critical context (path construction with bounds check), so if `pci_name` is garbage due to the NULL dereference, the behavior is undefined.

Actually, wait: the original code was:
```c
snprintf(socket_handle, sizeof(socket_handle), "/tmp/%s", pci_name);
```

If `pci_name` is `NULL + 1`, this would **also** be undefined behavior in the original code. So this is a **pre-existing bug**, not introduced by the patch. Do we flag it?

The guidelines say: "Review patches for compliance with DPDK coding standards." If the patch doesn't touch the buggy line, we typically don't flag it, unless the patch makes it worse or adds new uses of the buggy value.

In this case, the patch **does not change** the `pci_name = strchr(...) + 1` line, but it **does add a new use** of `pci_name` in a bounds-checked snprintf, which means the impact of the bug (if it triggers) might change. However, both the old and new code would exhibit undefined behavior if `strchr` returns NULL.

I'll flag this as a **Warning** because it's a pre-existing issue that the patch doesn't fix, but the patch's new bounds checking makes the code slightly safer overall (at least truncation is caught), even though the NULL deref is still there.

Actually, let me reconsider: the guidelines say "Review patches for compliance" and "Correctness bugs are the highest priority." If the patch adds new code that **uses** a value that could be NULL+1, even if the NULL+1 assignment is pre-existing, the **use** in the new code is part of the patch's correctness.

But the old code **also used** `pci_name` in `snprintf(socket_handle, "%s", pci_name)`, so the use is not new -- only the format string changed.

I think the most accurate classification is: this is a **pre-existing bug** that the patch does not fix, and flagging it would be pointing out a bug in code the patch doesn't modify. The guidelines say to focus on the patch itself.

However, since the patch is a security improvement (moving sockets out of /tmp), it would be prudent to also fix the NULL deref in the same area. I'll note this as an **Info** item, not an Error.

Actually, re-reading the guidelines again:

> **Error** (must fix):
> - NULL pointer dereference on reachable code path

The strchr+1 pattern is a NULL pointer dereference if the colon is not found. The code path is reachable if an attacker can control the PCI device name (unlikely) or if there's a malformed PCI device. In a production DPDK system, PCI device names should always have colons, but defensive programming would check.

But again: **the patch does not introduce this bug**. The patch does not modify that line. So I will **not flag it as an Error** because it's not part of the patch's changes. I'll mention it as an **Info** item for completeness.

### Warnings

None beyond what's already noted.

---

## Info

**Patch 2/2: Pre-existing NULL dereference risk**

```c
pci_name = strchr(pf_dev->pci_dev->name, ':') + 1;
```

This line (unchanged by the patch) will dereference NULL+1 if the PCI device name does not contain a colon. Consider adding a NULL check in a follow-up patch:

```c
pci_name = strchr(pf_dev->pci_dev->name, ':');
if (pci_name == NULL)
    return -EINVAL;
pci_name++;
```

---

## General Observations

**Patch 1/2:**
- Correctly refactors socket path construction into a helper function
- Properly checks `snprintf` return value for truncation
- Error paths correctly clean up resources (close socket, return error code)
- No resource leaks identified

**Patch 2/2:**
- Correctly moves path construction and validation before socket creation
- Uses `struct sockaddr_un` instead of truncating into `struct sockaddr` (14-byte `sa_data`)
- Properly validates `snprintf` for both error and truncation

Both patches follow the DPDK C coding style (lowercase with underscores, hard tabs, proper function formatting).

**Security improvement:** Moving sockets from `/tmp` to the EAL runtime directory is a meaningful security hardening, as it prevents cross-instance interference and leverages per-prefix isolation.

---

## Conclusion

**Patch 1/2:** No correctness bugs found. Code is correct.

**Patch 2/2:** No correctness bugs introduced by the patch. A pre-existing NULL dereference risk exists in the `strchr() + 1` pattern but is not part of this patch's changes.

Both patches are well-structured and improve security without introducing new bugs.