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 <[email protected]> Cc: Alexander Viro <[email protected]> --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
