sanitize: cover the remaining second user pointers - #1630
Open
StressTestor wants to merge 3 commits into
Open
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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->addr2ontosanitize_sqe_addr_and_add2(). Two were missed, and a third gap of the same shape exists inregister.c.IORING_OP_STATX.io_uring_prep_statx()passesstatxbufas the offset argument ofio_uring_prep_rw(), so it lands insqe->off, which is the same union member asaddr2. The buffer the kernel writes the statx result into was never checked.IORING_OP_TIMEOUT_REMOVE.io_uring_prep_timeout_update()puts itsstruct __kernel_timespecpointer insqe->addr2, and the opcode was onsanitize_sqe_nop(). This one needssanitize_sqe_addr2()rather thansanitize_sqe_addr_and_add2(), because the opcode is shared withio_uring_prep_timeout_remove()and both helpers put auser_datavalue insqe->addr, not a pointer.io_uring_register_buffers_tags()/io_uring_register_buffers_update_tag(). Both check the iovec array and never look attags. Their files counterparts check both.do_register()'s ownliburing_sanitize_address(arg)does not cover it, sinceargis the address of the on-stackio_uring_rsrc_update2.On the unconditional
addr2check in (2):io_uring_prep_timeout_remove()passes a literal 0 as the offset andio_uring_prep_rw()always writessqe->off, soaddr2is deterministically zero on that path rather than stale.__asan_address_is_poisoned(NULL)returns 0, so it stays a no-op there. This matches howIORING_OP_ACCEPTandIORING_OP_SEND_ZCare already handled, whereaddrlenand 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 onsanitize_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:
Builds clean on x86_64 both with
--enable-sanitizerand with the default configure. No new tests, matching835f697.