* [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).