linux-kernel - Re: [PATCH 2/2] io_uring/kbuf: fix infinite loop in io_kbuf_inc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fcfd5324-9918-4613-94b0-c27fb8398375@kernel.dk>
Date: Wed, 27 Aug 2025 08:30:52 -0600
From: Jens Axboe <axboe@...nel.dk>
To: Qingyue Zhang <chunzhennn@...com>
Cc: io-uring@...r.kernel.org, linux-kernel@...r.kernel.org,
 Suoxing Zhang <aftern00n@...com>
Subject: Re: [PATCH 2/2] io_uring/kbuf: fix infinite loop in
 io_kbuf_inc_commit()

On 8/27/25 5:44 AM, Qingyue Zhang wrote:
> In io_kbuf_inc_commit(), buf points to a user-mapped memory region,
> which means buf->len might be changed between importing and committing.
> Add a check to avoid infinite loop when sum of buf->len is less than
> len.
> 
> Co-developed-by: Suoxing Zhang <aftern00n@...com>
> Signed-off-by: Suoxing Zhang <aftern00n@...com>
> Signed-off-by: Qingyue Zhang <chunzhennn@...com>
> ---
>  io_uring/kbuf.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> index 81a13338dfab..80ffe6755598 100644
> --- a/io_uring/kbuf.c
> +++ b/io_uring/kbuf.c
> @@ -34,11 +34,12 @@ struct io_provide_buf {
>  
>  static bool io_kbuf_inc_commit(struct io_buffer_list *bl, int len)
>  {
> +	struct io_uring_buf *buf, *buf_start;
> +
> +	buf_start = buf = io_ring_head_to_buf(bl->buf_ring, bl->head, bl->mask);
>  	while (len) {
> -		struct io_uring_buf *buf;
>  		u32 this_len;
>  
> -		buf = io_ring_head_to_buf(bl->buf_ring, bl->head, bl->mask);
>  		this_len = min_t(u32, len, buf->len);
>  		buf->len -= this_len;
>  		if (buf->len) {
> @@ -47,6 +48,10 @@ static bool io_kbuf_inc_commit(struct io_buffer_list *bl, int len)
>  		}
>  		bl->head++;
>  		len -= this_len;
> +
> +		buf = io_ring_head_to_buf(bl->buf_ring, bl->head, bl->mask);
> +		if (unlikely(buf == buf_start))
> +			break;
>  	}
>  	return true;
>  }

Maybe I'm dense, but I don't follow this one. 'len' is passed in, and
the only thing that should cause things to loop more than it should
would be if we do:

len -= this_len;

and this_len > len;

Yes, buf->len is user mapped, perhaps we just need to do:

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index f2d2cc319faa..569f4d957051 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -36,15 +36,18 @@ static bool io_kbuf_inc_commit(struct io_buffer_list *bl, int len)
 {
 	while (len) {
 		struct io_uring_buf *buf;
-		u32 this_len;
+		u32 buf_len, this_len;
 
 		buf = io_ring_head_to_buf(bl->buf_ring, bl->head, bl->mask);
-		this_len = min_t(int, len, buf->len);
-		buf->len -= this_len;
-		if (buf->len) {
+		buf_len = READ_ONCE(buf->len);
+		this_len = min_t(int, len, buf_len);
+		buf_len -= this_len;
+		if (buf_len) {
 			buf->addr += this_len;
+			buf->len = buf_len;
 			return false;
 		}
+		buf->len = 0;
 		bl->head++;
 		len -= this_len;
 	}

so that we operate on a local variable, and just set buf->len
appropriate for each buffer.

?

-- 
Jens Axboe