[PATCH] security: inode: fix a missing check for securityfs_create_file

[PATCH] security: inode: fix a missing check for securityfs_create_file

[Kangjie Lu]

securityfs_create_file  may fail. The fix checks its status and
returns EFAULT upstream if it fails.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 security/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/inode.c b/security/inode.c
index b7772a9b315e..11d9a6bc2161 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -339,6 +339,11 @@ static int __init securityfs_init(void)
 #ifdef CONFIG_SECURITY
 	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
 						&lsm_ops);
+	if (IS_ERR(lsm_dentry)) {
+		unregister_filesystem(&fs_type);
+		sysfs_remove_mount_point(kernel_kobj, "security");
+		return -EFAULT;
+	}
 #endif
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] security: inode: fix a missing check for securityfs_create_file

[James Morris]

On Thu, 14 Mar 2019, Kangjie Lu wrote:

> securityfs_create_file  may fail. The fix checks its status and
> returns EFAULT upstream if it fails.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  security/inode.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/security/inode.c b/security/inode.c
> index b7772a9b315e..11d9a6bc2161 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -339,6 +339,11 @@ static int __init securityfs_init(void)
>  #ifdef CONFIG_SECURITY
>  	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
>  						&lsm_ops);
> +	if (IS_ERR(lsm_dentry)) {
> +		unregister_filesystem(&fs_type);
> +		sysfs_remove_mount_point(kernel_kobj, "security");
> +		return -EFAULT;
> +	}
>  #endif
>  	return 0;
>  }
> 

Good catch, but you should propagate the error returned from 
securityfs_create_file().


-- 
James Morris
<jmorris@namei.org>

[PATCH] security: inode: fix a missing check for securityfs_create_file

[Kangjie Lu]

securityfs_create_file  may fail. The fix checks its status and
returns the error code upstream if it fails.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>

---
Return the exact error code upstream.
---
 security/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/inode.c b/security/inode.c
index b7772a9b315e..667f8b15027d 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -339,6 +339,11 @@ static int __init securityfs_init(void)
 #ifdef CONFIG_SECURITY
 	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
 						&lsm_ops);
+	if (IS_ERR(lsm_dentry)) {
+		unregister_filesystem(&fs_type);
+		sysfs_remove_mount_point(kernel_kobj, "security");
+		return PTR_ERR(lsm_dentry);
+	}
 #endif
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] security: inode: fix a missing check for securityfs_create_file

[Tetsuo Handa]

On 2019/03/16 6:00, Kangjie Lu wrote:
> securityfs_create_file  may fail. The fix checks its status and
> returns the error code upstream if it fails.

Failure in __init functions of vmlinux means that the system failed
before the global /sbin/init process starts. There is little value
with continuing the boot process. Calling panic() or BUG_ON() will
be OK, for the userspace will be get confused by lack of that file
even if we continued without securityfs entry in /proc/filesystems .

> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> 
> ---
> Return the exact error code upstream.
> ---
>  security/inode.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/security/inode.c b/security/inode.c
> index b7772a9b315e..667f8b15027d 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -339,6 +339,11 @@ static int __init securityfs_init(void)
>  #ifdef CONFIG_SECURITY
>  	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
>  						&lsm_ops);
> +	if (IS_ERR(lsm_dentry)) {
> +		unregister_filesystem(&fs_type);
> +		sysfs_remove_mount_point(kernel_kobj, "security");
> +		return PTR_ERR(lsm_dentry);
> +	}
>  #endif
>  	return 0;
>  }
> 

Re: [PATCH] security: inode: fix a missing check for securityfs_create_file

[James Morris]

On Fri, 15 Mar 2019, Kangjie Lu wrote:

> securityfs_create_file  may fail. The fix checks its status and
> returns the error code upstream if it fails.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> 

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general

> ---
> Return the exact error code upstream.
> ---
>  security/inode.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/security/inode.c b/security/inode.c
> index b7772a9b315e..667f8b15027d 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -339,6 +339,11 @@ static int __init securityfs_init(void)
>  #ifdef CONFIG_SECURITY
>  	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
>  						&lsm_ops);
> +	if (IS_ERR(lsm_dentry)) {
> +		unregister_filesystem(&fs_type);
> +		sysfs_remove_mount_point(kernel_kobj, "security");
> +		return PTR_ERR(lsm_dentry);
> +	}
>  #endif
>  	return 0;
>  }
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] security: inode: fix a missing check for securityfs_create_file

[Al Viro]

On Thu, Apr 11, 2019 at 03:34:43AM +1000, James Morris wrote:
> On Fri, 15 Mar 2019, Kangjie Lu wrote:
> 
> > securityfs_create_file  may fail. The fix checks its status and
> > returns the error code upstream if it fails.
> > 
> > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > 
> 
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
> 
> > ---
> > Return the exact error code upstream.
> > ---
> >  security/inode.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/security/inode.c b/security/inode.c
> > index b7772a9b315e..667f8b15027d 100644
> > --- a/security/inode.c
> > +++ b/security/inode.c
> > @@ -339,6 +339,11 @@ static int __init securityfs_init(void)
> >  #ifdef CONFIG_SECURITY
> >  	lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
> >  						&lsm_ops);
> > +	if (IS_ERR(lsm_dentry)) {
> > +		unregister_filesystem(&fs_type);
> > +		sysfs_remove_mount_point(kernel_kobj, "security");
> > +		return PTR_ERR(lsm_dentry);
> > +	}

Rather bad way to do it - generally, register_filesystem() should be
the last thing done by initialization.  Any modular code that
does unregister_filesystem() on failure exit is flat-out broken;
here it's not instantly FUBAR, but it's a bloody bad example.

What's more, why not let simple_fill_super() do it?  Just
static int fill_super(struct super_block *sb, void *data, int silent)
{
        static const struct tree_descr files[] = {
#ifdef CONFIG_SECURITY
		{"lsm", &lsm_ops, 0444},
#endif
		{""}
	};

and to hell with that call of securityfs_create_file() and all its
failure handling...

Re: [PATCH] security: inode: fix a missing check for securityfs_create_file

[James Morris]

On Wed, 10 Apr 2019, Al Viro wrote:

> Rather bad way to do it - generally, register_filesystem() should be
> the last thing done by initialization.  Any modular code that
> does unregister_filesystem() on failure exit is flat-out broken;
> here it's not instantly FUBAR, but it's a bloody bad example.
> 
> What's more, why not let simple_fill_super() do it?  Just
> static int fill_super(struct super_block *sb, void *data, int silent)
> {
>         static const struct tree_descr files[] = {
> #ifdef CONFIG_SECURITY
> 		{"lsm", &lsm_ops, 0444},
> #endif
> 		{""}
> 	};
> 
> and to hell with that call of securityfs_create_file() and all its
> failure handling...

Thanks for the review.  Reverted.

-- 
James Morris
<jmorris@namei.org>