Messages in this thread | | | Date | Fri, 26 Jul 2013 17:12:37 +0800 | From | Dennis Chen <> | Subject | Re: [PATCH]Fix kernel NULL function pointer dereference in vfs_kern_mount() |
| |
On 07/23/2013 04:12 PM, Dennis Chen wrote:
> 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. > Signed-off-by: Dennis Chen <xschen@tnsoft.com.cn> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > --- > fs/filesystems.c | 5 +++++ > fs/namespace.c | 2 ++ > fs/super.c | 10 ++++++++++ > 3 files changed, 17 insertions(+) > diff --git a/fs/filesystems.c b/fs/filesystems.c > index 92567d9..6d6b162 100644 > --- a/fs/filesystems.c > +++ b/fs/filesystems.c > @@ -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; > diff --git a/fs/namespace.c b/fs/namespace.c > index 7b1ca9b..ad17692 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -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)) { > /* > diff --git a/fs/super.c b/fs/super.c > index 68307c0..d995a19 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -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); > 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
| |