* [PATCH] security: inode: fix a missing check for securityfs_create_file
@ 2019-03-15 4:09 Kangjie Lu
2019-03-15 19:08 ` James Morris
0 siblings, 1 reply; 7+ messages in thread
From: Kangjie Lu @ 2019-03-15 4:09 UTC (permalink / raw)
To: kjlu
Cc: pakki001, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
2019-03-15 4:09 [PATCH] security: inode: fix a missing check for securityfs_create_file Kangjie Lu
@ 2019-03-15 19:08 ` James Morris
2019-03-15 21:00 ` Kangjie Lu
0 siblings, 1 reply; 7+ messages in thread
From: James Morris @ 2019-03-15 19:08 UTC (permalink / raw)
To: Kangjie Lu
Cc: pakki001, Serge E. Hallyn, linux-security-module, linux-kernel,
Casey Schaufler
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] security: inode: fix a missing check for securityfs_create_file
2019-03-15 19:08 ` James Morris
@ 2019-03-15 21:00 ` Kangjie Lu
2019-03-15 22:34 ` Tetsuo Handa
2019-04-10 17:34 ` James Morris
0 siblings, 2 replies; 7+ messages in thread
From: Kangjie Lu @ 2019-03-15 21:00 UTC (permalink / raw)
To: kjlu
Cc: pakki001, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
2019-03-15 21:00 ` Kangjie Lu
@ 2019-03-15 22:34 ` Tetsuo Handa
2019-04-10 17:34 ` James Morris
1 sibling, 0 replies; 7+ messages in thread
From: Tetsuo Handa @ 2019-03-15 22:34 UTC (permalink / raw)
To: Kangjie Lu
Cc: pakki001, James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
2019-03-15 21:00 ` Kangjie Lu
2019-03-15 22:34 ` Tetsuo Handa
@ 2019-04-10 17:34 ` James Morris
2019-04-10 18:01 ` Al Viro
1 sibling, 1 reply; 7+ messages in thread
From: James Morris @ 2019-04-10 17:34 UTC (permalink / raw)
To: Kangjie Lu; +Cc: pakki001, Serge E. Hallyn, linux-security-module, linux-kernel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
2019-04-10 17:34 ` James Morris
@ 2019-04-10 18:01 ` Al Viro
2019-04-10 22:00 ` James Morris
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2019-04-10 18:01 UTC (permalink / raw)
To: James Morris
Cc: Kangjie Lu, pakki001, Serge E. Hallyn, linux-security-module,
linux-kernel
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...
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
2019-04-10 18:01 ` Al Viro
@ 2019-04-10 22:00 ` James Morris
0 siblings, 0 replies; 7+ messages in thread
From: James Morris @ 2019-04-10 22:00 UTC (permalink / raw)
To: Al Viro
Cc: Kangjie Lu, pakki001, Serge E. Hallyn, linux-security-module,
linux-kernel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-10 22:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-15 4:09 [PATCH] security: inode: fix a missing check for securityfs_create_file Kangjie Lu
2019-03-15 19:08 ` James Morris
2019-03-15 21:00 ` Kangjie Lu
2019-03-15 22:34 ` Tetsuo Handa
2019-04-10 17:34 ` James Morris
2019-04-10 18:01 ` Al Viro
2019-04-10 22:00 ` James Morris
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).