* [PATCH]Fix kernel NULL function pointer dereference in vfs_kern_mount()
@ 2013-07-23 8:12 Dennis Chen
2013-07-26 9:12 ` Dennis Chen
0 siblings, 1 reply; 2+ messages in thread
From: Dennis Chen @ 2013-07-23 8:12 UTC (permalink / raw)
To: linux-kernel; +Cc: viro, 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.
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);
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH]Fix kernel NULL function pointer dereference in vfs_kern_mount()
2013-07-23 8:12 [PATCH]Fix kernel NULL function pointer dereference in vfs_kern_mount() Dennis Chen
@ 2013-07-26 9:12 ` Dennis Chen
0 siblings, 0 replies; 2+ messages in thread
From: Dennis Chen @ 2013-07-26 9:12 UTC (permalink / raw)
To: Dennis Chen; +Cc: linux-kernel, viro
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-07-26 9:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-23 8:12 [PATCH]Fix kernel NULL function pointer dereference in vfs_kern_mount() Dennis Chen
2013-07-26 9:12 ` Dennis Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).