diff mbox series

fs: Use CHECK_DATA_CORRUPTION() when kernel bugs are detected

Message ID 20230116191425.458864-1-jannh@google.com (mailing list archive)
State New, archived
Headers show
Series fs: Use CHECK_DATA_CORRUPTION() when kernel bugs are detected | expand

Commit Message

Jann Horn Jan. 16, 2023, 7:14 p.m. UTC 
Currently, filp_close() and generic_shutdown_super() use printk() to log
messages when bugs are detected. This is problematic because infrastructure
like syzkaller has no idea that this message indicates a bug.
In addition, some people explicitly want their kernels to BUG() when kernel
data corruption has been detected (CONFIG_BUG_ON_DATA_CORRUPTION).
And finally, when generic_shutdown_super() detects remaining inodes on a
system without CONFIG_BUG_ON_DATA_CORRUPTION, it would be nice if later
accesses to a busy inode would at least crash somewhat cleanly rather than
walking through freed memory.

To address all three, use CHECK_DATA_CORRUPTION() when kernel bugs are
detected.

Signed-off-by: Jann Horn <jannh@google.com>
---
 fs/open.c              |  5 +++--
 fs/super.c             | 21 +++++++++++++++++----
 include/linux/poison.h |  3 +++
 3 files changed, 23 insertions(+), 6 deletions(-)


base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4

Comments

Christian Brauner Jan. 24, 2023, 10:12 a.m. UTC | #1 
On Mon, Jan 16, 2023 at 08:14:25PM +0100, Jann Horn wrote:
> Currently, filp_close() and generic_shutdown_super() use printk() to log
> messages when bugs are detected. This is problematic because infrastructure
> like syzkaller has no idea that this message indicates a bug.
> In addition, some people explicitly want their kernels to BUG() when kernel
> data corruption has been detected (CONFIG_BUG_ON_DATA_CORRUPTION).
> And finally, when generic_shutdown_super() detects remaining inodes on a
> system without CONFIG_BUG_ON_DATA_CORRUPTION, it would be nice if later
> accesses to a busy inode would at least crash somewhat cleanly rather than
> walking through freed memory.
> 
> To address all three, use CHECK_DATA_CORRUPTION() when kernel bugs are
> detected.
> 
> Signed-off-by: Jann Horn <jannh@google.com>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Kees Cook Jan. 26, 2023, 4:35 p.m. UTC | #2 
On Mon, Jan 16, 2023 at 08:14:25PM +0100, Jann Horn wrote:
> Currently, filp_close() and generic_shutdown_super() use printk() to log
> messages when bugs are detected. This is problematic because infrastructure
> like syzkaller has no idea that this message indicates a bug.
> In addition, some people explicitly want their kernels to BUG() when kernel
> data corruption has been detected (CONFIG_BUG_ON_DATA_CORRUPTION).
> And finally, when generic_shutdown_super() detects remaining inodes on a
> system without CONFIG_BUG_ON_DATA_CORRUPTION, it would be nice if later
> accesses to a busy inode would at least crash somewhat cleanly rather than
> walking through freed memory.
> 
> To address all three, use CHECK_DATA_CORRUPTION() when kernel bugs are
> detected.

Seems reasonable to me. I'll carry this unless someone else speaks up.
:)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> 
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  fs/open.c              |  5 +++--
>  fs/super.c             | 21 +++++++++++++++++----
>  include/linux/poison.h |  3 +++
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 82c1a28b3308..ceb88ac0ca3b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1411,8 +1411,9 @@ int filp_close(struct file *filp, fl_owner_t id)
>  {
>  	int retval = 0;
>  
> -	if (!file_count(filp)) {
> -		printk(KERN_ERR "VFS: Close: file count is 0\n");
> +	if (CHECK_DATA_CORRUPTION(file_count(filp) == 0,
> +			"VFS: Close: file count is 0 (f_op=%ps)",
> +			filp->f_op)) {
>  		return 0;
>  	}
>  
> diff --git a/fs/super.c b/fs/super.c
> index 12c08cb20405..cf737ec2bd05 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -491,10 +491,23 @@ void generic_shutdown_super(struct super_block *sb)
>  		if (sop->put_super)
>  			sop->put_super(sb);
>  
> -		if (!list_empty(&sb->s_inodes)) {
> -			printk("VFS: Busy inodes after unmount of %s. "
> -			   "Self-destruct in 5 seconds.  Have a nice day...\n",
> -			   sb->s_id);
> +		if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes),
> +				"VFS: Busy inodes after unmount of %s (%s)",
> +				sb->s_id, sb->s_type->name)) {
> +			/*
> +			 * Adding a proper bailout path here would be hard, but
> +			 * we can at least make it more likely that a later
> +			 * iput_final() or such crashes cleanly.
> +			 */
> +			struct inode *inode;
> +
> +			spin_lock(&sb->s_inode_list_lock);
> +			list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> +				inode->i_op = VFS_PTR_POISON;
> +				inode->i_sb = VFS_PTR_POISON;
> +				inode->i_mapping = VFS_PTR_POISON;
> +			}
> +			spin_unlock(&sb->s_inode_list_lock);
>  		}
>  	}
>  	spin_lock(&sb_lock);
> diff --git a/include/linux/poison.h b/include/linux/poison.h
> index 2d3249eb0e62..0e8a1f2ceb2f 100644
> --- a/include/linux/poison.h
> +++ b/include/linux/poison.h
> @@ -84,4 +84,7 @@
>  /********** kernel/bpf/ **********/
>  #define BPF_PTR_POISON ((void *)(0xeB9FUL + POISON_POINTER_DELTA))
>  
> +/********** VFS **********/
> +#define VFS_PTR_POISON ((void *)(0xF5 + POISON_POINTER_DELTA))
> +
>  #endif
> 
> base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4
> -- 
> 2.39.0.314.g84b9a713c41-goog
>
Christian Brauner Jan. 27, 2023, 10:58 a.m. UTC | #3 
On Thu, Jan 26, 2023 at 08:35:49AM -0800, Kees Cook wrote:
> On Mon, Jan 16, 2023 at 08:14:25PM +0100, Jann Horn wrote:
> > Currently, filp_close() and generic_shutdown_super() use printk() to log
> > messages when bugs are detected. This is problematic because infrastructure
> > like syzkaller has no idea that this message indicates a bug.
> > In addition, some people explicitly want their kernels to BUG() when kernel
> > data corruption has been detected (CONFIG_BUG_ON_DATA_CORRUPTION).
> > And finally, when generic_shutdown_super() detects remaining inodes on a
> > system without CONFIG_BUG_ON_DATA_CORRUPTION, it would be nice if later
> > accesses to a busy inode would at least crash somewhat cleanly rather than
> > walking through freed memory.
> > 
> > To address all three, use CHECK_DATA_CORRUPTION() when kernel bugs are
> > detected.
> 
> Seems reasonable to me. I'll carry this unless someone else speaks up.

I've already picked this into a branch with other fs changes for coming cycle.

Al, please tell me in case you end up picking this up and I'll drop it ofc.

Christian
Kees Cook Jan. 27, 2023, 6:38 p.m. UTC | #4 
On Fri, Jan 27, 2023 at 11:58:15AM +0100, Christian Brauner wrote:
> On Thu, Jan 26, 2023 at 08:35:49AM -0800, Kees Cook wrote:
> > On Mon, Jan 16, 2023 at 08:14:25PM +0100, Jann Horn wrote:
> > > Currently, filp_close() and generic_shutdown_super() use printk() to log
> > > messages when bugs are detected. This is problematic because infrastructure
> > > like syzkaller has no idea that this message indicates a bug.
> > > In addition, some people explicitly want their kernels to BUG() when kernel
> > > data corruption has been detected (CONFIG_BUG_ON_DATA_CORRUPTION).
> > > And finally, when generic_shutdown_super() detects remaining inodes on a
> > > system without CONFIG_BUG_ON_DATA_CORRUPTION, it would be nice if later
> > > accesses to a busy inode would at least crash somewhat cleanly rather than
> > > walking through freed memory.
> > > 
> > > To address all three, use CHECK_DATA_CORRUPTION() when kernel bugs are
> > > detected.
> > 
> > Seems reasonable to me. I'll carry this unless someone else speaks up.
> 
> I've already picked this into a branch with other fs changes for coming cycle.

Okay, great! I'll drop it from my tree.

Reviewed-by: Kees Cook <keescook@chromium.org>
diff mbox series

Patch

diff --git a/fs/open.c b/fs/open.c
index 82c1a28b3308..ceb88ac0ca3b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1411,8 +1411,9 @@  int filp_close(struct file *filp, fl_owner_t id)
 {
 	int retval = 0;
 
-	if (!file_count(filp)) {
-		printk(KERN_ERR "VFS: Close: file count is 0\n");
+	if (CHECK_DATA_CORRUPTION(file_count(filp) == 0,
+			"VFS: Close: file count is 0 (f_op=%ps)",
+			filp->f_op)) {
 		return 0;
 	}
 
diff --git a/fs/super.c b/fs/super.c
index 12c08cb20405..cf737ec2bd05 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -491,10 +491,23 @@  void generic_shutdown_super(struct super_block *sb)
 		if (sop->put_super)
 			sop->put_super(sb);
 
-		if (!list_empty(&sb->s_inodes)) {
-			printk("VFS: Busy inodes after unmount of %s. "
-			   "Self-destruct in 5 seconds.  Have a nice day...\n",
-			   sb->s_id);
+		if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes),
+				"VFS: Busy inodes after unmount of %s (%s)",
+				sb->s_id, sb->s_type->name)) {
+			/*
+			 * Adding a proper bailout path here would be hard, but
+			 * we can at least make it more likely that a later
+			 * iput_final() or such crashes cleanly.
+			 */
+			struct inode *inode;
+
+			spin_lock(&sb->s_inode_list_lock);
+			list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+				inode->i_op = VFS_PTR_POISON;
+				inode->i_sb = VFS_PTR_POISON;
+				inode->i_mapping = VFS_PTR_POISON;
+			}
+			spin_unlock(&sb->s_inode_list_lock);
 		}
 	}
 	spin_lock(&sb_lock);
diff --git a/include/linux/poison.h b/include/linux/poison.h
index 2d3249eb0e62..0e8a1f2ceb2f 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -84,4 +84,7 @@ 
 /********** kernel/bpf/ **********/
 #define BPF_PTR_POISON ((void *)(0xeB9FUL + POISON_POINTER_DELTA))
 
+/********** VFS **********/
+#define VFS_PTR_POISON ((void *)(0xF5 + POISON_POINTER_DELTA))
+
 #endif