Re: [PATCH] io_uring/net: clear stale vec on buffer peek error after expansion

[Gabriel Krisman Bertazi]

Feng Xue <feng.xue@xxxxxxxxxxx> writes:

> Subject: [PATCH] io_uring/net: clear stale vec on buffer peek error after expansion
>
> When io_ring_buffers_peek() expands the iovec array during a bundle
> recv retry, it frees the old array (A) and allocates a new one (B).
> If access_ok() then fails, B is also freed and -EFAULT is returned.
>
> The callers io_recv_buf_select() and io_send_select_buffer() only
> update kmsg->vec.iovec on success, so on this error path vec.iovec
> still points to freed A. The stale pointer survives into the netmsg
> alloc cache via io_netmsg_recycle() (vec.nr < IO_VEC_CACHE_SOFT_CAP
> so io_vec_free is not called). A subsequent bundle operation reuses
> the cached hdr, sees vec.iovec non-NULL, sets REQ_F_NEED_CLEANUP,
> and passes the dangling pointer back to io_ring_buffers_peek() —
> which writes iovec entries to freed memory (use-after-free).
>
> If the alloc cache is full, the alternative cleanup path through
> io_clean_op() → io_vec_free() kfree()s the already-freed A
> (double-free).
>
> Fix this by NULLing vec.iovec and zeroing vec.nr on the error path
> when expansion occurred (detected by arg.iovs != kmsg->vec.iovec).
> Do not call io_vec_free() here — A is already freed by the expansion
> block, so kfree()ing it again would itself be a double-free.
>
> Apply the same fix to io_send_select_buffer() which has the identical
> update-after-success pattern.

cleaning in the caller makes the issue much more likely to happen again
in a future use of this function.  It would be better to fix the bad
semantics of io_ring_buffers_peek instead.

In fact, this is exactly the point of this patch, which I believe
already fixed this issue:

https://lore.kernel.org/io-uring/178338543579.49877.9882374687710864124.b4-ty@b4/T/#t

>
> Signed-off-by: Feng Xue <feng.xue@xxxxxxxxxxx>
> Assisted by: XGPT
> ---
>  io_uring/net.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/io_uring/net.c b/io_uring/net.c
> index XXXXXXX..XXXXXXX 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -631,8 +631,15 @@ static int io_send_select_buffer(struct io_kiocb *req, unsigned int issue_flags,
>  
>  	ret = io_buffers_select(req, &arg, sel, issue_flags);
> -	if (unlikely(ret < 0))
> +	if (unlikely(ret < 0)) {
> +		/*
> +		 * Buffer selection may have freed the old iovec during
> +		 * expansion. Clear vec to prevent stale-pointer reuse.
> +		 */
> +		if (kmsg->vec.iovec && arg.iovs != kmsg->vec.iovec) {
> +			kmsg->vec.iovec = NULL;
> +			kmsg->vec.nr = 0;
> +		}
>  		return ret;
> +	}
>  
>  	if (arg.iovs != &kmsg->fast_iov && arg.iovs != kmsg->vec.iovec) {
> @@ -1174,8 +1181,15 @@ static int io_recv_buf_select(struct io_kiocb *req,
>  
>  		ret = io_buffers_peek(req, &arg, sel);
> -		if (unlikely(ret < 0))
> +		if (unlikely(ret < 0)) {
> +			/*
> +			 * Peek may have freed the old iovec during expansion.
> +			 * Clear vec to prevent stale-pointer reuse or
> +			 * double-free via io_vec_free on the cleanup path.
> +			 */
> +			if (kmsg->vec.iovec && arg.iovs != kmsg->vec.iovec) {
> +				kmsg->vec.iovec = NULL;
> +				kmsg->vec.nr = 0;
> +			}
>  			return ret;
> +		}
>  
>  		if (arg.iovs != &kmsg->fast_iov && arg.iovs != kmsg->vec.iovec) {

-- 
Gabriel Krisman Bertazi