Skip to content

sanitize: cover the remaining second user pointers - #1630

Open
StressTestor wants to merge 3 commits into
axboe:masterfrom
StressTestor:harden/sanitize-second-pointers
Open

sanitize: cover the remaining second user pointers#1630
StressTestor wants to merge 3 commits into
axboe:masterfrom
StressTestor:harden/sanitize-second-pointers

Conversation

@StressTestor

Copy link
Copy Markdown

Three sanitizer coverage gaps, each its own commit.

835f697 ("src/sanitize: check secondary user pointers as well") moved the opcodes carrying a second user pointer in ->addr2 onto sanitize_sqe_addr_and_add2(). Two were missed, and a third gap of the same shape exists in register.c.

  1. IORING_OP_STATX. io_uring_prep_statx() passes statxbuf as the offset argument of io_uring_prep_rw(), so it lands in sqe->off, which is the same union member as addr2. The buffer the kernel writes the statx result into was never checked.

  2. IORING_OP_TIMEOUT_REMOVE. io_uring_prep_timeout_update() puts its struct __kernel_timespec pointer in sqe->addr2, and the opcode was on sanitize_sqe_nop(). This one needs sanitize_sqe_addr2() rather than sanitize_sqe_addr_and_add2(), because the opcode is shared with io_uring_prep_timeout_remove() and both helpers put a user_data value in sqe->addr, not a pointer.

  3. io_uring_register_buffers_tags() / io_uring_register_buffers_update_tag(). Both check the iovec array and never look at tags. Their files counterparts check both. do_register()'s own liburing_sanitize_address(arg) does not cover it, since arg is the address of the on-stack io_uring_rsrc_update2.

On the unconditional addr2 check in (2): io_uring_prep_timeout_remove() passes a literal 0 as the offset and io_uring_prep_rw() always writes sqe->off, so addr2 is deterministically zero on that path rather than stale. __asan_address_is_poisoned(NULL) returns 0, so it stays a no-op there. This matches how IORING_OP_ACCEPT and IORING_OP_SEND_ZC are already handled, where addrlen and the destination address are likewise optional.

Verified that these are the last of this class: I swept every prep helper writing a pointer into off/addr2/addr3, and every opcode currently on sanitize_sqe_nop(). io_uring_prep_timeout_update() was the only pointer-bearing helper among the nop-handled opcodes.

Checked with a small program built against the patched tree, confirming the pointers land where the handlers now look:

timeout_update : opcode=12 addr=0x1234 addr2=0x7f286e2e0b40  ts=0x7f286e2e0b40
timeout_remove : opcode=12 addr=0x1234 addr2=0x0
statx          : addr == path, addr2 == statxbuf

Builds clean on x86_64 both with --enable-sanitizer and with the default configure. No new tests, matching 835f697.

835f697 ("src/sanitize: check secondary user pointers as well") swept the
opcodes carrying a second user pointer in ->addr2 and moved them to
sanitize_sqe_addr_and_add2(). IORING_OP_STATX was missed.

io_uring_prep_statx() passes statxbuf as the offset argument of
io_uring_prep_rw(), which stores it in sqe->off, and off and addr2 are the
same union member. So the buffer the kernel writes the statx result into
sits in exactly the slot that sweep taught the sanitizer to check.

Signed-off-by: Joe Grey <212606152+StressTestor@users.noreply.github.com>
io_uring_prep_timeout_update() writes its struct __kernel_timespec pointer
into sqe->addr2, but IORING_OP_TIMEOUT_REMOVE is handled by
sanitize_sqe_nop() and nothing looks at it.

The opcode is shared with io_uring_prep_timeout_remove(), which is why
sanitize_sqe_addr_and_add2() is the wrong handler here: both helpers put a
user_data value in sqe->addr, not a pointer. sanitize_sqe_addr2() checks
only the slot that actually holds one. The remove variant leaves addr2 at
zero, and __asan_address_is_poisoned(NULL) is 0, so it stays a no-op there.

Signed-off-by: Joe Grey <212606152+StressTestor@users.noreply.github.com>
io_uring_register_files_update_tag() and io_uring_register_files_tags()
check both of their user pointers. Their buffers counterparts check only
the iovec array and never look at tags.

do_register()'s own liburing_sanitize_address(arg) does not cover it,
since arg is the address of the on-stack io_uring_rsrc_update2 or
io_uring_rsrc_register, not of the caller's tags array. A NULL tags
argument is legal and passes through liburing_sanitize_address()
unharmed, as it already does on the files paths.

Signed-off-by: Joe Grey <212606152+StressTestor@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant