netdev - Re: [syzbot] [fs?] general protection fault in splice_to

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]

Date: Wed, 14 Jun 2023 10:00:30 +0100
From: David Howells <dhowells@...hat.com>
To: syzbot <syzbot+f9e28a23426ac3b24f20@...kaller.appspotmail.com>
Cc: dhowells@...hat.com, brauner@...nel.org, kuba@...nel.org,
    linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
    netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
    viro@...iv.linux.org.uk
Subject: Re: [syzbot] [fs?] general protection fault in splice_to_socket

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git main

commit d302bc9baf84c549891bedee57ee917d9e0485d7
Author: David Howells <dhowells@...hat.com>
Date:   Wed Jun 14 09:14:50 2023 +0100

    splice: Fix splice_to_socket() to handle pipe bufs larger than a page
    
    splice_to_socket() assumes that a pipe_buffer won't hold more than a single
    page of data - but it seems that this assumption can be violated when
    splicing from a socket into a pipe.
    
    The problem is that splice_to_socket() doesn't advance the pipe_buffer
    length and offset when transcribing from the pipe buf into a bio_vec, so if
    the buf is >PAGE_SIZE, it keeps repeating the same initial chunk and
    doesn't advance the tail index.  It then subtracts this from "remain" and
    overcounts the amount of data to be sent.
    
    The cleanup phase then tries to overclean the pipe, hits an unused pipe buf
    and a NULL-pointer dereference occurs.
    
    Fix this by not restricting the bio_vec size to PAGE_SIZE and instead
    transcribing the entirety of each pipe_buffer into a single bio_vec and
    advancing the tail index if remain hasn't hit zero yet.
    
    Large bio_vecs will then be split up iterator functions such as
    iov_iter_extract_pages().
    
    This resulted in a KASAN report looking like:
    
    general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] PREEMPT SMP KASAN
    KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
    ...
    RIP: 0010:pipe_buf_release include/linux/pipe_fs_i.h:203 [inline]
    RIP: 0010:splice_to_socket+0xa91/0xe30 fs/splice.c:933
    
    Reported-by: syzbot+f9e28a23426ac3b24f20@...kaller.appspotmail.com
    Fixes: 2dc334f1a63a ("splice, net: Use sendmsg(MSG_SPLICE_PAGES) rather than ->sendpage()")
    
    Signed-off-by: David Howells <dhowells@...hat.com>
    cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
    cc: David Ahern <dsahern@...nel.org>
    cc: "David S. Miller" <davem@...emloft.net>
    cc: Eric Dumazet <edumazet@...gle.com>
    cc: Jakub Kicinski <kuba@...nel.org>
    cc: Paolo Abeni <pabeni@...hat.com>
    cc: Jens Axboe <axboe@...nel.dk>
    cc: Matthew Wilcox <willy@...radead.org>
    cc: Christian Brauner <brauner@...nel.org>
    cc: Alexander Viro <viro@...iv.linux.org.uk>
    cc: netdev@...r.kernel.org
    cc: linux-fsdevel@...r.kernel.org

diff --git a/fs/splice.c b/fs/splice.c
index e337630aed64..567a1f03ea1e 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -886,7 +886,6 @@ ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
 			}
 
 			seg = min_t(size_t, remain, buf->len);
-			seg = min_t(size_t, seg, PAGE_SIZE);
 
 			ret = pipe_buf_confirm(pipe, buf);
 			if (unlikely(ret)) {
@@ -897,10 +896,9 @@ ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
 
 			bvec_set_page(&bvec[bc++], buf->page, seg, buf->offset);
 			remain -= seg;
-			if (seg >= buf->len)
-				tail++;
-			if (bc >= ARRAY_SIZE(bvec))
+			if (remain == 0 || bc >= ARRAY_SIZE(bvec))
 				break;
+			tail++;
 		}
 
 		if (!bc)