Re: [PATCH] io_uring/net: don't truncate the bundle segment length to int

Baul Lee <[email protected]> writes:

> io_bundle_nbufs() works out how many buffers a short bundle transfer
> consumed by walking the mapped iovec and subtracting each segment from the
> transferred length:
>
> 	do {
> 		int this_len = min_t(int, iov[nbufs].iov_len, ret);
>
> 		nbufs++;
> 		ret -= this_len;
> 	} while (ret);
>
> iov_len is a size_t while this_len is an int, so a segment longer than
> INT_MAX comes out negative.  ret then grows instead of shrinking and the
> loop keeps walking, reading past the end of the array it was handed.
>
> The recv bundle path can hand it such a segment.  io_recv_buf_select()
> folds sr->mshot_total_len into arg.max_len, and that field is unsigned and
> taken straight from sqe->optlen:
>
> 	if (sr->flags & IORING_RECV_MSHOT_LIM)
> 		arg.max_len = min_not_zero(arg.max_len, sr->mshot_total_len);
>
> With sqe->len left at 0 nothing else lowers it, so arg.max_len can be
> 0xffffffff.  io_ring_buffers_peek() clamps each buffer against that rather
> than against INT_MAX and records an iov_len above INT_MAX; a short receive
> over that bundle then enters the loop above.
>
> An unprivileged multishot IORING_OP_RECV with IOSQE_BUFFER_SELECT and
> IORING_RECVSEND_BUNDLE, a provided buffer ring registered with
> IOU_PBUF_RING_INC holding two buffers, and two bytes of data is enough to
> reach it.  The two-entry iovec is a 32-byte kmalloc:
>
>   BUG: KASAN: slab-out-of-bounds in io_bundle_nbufs+0x114/0x1a8
>   Read of size 8 at addr ffff0000d39a1b68 by task io_repro/165
>   Call trace:
>    io_bundle_nbufs+0x114/0x1a8
>    io_recv_finish+0x138/0xa38
>    io_recv+0x38c/0x1008
>    io_issue_sqe+0xc4/0x155c
>    io_submit_sqes+0x6d0/0x1fac
>    __arm64_sys_io_uring_enter+0x30c/0x1180
>
>   Allocated by task 165:
>    __kmalloc_noprof+0x1b8/0x444
>    io_ring_buffers_peek+0x57c/0xbb4
>    io_buffers_peek+0x168/0x2a0
>    io_recv+0x598/0x1008
>
>   The buggy address is located 8 bytes to the right of
>   allocated 32-byte region [ffff0000d39a1b40, ffff0000d39a1b60)
>
> Count in size_t.  ret is positive by the time the loop runs, since the
> function returns early for ret <= 0, so widening loses nothing; this_len
> stays bounded by ret, and the mapped length is never less than ret, so the
> walk ends inside the array.
>
> Fixing it here rather than by capping arg.max_len keeps the accounting
> correct for every caller and does not disturb buffer selection: an
> arg.max_len of 0 is a distinct "not set yet" state that
> io_ring_buffers_peek() tests before it applies its own INT_MAX default,
> and pre-filling it changes which bundles get expanded.
>
> Discovered by XBOW, triaged by Baul Lee <[email protected]>
>
> Fixes: a05d1f625c7a ("io_uring/net: support bundles for send")
> Cc: [email protected]
> Signed-off-by: Baul Lee <[email protected]>

Reviewed-by: Gabriel Krisman Bertazi <[email protected]>

-- 
Gabriel Krisman Bertazi