Skip to contentSkip to conversation

Fix kernel NULL function pointer dereference in vfs_kern_mount()

PATCH In review Dennis Chen 1 participant 2 messages Jul 23, 2013 – Jul 26, 2013 VFS

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix kernel NULL function pointer dereference in vfs_kern_mount()

This patch is try to fix a potential NULL function pointer dereference in the exported vfs_kern_mount() function which can be triggered by following kernel module code piece:

static struct vfsmount *npfs_mnt;
static struct file_system_type np_fs_type = {
.name = "npfs",
.kill_sb = npfs_kill_sb,
.fs_flags = FS_USERNS_MOUNT,
};

static int fsmod_init(void)

{
	int err = -ENOMEM;

	err = register_filesystem(&np_fs_type);
	if (!err) {
		npfs_mnt = kern_mount(&np_fs_type);
		if (IS_ERR(npfs_mnt)) {
			printk(KERN_ERR "npfs: could not mount!\n");
			unregister_filesystem(&np_fs_type);
			return PTR_ERR(npfs_mnt);
		}
	}

	return err;
}

I happened to forget to implement a (*mount)() function in the np_fs_type instance which is buggy definitely, but as an exported function to the outside, kernel should not suppose the caller will always take the right action. In this scenario, I have to reboot my machine to clean the loaded kernel module.

Cc Alexander Viro <[email protected]>

---

fs/filesystems.c |    5 +++++
fs/namespace.c   |    2 ++
fs/super.c       |   10 ++++++++++
3 files changed, 17 insertions(+)
fs/filesystems.c +5 -0
index 92567d9..6d6b162 100644
@@ -71,6 +71,11 @@ int register_filesystem(struct file_system_type * fs)
        int res = 0;
        struct file_system_type ** p;
 
+       if (!fs)
+               return -ENODEV;
+       if (!fs->name)
+               return -EINVAL;
+
        BUG_ON(strchr(fs->name, '.'));
        if (fs->next)
                return -EBUSY;
fs/namespace.c +2 -0
index 7b1ca9b..ad17692 100644
@@ -2758,6 +2758,8 @@ void put_mnt_ns(struct mnt_namespace *ns)
 struct vfsmount *kern_mount_data(struct file_system_type *type, void *data)
 {
        struct vfsmount *mnt;
+       if (!type)
+               return ERR_PTR(-ENODEV);
        mnt = vfs_kern_mount(type, MS_KERNMOUNT, type->name, data);
        if (!IS_ERR(mnt)) {
                /*
fs/super.c +10 -0
index 68307c0..d995a19 100644
@@ -1101,11 +1101,21 @@ mount_fs(struct file_system_type *type, int flags, const char *name, void *data)
                        goto out_free_secdata;
        }
 
+       if (!type->mount) {
+               error = -EINVAL;
+               printk(KERN_ERR "No 'mount' method defined in %s\n", type->name);
+               goto out_free_secdata;
+       }
        root = type->mount(type, flags, name, data);
        if (IS_ERR(root)) {
                error = PTR_ERR(root);
                goto out_free_secdata;
        }
+       if (!root) {
+               error = -EINVAL;
+               goto out_free_secdata;
+       }
+
        sb = root->d_sb;
        BUG_ON(!sb);
        WARN_ON(!sb->s_bdi);
#1 lore raw
Quoted 79 lines · Dennis Chen

This patch is try to fix a potential NULL function pointer dereference in the exported vfs_kern_mount() function which can be triggered by following kernel module code piece:
static struct vfsmount *npfs_mnt;
static struct file_system_type np_fs_type = {
.name = "npfs",
.kill_sb = npfs_kill_sb,
.fs_flags = FS_USERNS_MOUNT,
};
static int fsmod_init(void)

{
     int err = -ENOMEM;
     err = register_filesystem(&np_fs_type);
     if (!err) {
         npfs_mnt = kern_mount(&np_fs_type);
         if (IS_ERR(npfs_mnt)) {
             printk(KERN_ERR "npfs: could not mount!\n");
             unregister_filesystem(&np_fs_type);
             return PTR_ERR(npfs_mnt);
         }
     }
     return err;
}

I happened to forget to implement a (*mount)() function in the np_fs_type instance which is buggy definitely, but as an exported function to the outside, kernel should not suppose the caller will always take the right action. In this scenario, I have to reboot my machine to clean the loaded kernel module.

Cc Alexander Viro <[email protected]>

---

fs/filesystems.c |    5 +++++
fs/namespace.c   |    2 ++
fs/super.c       |   10 ++++++++++
3 files changed, 17 insertions(+)
fs/filesystems.c +5 -0
index 92567d9..6d6b162 100644
@@ -71,6 +71,11 @@ int register_filesystem(struct file_system_type * fs)
         int res = 0;
         struct file_system_type ** p;
 
+       if (!fs)
+               return -ENODEV;
+       if (!fs->name)
+               return -EINVAL;
+
         BUG_ON(strchr(fs->name, '.'));
         if (fs->next)
                 return -EBUSY;
fs/namespace.c +2 -0
index 7b1ca9b..ad17692 100644
@@ -2758,6 +2758,8 @@ void put_mnt_ns(struct mnt_namespace *ns)
  struct vfsmount *kern_mount_data(struct file_system_type *type, void *data)
  {
         struct vfsmount *mnt;
+       if (!type)
+               return ERR_PTR(-ENODEV);
         mnt = vfs_kern_mount(type, MS_KERNMOUNT, type->name, data);
         if (!IS_ERR(mnt)) {
                 /*
fs/super.c +5 -0
index 68307c0..d995a19 100644
@@ -1101,11 +1101,21 @@ mount_fs(struct file_system_type *type, int flags, const char *name, void *data)
                         goto out_free_secdata;
         }
 
+       if (!type->mount) {
+               error = -EINVAL;
+               printk(KERN_ERR "No 'mount' method defined in %s\n", type->name);
+               goto out_free_secdata;
+       }
         root = type->mount(type, flags, name, data);
         if (IS_ERR(root)) {
                 error = PTR_ERR(root);

fs/super.c mount_fs(struct file_system_type *type, int flags, const char *name, void *data) · line 1121

Quoted 10 lines
fs/super.c +5 -0
                 goto out_free_secdata;
         }
+       if (!root) {
+               error = -EINVAL;
+               goto out_free_secdata;
+       }
+
         sb = root->d_sb;
         BUG_ON(!sb);
         WARN_ON(!sb->s_bdi);

Any feedback for this patch? If we don't need it meaning the kernel module developer should take the responsibility to make sure his/her codes will not crash something, but in my opinion, as exported kernel function, it should do its best to avoid put the kernel in a unstable state, while not transfer the responsibility to the user, right?

den