diff --git a/scripts/pre-commit.hook b/scripts/pre-commit.hook index 72a3d3b..0fe0ff0 100755 --- a/scripts/pre-commit.hook +++ b/scripts/pre-commit.hook @@ -183,6 +183,9 @@ build_cppcheck_suppressions() { "syntaxError:include/kbox/compiler.h" "syntaxError:src/loader-transfer.c" "invalidFunctionArg:src/rewrite.c" + "invalidFunctionArg:tests/unit/test-procmem.c" + "nullPointerArithmeticOutOfMemory:tests/unit/test-procmem.c" + "nullPointerOutOfMemory:tests/unit/test-procmem.c" "knownConditionTrueFalse" "usleepCalled" ) diff --git a/src/dispatch-misc.c b/src/dispatch-misc.c index c6452ce..e6b4478 100644 --- a/src/dispatch-misc.c +++ b/src/dispatch-misc.c @@ -475,20 +475,30 @@ struct kbox_dispatch forward_pwrite64(const struct kbox_syscall_request *req, if (chunk_len > count - total) chunk_len = count - total; - uint64_t remote = remote_buf + total; + uint64_t remote; + if (__builtin_add_overflow(remote_buf, (uint64_t) total, &remote)) { + if (total == 0) + return kbox_dispatch_errno(EFAULT); + break; + } int rrc = guest_mem_read(ctx, pid, remote, scratch, chunk_len); if (rrc < 0) { - if (total > 0) - break; - return kbox_dispatch_errno(-rrc); + if (total == 0) + return kbox_dispatch_errno(-rrc); + break; } + long pwrite_off; + if (__builtin_add_overflow(offset, (long) total, &pwrite_off)) { + if (total == 0) + return kbox_dispatch_errno(EOVERFLOW); + break; + } long ret = kbox_lkl_pwrite64(ctx->sysnrs, lkl_fd, scratch, - (long) chunk_len, offset + (long) total); + (long) chunk_len, pwrite_off); if (ret < 0) { - if (total == 0) { + if (total == 0) return kbox_dispatch_errno((int) (-ret)); - } break; } @@ -577,9 +587,14 @@ static struct kbox_dispatch dispatch_iov_transfer( if (chunk > len - seg_total) chunk = len - seg_total; + uint64_t remote; + if (__builtin_add_overflow(base, seg_total, &remote)) { + err = EFAULT; + goto done; + } + if (is_write) { - rrc = - guest_mem_read(ctx, pid, base + seg_total, scratch, chunk); + rrc = guest_mem_read(ctx, pid, remote, scratch, chunk); if (rrc < 0) { err = -rrc; goto done; @@ -605,8 +620,7 @@ static struct kbox_dispatch dispatch_iov_transfer( (void) written; } } else { - int wrc = - guest_mem_write(ctx, pid, base + seg_total, scratch, n); + int wrc = guest_mem_write(ctx, pid, remote, scratch, n); if (wrc < 0) return kbox_dispatch_errno(-wrc); } diff --git a/src/elf.c b/src/elf.c index 2072142..5baebe5 100644 --- a/src/elf.c +++ b/src/elf.c @@ -176,21 +176,25 @@ int kbox_find_elf_interp_loc(const unsigned char *buf, if (p_offset >= buf_len) return -1; + /* Reject if PT_INTERP segment extends beyond the mapped ELF. */ uint64_t end; if (__builtin_add_overflow(p_offset, p_filesz, &end) || end > buf_len) - end = buf_len; + return -1; const unsigned char *s = buf + p_offset; - size_t slen = (size_t) (end - p_offset); + size_t slen = (size_t) p_filesz; - if (slen > 0 && s[slen - 1] == '\0') - slen--; + /* Require NUL terminator within the segment. */ + if (slen == 0 || s[slen - 1] != '\0') + return -1; + slen--; /* exclude trailing NUL from length */ if (slen == 0) - return 0; + return -1; + /* Reject if path does not fit in caller's buffer. */ if (slen >= out_size) - slen = out_size - 1; + return -1; memcpy(out, s, slen); out[slen] = '\0'; diff --git a/src/procmem.c b/src/procmem.c index c781f54..aa485ce 100644 --- a/src/procmem.c +++ b/src/procmem.c @@ -339,14 +339,18 @@ int kbox_vm_read_string(pid_t pid, local_iov.iov_base = buf + total; local_iov.iov_len = chunk; - remote_iov.iov_base = - (void *) (uintptr_t) (remote_addr + (uint64_t) total); + uint64_t remote; + if (__builtin_add_overflow(remote_addr, (uint64_t) total, &remote)) + return -EFAULT; + remote_iov.iov_base = (void *) (uintptr_t) remote; remote_iov.iov_len = chunk; n = syscall(SYS_process_vm_readv, pid, &local_iov, 1, &remote_iov, 1, 0); - if (n <= 0) - return errno ? -errno : -EIO; + if (n < 0) + return -errno; + if (n == 0) + return -EFAULT; for (i = 0; i < (size_t) n; i++) { if (buf[total + i] == '\0') diff --git a/src/seccomp-dispatch.c b/src/seccomp-dispatch.c index 90a2dfb..22a8bcd 100644 --- a/src/seccomp-dispatch.c +++ b/src/seccomp-dispatch.c @@ -1218,8 +1218,13 @@ static struct kbox_dispatch forward_local_shadow_read_like( chunk_len = count - total; if (is_pread) { long offset = to_c_long_arg(kbox_syscall_request_arg(req, 3)); - nr = pread(entry->shadow_sp, scratch, chunk_len, - (off_t) (offset + (long) total)); + long pread_off; + if (__builtin_add_overflow(offset, (long) total, &pread_off)) { + if (total == 0) + return kbox_dispatch_errno(EOVERFLOW); + break; + } + nr = pread(entry->shadow_sp, scratch, chunk_len, (off_t) pread_off); } else { nr = read(entry->shadow_sp, scratch, chunk_len); } @@ -1230,8 +1235,13 @@ static struct kbox_dispatch forward_local_shadow_read_like( } if (nr == 0) break; - if (guest_mem_write(ctx, pid, remote_buf + total, scratch, - (size_t) nr) < 0) { + uint64_t remote; + if (__builtin_add_overflow(remote_buf, (uint64_t) total, &remote)) { + if (total == 0) + return kbox_dispatch_errno(EFAULT); + break; + } + if (guest_mem_write(ctx, pid, remote, scratch, (size_t) nr) < 0) { return kbox_dispatch_errno(EFAULT); } total += (size_t) nr; @@ -2113,16 +2123,21 @@ static struct kbox_dispatch forward_read_like( long ret; if (is_pread) { long offset = to_c_long_arg(kbox_syscall_request_arg(req, 3)); + long pread_off; + if (__builtin_add_overflow(offset, (long) total, &pread_off)) { + if (total == 0) + return kbox_dispatch_errno(EOVERFLOW); + break; + } ret = kbox_lkl_pread64(ctx->sysnrs, lkl_fd, scratch, - (long) chunk_len, offset + (long) total); + (long) chunk_len, pread_off); } else { ret = kbox_lkl_read(ctx->sysnrs, lkl_fd, scratch, (long) chunk_len); } if (ret < 0) { - if (total == 0) { + if (total == 0) return kbox_dispatch_errno((int) (-ret)); - } break; } @@ -2130,7 +2145,12 @@ static struct kbox_dispatch forward_read_like( if (n == 0) break; - uint64_t remote = remote_buf + total; + uint64_t remote; + if (__builtin_add_overflow(remote_buf, (uint64_t) total, &remote)) { + if (total == 0) + return kbox_dispatch_errno(EFAULT); + break; + } if (ctx->verbose) { fprintf( stderr, @@ -2197,20 +2217,24 @@ static struct kbox_dispatch forward_write( if (chunk_len > count - total) chunk_len = count - total; - uint64_t remote = remote_buf + total; + uint64_t remote; + if (__builtin_add_overflow(remote_buf, (uint64_t) total, &remote)) { + if (total == 0) + return kbox_dispatch_errno(EFAULT); + break; + } int rrc = guest_mem_read(ctx, pid, remote, scratch, chunk_len); if (rrc < 0) { - if (total > 0) - break; - return kbox_dispatch_errno(-rrc); + if (total == 0) + return kbox_dispatch_errno(-rrc); + break; } long ret = kbox_lkl_write(ctx->sysnrs, lkl_fd, scratch, (long) chunk_len); if (ret < 0) { - if (total == 0) { + if (total == 0) return kbox_dispatch_errno((int) (-ret)); - } break; } @@ -2317,16 +2341,22 @@ static struct kbox_dispatch forward_sendfile( /* Read from source (LKL fd). */ long nr; - if (has_offset) + if (has_offset) { + long pread_off; + if (__builtin_add_overflow(offset, (long) total, &pread_off)) { + if (total == 0) + return kbox_dispatch_errno(EOVERFLOW); + break; + } nr = kbox_lkl_pread64(ctx->sysnrs, in_lkl, scratch, (long) chunk, - offset + (long) total); - else + pread_off); + } else { nr = kbox_lkl_read(ctx->sysnrs, in_lkl, scratch, (long) chunk); + } if (nr < 0) { - if (total == 0) { + if (total == 0) return kbox_dispatch_errno((int) (-nr)); - } break; } if (nr == 0) @@ -2334,38 +2364,47 @@ static struct kbox_dispatch forward_sendfile( size_t n = (size_t) nr; - /* Write to destination. */ - if (out_lkl >= 0) { - long wr = kbox_lkl_write(ctx->sysnrs, out_lkl, scratch, (long) n); - if (wr < 0) { - if (total == 0) { - return kbox_dispatch_errno((int) (-wr)); + /* Write to destination, looping on short writes. */ + size_t written = 0; + while (written < n) { + if (out_lkl >= 0) { + long wr = + kbox_lkl_write(ctx->sysnrs, out_lkl, scratch + written, + (long) (n - written)); + if (wr <= 0) { + if (total + written == 0) + return kbox_dispatch_errno(wr < 0 ? (int) (-wr) : EIO); + total += written; + goto done; } - break; - } - } else { - /* Destination is a host FD (e.g. stdout). The supervisor - * shares the FD table with the tracee (from fork), so write() - * goes to the same file description. - */ - ssize_t wr = write((int) out_fd, scratch, n); - if (wr < 0) { - if (total == 0) { - return kbox_dispatch_errno(errno); + written += (size_t) wr; + } else { + ssize_t wr = + write((int) out_fd, scratch + written, n - written); + if (wr <= 0) { + if (total + written == 0) + return kbox_dispatch_errno(wr < 0 ? errno : EIO); + total += written; + goto done; } - break; + written += (size_t) wr; } } - total += n; + total += written; if (n < chunk) break; } - /* Update offset in tracee memory if provided. */ +done: + /* Update offset in tracee memory if provided. Best-effort: data has + * already been transferred, so return the byte count even if the + * offset writeback fails (avoids data duplication on retry). + */ if (has_offset && total > 0) { - off_t new_off = offset + (off_t) total; - guest_mem_write(ctx, pid, offset_ptr, &new_off, sizeof(new_off)); + off_t new_off; + if (!__builtin_add_overflow(offset, (off_t) total, &new_off)) + guest_mem_write(ctx, pid, offset_ptr, &new_off, sizeof(new_off)); } return kbox_dispatch_value((int64_t) total); diff --git a/tests/unit/test-elf.c b/tests/unit/test-elf.c index 03c6f9d..306eedc 100644 --- a/tests/unit/test-elf.c +++ b/tests/unit/test-elf.c @@ -576,6 +576,92 @@ static void test_elf_build_load_plan_rejects_filesz_gt_memsz(void) ASSERT_EQ(kbox_build_elf_load_plan(elf, sizeof(elf), 0x1000, &plan), -1); } +/* PT_INTERP segment extends beyond the ELF buffer (p_offset + p_filesz > + * buf_len). */ +static void test_elf_interp_rejects_segment_overflow(void) +{ + unsigned char elf[256]; + char out[256]; + + init_elf64(elf, sizeof(elf), ET_EXEC, 0x3e, 0, 64, 1); + /* p_offset=200, p_filesz=100: 200+100=300 > 256 */ + set_phdr(elf, 0, PT_INTERP, 0, 200, 0, 100, 100, 1); + + ASSERT_EQ(kbox_parse_elf_interp(elf, sizeof(elf), out, sizeof(out)), -1); +} + +/* PT_INTERP string missing NUL terminator within the segment. */ +static void test_elf_interp_rejects_no_nul(void) +{ + unsigned char elf[256]; + char out[256]; + + init_elf64(elf, sizeof(elf), ET_EXEC, 0x3e, 0, 64, 1); + /* Place 4-byte PT_INTERP at offset 200, no NUL byte. */ + set_phdr(elf, 0, PT_INTERP, 0, 200, 0, 4, 4, 1); + elf[200] = '/'; + elf[201] = 'l'; + elf[202] = 'd'; + elf[203] = 'x'; /* no NUL */ + + ASSERT_EQ(kbox_parse_elf_interp(elf, sizeof(elf), out, sizeof(out)), -1); +} + +/* PT_INTERP path too large for caller's output buffer. */ +static void test_elf_interp_rejects_path_too_large(void) +{ + unsigned char elf[256]; + char out[4]; /* too small for "/ld.so" */ + + init_elf64(elf, sizeof(elf), ET_EXEC, 0x3e, 0, 64, 1); + set_phdr(elf, 0, PT_INTERP, 0, 200, 0, 7, 7, 1); + memcpy(elf + 200, "/ld.so", 7); /* 6 chars + NUL = 7 bytes */ + + ASSERT_EQ(kbox_parse_elf_interp(elf, sizeof(elf), out, sizeof(out)), -1); +} + +/* PT_INTERP with empty string (just NUL) is rejected as malformed. */ +static void test_elf_interp_rejects_empty_path(void) +{ + unsigned char elf[256]; + char out[256]; + + init_elf64(elf, sizeof(elf), ET_EXEC, 0x3e, 0, 64, 1); + set_phdr(elf, 0, PT_INTERP, 0, 200, 0, 1, 1, 1); + elf[200] = '\0'; /* just a NUL byte */ + + ASSERT_EQ(kbox_parse_elf_interp(elf, sizeof(elf), out, sizeof(out)), -1); +} + +/* PT_INTERP with zero p_filesz is rejected. */ +static void test_elf_interp_rejects_zero_filesz(void) +{ + unsigned char elf[256]; + char out[256]; + + init_elf64(elf, sizeof(elf), ET_EXEC, 0x3e, 0, 64, 1); + set_phdr(elf, 0, PT_INTERP, 0, 200, 0, 0, 0, 1); + + ASSERT_EQ(kbox_parse_elf_interp(elf, sizeof(elf), out, sizeof(out)), -1); +} + +/* PT_INTERP with p_offset + p_filesz integer overflow is rejected. + * p_offset is valid (within buf_len) but p_filesz is huge, causing + * the sum to wrap around UINT64. This exercises the __builtin_add_overflow + * guard rather than the simpler p_offset >= buf_len check. + */ +static void test_elf_interp_rejects_offset_filesz_overflow(void) +{ + unsigned char elf[256]; + char out[256]; + + init_elf64(elf, sizeof(elf), ET_EXEC, 0x3e, 0, 64, 1); + /* p_offset=200 (valid), p_filesz=UINT64_MAX: 200 + UINT64_MAX wraps. */ + set_phdr(elf, 0, PT_INTERP, 0, 200, 0, UINT64_MAX, UINT64_MAX, 1); + + ASSERT_EQ(kbox_parse_elf_interp(elf, sizeof(elf), out, sizeof(out)), -1); +} + void test_elf_init(void) { TEST_REGISTER(test_elf_parse_interp); @@ -591,4 +677,10 @@ void test_elf_init(void) TEST_REGISTER(test_elf_build_load_plan_pie_with_phdr_and_stack); TEST_REGISTER(test_elf_build_load_plan_honors_large_segment_align); TEST_REGISTER(test_elf_build_load_plan_rejects_filesz_gt_memsz); + TEST_REGISTER(test_elf_interp_rejects_segment_overflow); + TEST_REGISTER(test_elf_interp_rejects_no_nul); + TEST_REGISTER(test_elf_interp_rejects_path_too_large); + TEST_REGISTER(test_elf_interp_rejects_empty_path); + TEST_REGISTER(test_elf_interp_rejects_zero_filesz); + TEST_REGISTER(test_elf_interp_rejects_offset_filesz_overflow); } diff --git a/tests/unit/test-procmem.c b/tests/unit/test-procmem.c index 4abc21c..ee00e80 100644 --- a/tests/unit/test-procmem.c +++ b/tests/unit/test-procmem.c @@ -121,6 +121,99 @@ static void test_vm_write_force_rejects_bad_pointer(void) ASSERT_EQ(kbox_vm_write_force(getpid(), 0, NULL, 0), 0); } +/* kbox_vm_read_string returns -EFAULT on unmapped memory (not stale errno). */ +static void test_vm_read_string_unmapped_returns_efault(void) +{ + long page_size = sysconf(_SC_PAGESIZE); + char *mapping; + char buf[64]; + pid_t pid; + int status = 0; + + ASSERT_TRUE(page_size > 0); + mapping = mmap(NULL, (size_t) page_size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + ASSERT_NE(mapping, MAP_FAILED); + ASSERT_EQ(munmap(mapping, (size_t) page_size), 0); + + /* Fork so process_vm_readv targets our own child (same address space + * layout after fork, but the mapping is unmapped in the child too). + */ + pid = fork(); + ASSERT_TRUE(pid >= 0); + if (pid == 0) { + int rc = kbox_vm_read_string(getpid(), (uint64_t) (uintptr_t) mapping, + buf, sizeof(buf)); + _exit(rc == -EFAULT ? 0 : 1); + } + + ASSERT_EQ(waitpid(pid, &status, 0), pid); + ASSERT_TRUE(WIFEXITED(status)); + ASSERT_EQ(WEXITSTATUS(status), 0); +} + +/* kbox_vm_read_string on valid NUL-terminated string returns length. */ +static void test_vm_read_string_valid(void) +{ + char buf[64]; + const char *src = "hello"; + int rc = kbox_vm_read_string(getpid(), (uint64_t) (uintptr_t) src, buf, + sizeof(buf)); + ASSERT_EQ(rc, 5); + ASSERT_STREQ(buf, "hello"); +} + +/* kbox_vm_read_string rejects NULL pointer. */ +static void test_vm_read_string_null_returns_efault(void) +{ + char buf[64]; + ASSERT_EQ(kbox_vm_read_string(getpid(), 0, buf, sizeof(buf)), -EFAULT); +} + +/* Cross-page short read: string without NUL at end of readable page, + * next page unmapped. process_vm_readv returns a short read (< chunk), + * and since no NUL was found, kbox_vm_read_string returns -EFAULT. + */ +static void test_vm_read_string_cross_page_short_read(void) +{ + long page_size = sysconf(_SC_PAGESIZE); + char *mapping; + char buf[64]; + pid_t pid; + int status = 0; + + ASSERT_TRUE(page_size > 0); + + /* Map two pages, then unmap the second so reads crossing the boundary + * produce a short read from process_vm_readv. + */ + mapping = mmap(NULL, (size_t) page_size * 2, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + ASSERT_NE(mapping, MAP_FAILED); + + /* Fill end of first page with non-NUL bytes (no terminator). */ + memset(mapping + page_size - 4, 'A', 4); + ASSERT_EQ(munmap(mapping + page_size, (size_t) page_size), 0); + + pid = fork(); + ASSERT_TRUE(pid >= 0); + if (pid == 0) { + /* Read starting 4 bytes before page boundary. The first chunk + * succeeds (short read of 4 bytes, no NUL), then the next chunk + * hits unmapped memory -> returns -EFAULT. + */ + int rc = kbox_vm_read_string( + getpid(), (uint64_t) (uintptr_t) (mapping + page_size - 4), buf, + sizeof(buf)); + _exit(rc == -EFAULT ? 0 : 1); + } + + ASSERT_EQ(waitpid(pid, &status, 0), pid); + ASSERT_TRUE(WIFEXITED(status)); + ASSERT_EQ(WEXITSTATUS(status), 0); + munmap(mapping, (size_t) page_size); +} + void test_procmem_init(void) { TEST_REGISTER(test_current_guest_mem_read_write); @@ -130,4 +223,8 @@ void test_procmem_init(void) TEST_REGISTER(test_current_guest_mem_force_write_cross_page); TEST_REGISTER(test_current_guest_mem_unmapped_pointer_returns_error); TEST_REGISTER(test_vm_write_force_rejects_bad_pointer); + TEST_REGISTER(test_vm_read_string_unmapped_returns_efault); + TEST_REGISTER(test_vm_read_string_valid); + TEST_REGISTER(test_vm_read_string_null_returns_efault); + TEST_REGISTER(test_vm_read_string_cross_page_short_read); } diff --git a/tests/unit/test-runner.c b/tests/unit/test-runner.c index 4df1ccb..ed105c1 100644 --- a/tests/unit/test-runner.c +++ b/tests/unit/test-runner.c @@ -10,7 +10,7 @@ #include -#define MAX_TESTS 256 +#define MAX_TESTS 512 struct test_entry { const char *name;