Message ID | tencent_DABB2333139E8D1BCF4B5D1B2725FABA9108@qq.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-10706-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2483:b0:fb:cd0c:d3e with SMTP id q3csp2096128dyi; Sun, 24 Dec 2023 04:00:05 -0800 (PST) X-Google-Smtp-Source: AGHT+IFOoRQMpFNjz5gU2d3Af+NHay6P95e7JXNhi8OxF1wpSsSr9zUyOOGjsPxfrD7AFDpadrAQ X-Received: by 2002:a05:6e02:348a:b0:35f:b462:dd6a with SMTP id bp10-20020a056e02348a00b0035fb462dd6amr7665143ilb.21.1703419205477; Sun, 24 Dec 2023 04:00:05 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703419205; cv=none; d=google.com; s=arc-20160816; b=MeuJ2mgBMXIfDcbb+ZcpBTCe6Eg4Y9OQC/dQGaSZdnfO7/tml5CM1qm4KND+4BXZGj fU7QCCjNZK42+zwRu8zPTmaY9I0VUy+fVLwuz656lKjrUclNPfxffN7wDAn+p+MhSPTX 6z+rhdYDFuJjgjLTBHuoMplu9yMyRROwFUI+RH7SPVXcESiIc7sVmfzjyMKFPPt9G7Dm 8yfvJ1uyMNfQhzfGaa5Xr+1WrQA9JJ7Brlh7Vob6p1/mDirYuObG9T61h/9g/l35vbS7 9Tsi1T9AUaD4BABuhMxQMGJXzfiO+GSoVwm+ipcOJlmtFEEh7YZbvromU2w6L/GnifUR ZrTA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:date :subject:cc:to:from:message-id:dkim-signature; bh=bwVnJ7f9d4R+HLmdVsQRUR+32VmNaaFUOK0l/TVS0aI=; fh=l0x+ClQ1tT0yRbh0N/v7Fc7Lu+Yiqn/MWD4fPnPmG+I=; b=bhORr2mpb9hkyLe/3X0Q72l1YvTTveUGEZcAf2uJP6uKKBgvJ3c2AOjazzbUhJqnxv +e8Z/Grv+xYQ2pi4YRH6FR7BTafq6O+axveBlVCtSfetJ0j4KmbPUobZRciNqoWCH/Jv Cwt976T4Xuz7EvorkUQgjKAYIqysZ8SJdI7EOnfhS4vf41U9nqXPbfCJrpsaCUi1R/a8 B6isIU/LeYQf/1y7zcxTFLx5UVdktvFBIlwr21uj6n83cpdUn5blVDX6phwsV7emFEPO O2ZadzBrnh2KWQ1+/7lor75aFdtIrgqGZZ5ZTigC5BZxnL9VVBUEPk2RxZ7ZewvvOIJz VcKA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@qq.com header.s=s201512 header.b=eUqsMIbq; spf=pass (google.com: domain of linux-kernel+bounces-10706-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-10706-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=qq.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [2604:1380:40f1:3f00::1]) by mx.google.com with ESMTPS id e9-20020a170902b78900b001d3e4207386si6107988pls.317.2023.12.24.04.00.05 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 24 Dec 2023 04:00:05 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-10706-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@qq.com header.s=s201512 header.b=eUqsMIbq; spf=pass (google.com: domain of linux-kernel+bounces-10706-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-10706-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=qq.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 88661B21849 for <ouuuleilei@gmail.com>; Sun, 24 Dec 2023 11:59:39 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 74782566B; Sun, 24 Dec 2023 11:59:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=qq.com header.i=@qq.com header.b="eUqsMIbq" X-Original-To: linux-kernel@vger.kernel.org Received: from out203-205-221-205.mail.qq.com (out203-205-221-205.mail.qq.com [203.205.221.205]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 017AB2101; Sun, 24 Dec 2023 11:59:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=qq.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=qq.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qq.com; s=s201512; t=1703419144; bh=bwVnJ7f9d4R+HLmdVsQRUR+32VmNaaFUOK0l/TVS0aI=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=eUqsMIbqrNUpihWBZuPqLp+yfPFfwqKQXlcYlCaCpwhqTJJ8uMLiTK1H54530nwL8 RrPUPxb+7Q8nyYU8PTX4Ny32DvNSoSqKc6gjwUIryBzYaD0BKBBHnQeWSqx/Xwhq+q uHKmM9y3jZ633JOpa81HnhUupX6kfXOG/j8AFNtM= Received: from pek-lxu-l1.wrs.com ([111.198.225.215]) by newxmesmtplogicsvrszb6-0.qq.com (NewEsmtp) with SMTP id D3B898C9; Sun, 24 Dec 2023 19:52:59 +0800 X-QQ-mid: xmsmtpt1703418779tale22qig Message-ID: <tencent_DABB2333139E8D1BCF4B5D1B2725FABA9108@qq.com> X-QQ-XMAILINFO: NV9lVvsB36OpktO1KCwTUj4smAG+MtweeaJdLuOyDS/Po3/TqA7ngDgHIjmPzH /eOFFFXBWyCNn5bC1+d+Ma2OAKtSQg3d7UgqoYZwpkUbtJc5FNKGwuUauV85azMolsjjQn96txSD sWRvXZNjM/mTQh+gPjgXt0RnJvNJ7mOCVfAXbzpPtQzflxe9fzyXBdwtaFMdBFt7tjLBjrOz7nZJ wgvyEL6RnoRaRtj4uUO+tAKmsrRHOp/gUUwWDH8UJ6phxhY4YPsPgiUMuNx8M8OzPlTNl1pHwfju KH6GPdeiOJWr33Cy73LJJj9eKsLqDw/EONIapusAgRb+5haKQy/bdR69M23UKYh3X3u02LW4vx2y eMz13UuAKQkwKaWevatCqBOs9EU1uJQhxZuGcJe82h11UH2t/YQFZmZ5mtiHPY7C2ciAYT7b4q6p pPcUym10L9S3WPIKhyPN+Ijz9nhTHQ8ZfPCN8d4GJRS1OIKXLGVT/30Pki9Fpkg0sJ/tDAYsUA0h I8vetm5mdpTh3TCazPjHKoO9mu8v9XnMWXCSdMibdcImciNZMNskdELV/geuJ1iFKA/qgNQ5EoQf H2aSJIIUxVt4v19GndQ+F1KeNmcV9XwfFl5ZyYEski62C1HTDbbcSb0wiMf5zKgYCAY+WIHjN0rG AP94jJ7BUOM86aAKFyIZxh6TizNE4rYM3tRoaiMhOYX5khRL4ZZlBXWvO7QJi3hQDYUmZaOjRDes jw7TnP3du919JMXseJGvDmaoi8E4IVJbgPXMII15mFOGokwenqjucHVVKQgQryXNl3QwbaFvInOh ZLBlj+FPk4YgOdlvk4YBkrxvHyF/LeSKMy0iNKOZyM64gDO/cii5UEd5G7o5fEaotddgkGOE50g+ y3Fj8boC/xKmKn+FRl5HJ7LIcCx7nGlFeDU7+v/YSdZAqIdVRVD4W475ECBjrQBP4IjfZVqSPCnw JUK7UL/lk= X-QQ-XMRINFO: Mp0Kj//9VHAxr69bL5MkOOs= From: Edward Adam Davis <eadavis@qq.com> To: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com Cc: adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, tytso@mit.edu Subject: [PATCH] ext4: fix WARNING in lock_two_nondirectories Date: Sun, 24 Dec 2023 19:53:00 +0800 X-OQ-MSGID: <20231224115259.3685280-2-eadavis@qq.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <000000000000e17185060c8caaad@google.com> References: <000000000000e17185060c8caaad@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786164496728215399 X-GMAIL-MSGID: 1786164496728215399 |
Series |
ext4: fix WARNING in lock_two_nondirectories
|
|
Commit Message
Edward Adam Davis
Dec. 24, 2023, 11:53 a.m. UTC
If inode is the ext4 boot loader inode, then when it is a directory, the inode
should also be set to bad inode.
Reported-and-tested-by: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
fs/ext4/inode.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
Comments
On Sun, Dec 24, 2023 at 07:53:00PM +0800, Edward Adam Davis wrote: > If inode is the ext4 boot loader inode, then when it is a directory, the inode > should also be set to bad inode. ... what? Are you saying that syzbot has randomly created a filesystem where the boot loader inode is a directory? If so, I'd suggest just rejecting the filesystem with EFSCORRUPT. > Reported-and-tested-by: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/ext4/inode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 61277f7f8722..b311f610f008 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > } else if (S_ISDIR(inode->i_mode)) { > - inode->i_op = &ext4_dir_inode_operations; > - inode->i_fop = &ext4_dir_operations; > + if (ino == EXT4_BOOT_LOADER_INO) > + make_bad_inode(inode); > + else { > + inode->i_op = &ext4_dir_inode_operations; > + inode->i_fop = &ext4_dir_operations; > + } > } else if (S_ISLNK(inode->i_mode)) { > /* VFS does not allow setting these so must be corruption */ > if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) { > -- > 2.43.0 > >
On 2023/12/24 19:53, Edward Adam Davis wrote: > If inode is the ext4 boot loader inode, then when it is a directory, the inode > should also be set to bad inode. > > Reported-and-tested-by: syzbot+2c4a3b922a860084cc7f@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/ext4/inode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 61277f7f8722..b311f610f008 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, > inode->i_fop = &ext4_file_operations; > ext4_set_aops(inode); > } else if (S_ISDIR(inode->i_mode)) { > - inode->i_op = &ext4_dir_inode_operations; > - inode->i_fop = &ext4_dir_operations; > + if (ino == EXT4_BOOT_LOADER_INO) > + make_bad_inode(inode); Marking the boot loader inode as a bad inode here is useless, EXT4_IGET_BAD allows us to get a bad boot loader inode. In my opinion, it doesn't make sense to call lock_two_nondirectories() here to determine if the inode is a regular file or not, since the logic for dealing with non-regular files comes after the locking, so calling lock_two_inodes() directly here will suffice. Merry Christmas! Baokun > + else { > + inode->i_op = &ext4_dir_inode_operations; > + inode->i_fop = &ext4_dir_operations; > + } > } else if (S_ISLNK(inode->i_mode)) { > /* VFS does not allow setting these so must be corruption */ > if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > In my opinion, it doesn't make sense to call lock_two_nondirectories() > here to determine if the inode is a regular file or not, since the logic > for dealing with non-regular files comes after the locking, so calling > lock_two_inodes() directly here will suffice. No. First of all, lock_two_inodes() is a mistake that is going to be removed in the coming cycle. What's more, why the hell do you need to lock *anything* to check the inode type? Inode type never changes, period. Just take that check prior to lock_two_nondirectories() and be done with that.
On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > Marking the boot loader inode as a bad inode here is useless, > EXT4_IGET_BAD allows us to get a bad boot loader inode. > In my opinion, it doesn't make sense to call lock_two_nondirectories() > here to determine if the inode is a regular file or not, since the logic > for dealing with non-regular files comes after the locking, so calling > lock_two_inodes() directly here will suffice. This is all very silly, and why I consider this sort of thing pure syzkaller noise. It really doesn't protect against any real threat, and it encourages people to put all sorts of random crud in kernel code, all in the name of trying to shut up syzbot. If we *are* going to care about shutting up syzkaller, the right approach is to simply add a check in swap_inode_boot_loader() which causes it to call ext4_error() and declare the file system corrupted if the bootloader inode is not a regular file, and then return -EFSCORRUPTED. We don't need to add random hacks to ext4_iget(), or in other places... - Ted
On Sun, Dec 24, 2023 at 09:11:36PM -0500, Theodore Ts'o wrote: > On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > > Marking the boot loader inode as a bad inode here is useless, > > EXT4_IGET_BAD allows us to get a bad boot loader inode. > > In my opinion, it doesn't make sense to call lock_two_nondirectories() > > here to determine if the inode is a regular file or not, since the logic > > for dealing with non-regular files comes after the locking, so calling > > lock_two_inodes() directly here will suffice. > > This is all very silly, and why I consider this sort of thing pure > syzkaller noise. It really doesn't protect against any real threat, > and it encourages people to put all sorts of random crud in kernel > code, all in the name of trying to shut up syzbot. > > If we *are* going to care about shutting up syzkaller, the right > approach is to simply add a check in swap_inode_boot_loader() which > causes it to call ext4_error() and declare the file system corrupted > if the bootloader inode is not a regular file, and then return > -EFSCORRUPTED. > > We don't need to add random hacks to ext4_iget(), or in other places... Just check the inode type before anything else and be done with that - if an in-core inode of a regular file manages to become a directory right under us, we have a much worse problem. IOW, the bug is real, but suggested patch is just plain wrong.
On 2023/12/25 10:07, Al Viro wrote: > On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: > >> In my opinion, it doesn't make sense to call lock_two_nondirectories() >> here to determine if the inode is a regular file or not, since the logic >> for dealing with non-regular files comes after the locking, so calling >> lock_two_inodes() directly here will suffice. > No. First of all, lock_two_inodes() is a mistake that is going to be > removed in the coming cycle. Okay, I didn't know about this. > What's more, why the hell do you need to lock *anything* to check the > inode type? Inode type never changes, period. > > Just take that check prior to lock_two_nondirectories() and be done with > that. Since in the current logic we update the boot loader file via swap_inode_boot_loader(), however the boot loader inode on disk may be uninitialized and may be garbage data, so we allow to get a bad boot loader inode and then initialize it and swap it with the boot loader file to be set. When reinitializing the bad boot loader inode, something like an inode type conversion may occur. Cheers, Baokun
On 2023/12/25 10:11, Theodore Ts'o wrote: > On Mon, Dec 25, 2023 at 09:38:51AM +0800, Baokun Li wrote: >> Marking the boot loader inode as a bad inode here is useless, >> EXT4_IGET_BAD allows us to get a bad boot loader inode. >> In my opinion, it doesn't make sense to call lock_two_nondirectories() >> here to determine if the inode is a regular file or not, since the logic >> for dealing with non-regular files comes after the locking, so calling >> lock_two_inodes() directly here will suffice. > This is all very silly, and why I consider this sort of thing pure > syzkaller noise. It really doesn't protect against any real threat, > and it encourages people to put all sorts of random crud in kernel > code, all in the name of trying to shut up syzbot. Indeed, the warning is meaningless, but it is undeniable that if the user can easily trigger the warning, something is wrong with the code. > If we *are* going to care about shutting up syzkaller, the right > approach is to simply add a check in swap_inode_boot_loader() which > causes it to call ext4_error() and declare the file system corrupted > if the bootloader inode is not a regular file, and then return > -EFSCORRUPTED. > > We don't need to add random hacks to ext4_iget(), or in other places... > > - Ted Without considering the case where the boot loader inode is uninitialized, I think this is fine and the logic to determine if the boot loader inode is initialized and to initialize it can be removed. Merry Christmas!
On Mon, Dec 25, 2023 at 10:33:20AM +0800, Baokun Li wrote: > Since in the current logic we update the boot loader file via > swap_inode_boot_loader(), however the boot loader inode on disk > may be uninitialized and may be garbage data, so we allow to get a > bad boot loader inode and then initialize it and swap it with the boot > loader file to be set. > When reinitializing the bad boot loader inode, something like an > inode type conversion may occur. Yes, but the boot laoder inode is *either* all zeros, or a regular file. If it's a directory, then it's a malicious syzbot trying to mess with our minds. Aside from the warning, it's pretty harmless, but it will very likely result in a corrupted file system --- but the file system was corrupted in the first place. So who cares? Just check to make sure that i_mode is either 0, or regular file, and return EFSCORRUPTEd, and we're done. - Ted
On 2023/12/25 10:49, Theodore Ts'o wrote: > On Mon, Dec 25, 2023 at 10:33:20AM +0800, Baokun Li wrote: >> Since in the current logic we update the boot loader file via >> swap_inode_boot_loader(), however the boot loader inode on disk >> may be uninitialized and may be garbage data, so we allow to get a >> bad boot loader inode and then initialize it and swap it with the boot >> loader file to be set. >> When reinitializing the bad boot loader inode, something like an >> inode type conversion may occur. > Yes, but the boot laoder inode is *either* all zeros, or a regular > file. If it's a directory, then it's a malicious syzbot trying to > mess with our minds. > > Aside from the warning, it's pretty harmless, but it will very likely > result in a corrupted file system --- but the file system was > corrupted in the first place. So who cares? > > Just check to make sure that i_mode is either 0, or regular file, and > return EFSCORRUPTEd, and we're done. > > - Ted Yes, this seems to work, but for that matter, when i_mode is 0, we still trigger the WARN_ON_ONCE in lock_two_nondirectories(). Merry Christmas!
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 61277f7f8722..b311f610f008 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4944,8 +4944,12 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, inode->i_fop = &ext4_file_operations; ext4_set_aops(inode); } else if (S_ISDIR(inode->i_mode)) { - inode->i_op = &ext4_dir_inode_operations; - inode->i_fop = &ext4_dir_operations; + if (ino == EXT4_BOOT_LOADER_INO) + make_bad_inode(inode); + else { + inode->i_op = &ext4_dir_inode_operations; + inode->i_fop = &ext4_dir_operations; + } } else if (S_ISLNK(inode->i_mode)) { /* VFS does not allow setting these so must be corruption */ if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {