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 <[email protected]>
---
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
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 <[email protected]>
> ---
Looks good,
Reviewed-by: Christian Brauner (Microsoft) <[email protected]>
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 <[email protected]>
-Kees
>
> Signed-off-by: Jann Horn <[email protected]>
> ---
> 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
>
--
Kees Cook
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
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 <[email protected]>
--
Kees Cook