securityfs_create_file may fail. The fix checks its status and
returns EFAULT upstream if it fails.
Signed-off-by: Kangjie Lu <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
<[email protected]>
securityfs_create_file may fail. The fix checks its status and
returns the error code upstream if it fails.
Signed-off-by: Kangjie Lu <[email protected]>
---
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
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 <[email protected]>
>
> ---
> 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;
> }
>
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 <[email protected]>
>
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
<[email protected]>
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 <[email protected]>
> >
>
> 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...
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
<[email protected]>