| Message ID | 20190315210025.17832-1-kjlu@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: inode: fix a missing check for securityfs_create_file | expand |
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; > } >
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; > } >
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...
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.
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; }
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(+)