Re: [syzbot] KASAN: use-after-free Read in filp_close

Re: [syzbot] KASAN: use-after-free Read in filp_close

[syzbot]

syzbot has bisected this issue to:

commit 6319194ec57b0452dcda4589d24c4e7db299c5bf
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Thu May 12 21:08:03 2022 +0000

    Unify the primitives for file descriptor closing

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=134cbe4ff00000
start commit:   952923ddc011 Merge tag 'pull-18-rc1-work.namei' of git://g..
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=10ccbe4ff00000
console output: https://syzkaller.appspot.com/x/log.txt?x=174cbe4ff00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=3096247591885bfa
dashboard link: https://syzkaller.appspot.com/bug?extid=47dd250f527cb7bebf24
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=114f7bcdf00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1659a94ff00000

Reported-by: syzbot+47dd250f527cb7bebf24@syzkaller.appspotmail.com
Fixes: 6319194ec57b ("Unify the primitives for file descriptor closing")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Re: [syzbot] KASAN: use-after-free Read in filp_close

[Al Viro]

On Sun, Jun 05, 2022 at 07:04:10AM -0700, syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit 6319194ec57b0452dcda4589d24c4e7db299c5bf
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Thu May 12 21:08:03 2022 +0000
> 
>     Unify the primitives for file descriptor closing
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=134cbe4ff00000
> start commit:   952923ddc011 Merge tag 'pull-18-rc1-work.namei' of git://g..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=10ccbe4ff00000
> console output: https://syzkaller.appspot.com/x/log.txt?x=174cbe4ff00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=3096247591885bfa
> dashboard link: https://syzkaller.appspot.com/bug?extid=47dd250f527cb7bebf24
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=114f7bcdf00000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1659a94ff00000
> 
> Reported-by: syzbot+47dd250f527cb7bebf24@syzkaller.appspotmail.com
> Fixes: 6319194ec57b ("Unify the primitives for file descriptor closing")
> 
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Argh...  I see what's going on.  Check if the following fixes the problem,
please.

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 27c9b004823a..73beea5dc18c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1857,6 +1857,8 @@ static void binder_deferred_fd_close(int fd)
 	init_task_work(&twcb->twork, binder_do_fd_close);
 	twcb->file = close_fd_get_file(fd);
 	if (twcb->file) {
+		// pin it until binder_do_fd_close(); see comments there
+		get_file(twcb->file);
 		filp_close(twcb->file, current->files);
 		task_work_add(current, &twcb->twork, TWA_RESUME);
 	} else {
diff --git a/fs/file.c b/fs/file.c
index dd6692048f4f..3bcc1ecc314a 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -800,8 +800,7 @@ struct file *__close_fd_get_file(unsigned int fd)
 
 /*
  * variant of close_fd that gets a ref on the file for later fput.
- * The caller must ensure that filp_close() called on the file, and then
- * an fput().
+ * The caller must ensure that filp_close() called on the file.
  */
 struct file *close_fd_get_file(unsigned int fd)
 {
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7257b0870353..33da5116cc38 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5110,7 +5110,7 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 	struct files_struct *files = current->files;
 	struct io_close *close = &req->close;
 	struct fdtable *fdt;
-	struct file *file = NULL;
+	struct file *file;
 	int ret = -EBADF;
 
 	if (req->close.file_slot) {
@@ -5127,7 +5127,6 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 	file = fdt->fd[close->fd];
 	if (!file || file->f_op == &io_uring_fops) {
 		spin_unlock(&files->file_lock);
-		file = NULL;
 		goto err;
 	}
 
@@ -5147,8 +5146,6 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 err:
 	if (ret < 0)
 		req_set_fail(req);
-	if (file)
-		fput(file);
 	__io_req_complete(req, issue_flags, ret, 0);
 	return 0;
 }

Re: [syzbot] KASAN: use-after-free Read in filp_close

[Al Viro]

On Sun, Jun 05, 2022 at 04:15:58PM +0000, Al Viro wrote:
> Argh...  I see what's going on.  Check if the following fixes the problem,
> please.

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.fd

Re: [syzbot] KASAN: use-after-free Read in filp_close

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+47dd250f527cb7bebf24@syzkaller.appspotmail.com

Tested on:

commit:         6dda6985 fix the breakage in close_fd_get_file() calli..
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.fd
kernel config:  https://syzkaller.appspot.com/x/.config?x=d4042ecb71632a26
dashboard link: https://syzkaller.appspot.com/bug?extid=47dd250f527cb7bebf24
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2

Note: no patches were applied.
Note: testing is done by a robot and is best-effort only.