| Message ID | 20240328191658.210221-1-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: avoid dereference of garbage after mount failure | expand |
On 3/28/2024 12:16 PM, Christian Göttsche wrote: > From: Christian Göttsche <cgzones@googlemail.com> > > In case kern_mount() fails and returns an error pointer return in the > error branch instead of continuing and dereferencing the error pointer. > > While on it drop the never read static variable selinuxfs_mount. > > Fixes: 0619f0f5e36f ("selinux: wrap selinuxfs state") > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/selinuxfs.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index d18581d741e8..7e9aa5d151b4 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -2125,7 +2125,6 @@ static struct file_system_type sel_fs_type = { > .kill_sb = sel_kill_sb, > }; > > -static struct vfsmount *selinuxfs_mount __ro_after_init; > struct path selinux_null __ro_after_init; > > static int __init init_sel_fs(void) > @@ -2147,18 +2146,21 @@ static int __init init_sel_fs(void) > return err; > } > > - selinux_null.mnt = selinuxfs_mount = kern_mount(&sel_fs_type); > - if (IS_ERR(selinuxfs_mount)) { > + selinux_null.mnt = kern_mount(&sel_fs_type); > + if (IS_ERR(selinux_null.mnt)) { > pr_err("selinuxfs: could not mount!\n"); > - err = PTR_ERR(selinuxfs_mount); > - selinuxfs_mount = NULL; > + err = PTR_ERR(selinux_null.mnt); > + selinux_null.mnt = NULL; > + return err; > } > + > selinux_null.dentry = d_hash_and_lookup(selinux_null.mnt->mnt_root, > &null_name); > if (IS_ERR(selinux_null.dentry)) { > pr_err("selinuxfs: could not lookup null!\n"); > err = PTR_ERR(selinux_null.dentry); > selinux_null.dentry = NULL; > + return err; Surely this is unnecessary. > } > > return err;
On Mar 28, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote: > > In case kern_mount() fails and returns an error pointer return in the > error branch instead of continuing and dereferencing the error pointer. > > While on it drop the never read static variable selinuxfs_mount. > > Fixes: 0619f0f5e36f ("selinux: wrap selinuxfs state") > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/selinuxfs.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index d18581d741e8..7e9aa5d151b4 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -2125,7 +2125,6 @@ static struct file_system_type sel_fs_type = { > .kill_sb = sel_kill_sb, > }; > > -static struct vfsmount *selinuxfs_mount __ro_after_init; > struct path selinux_null __ro_after_init; > > static int __init init_sel_fs(void) > @@ -2147,18 +2146,21 @@ static int __init init_sel_fs(void) > return err; > } > > - selinux_null.mnt = selinuxfs_mount = kern_mount(&sel_fs_type); > - if (IS_ERR(selinuxfs_mount)) { > + selinux_null.mnt = kern_mount(&sel_fs_type); > + if (IS_ERR(selinux_null.mnt)) { > pr_err("selinuxfs: could not mount!\n"); > - err = PTR_ERR(selinuxfs_mount); > - selinuxfs_mount = NULL; > + err = PTR_ERR(selinux_null.mnt); > + selinux_null.mnt = NULL; > + return err; > } > + > selinux_null.dentry = d_hash_and_lookup(selinux_null.mnt->mnt_root, > &null_name); > if (IS_ERR(selinux_null.dentry)) { > pr_err("selinuxfs: could not lookup null!\n"); > err = PTR_ERR(selinux_null.dentry); > selinux_null.dentry = NULL; > + return err; Casey's correct in that we probably don't need this, but it does harden the source a bit against future changes so it isn't entirely a bad thing. If nothing else, some new kernel dev will get excited writing a oneliner several years from now that removes the redundant return; I'm getting exited about the patch just thinking about it. Anyway, thanks for noticing this and submitting a patch Christian, beyond the above nitpick, everything looks good to me. I'm going to merge this into selinux/stable-6.9 with a stable marking and assuming all goes well I'll send this up to Linus in a few days. Thanks! > } > > return err; > -- > 2.43.0 -- paul-moore.com
On Tue, 2 Apr 2024 at 04:05, Paul Moore <paul@paul-moore.com> wrote: > > On Mar 28, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote: > > > > In case kern_mount() fails and returns an error pointer return in the > > error branch instead of continuing and dereferencing the error pointer. > > > > While on it drop the never read static variable selinuxfs_mount. > > > > Fixes: 0619f0f5e36f ("selinux: wrap selinuxfs state") > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > security/selinux/selinuxfs.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > > index d18581d741e8..7e9aa5d151b4 100644 > > --- a/security/selinux/selinuxfs.c > > +++ b/security/selinux/selinuxfs.c > > @@ -2125,7 +2125,6 @@ static struct file_system_type sel_fs_type = { > > .kill_sb = sel_kill_sb, > > }; > > > > -static struct vfsmount *selinuxfs_mount __ro_after_init; > > struct path selinux_null __ro_after_init; > > > > static int __init init_sel_fs(void) > > @@ -2147,18 +2146,21 @@ static int __init init_sel_fs(void) > > return err; > > } > > > > - selinux_null.mnt = selinuxfs_mount = kern_mount(&sel_fs_type); > > - if (IS_ERR(selinuxfs_mount)) { > > + selinux_null.mnt = kern_mount(&sel_fs_type); > > + if (IS_ERR(selinux_null.mnt)) { > > pr_err("selinuxfs: could not mount!\n"); > > - err = PTR_ERR(selinuxfs_mount); > > - selinuxfs_mount = NULL; > > + err = PTR_ERR(selinux_null.mnt); > > + selinux_null.mnt = NULL; > > + return err; > > } > > + > > selinux_null.dentry = d_hash_and_lookup(selinux_null.mnt->mnt_root, > > &null_name); > > if (IS_ERR(selinux_null.dentry)) { > > pr_err("selinuxfs: could not lookup null!\n"); > > err = PTR_ERR(selinux_null.dentry); > > selinux_null.dentry = NULL; > > + return err; > > Casey's correct in that we probably don't need this, but it does harden > the source a bit against future changes so it isn't entirely a bad > thing. If nothing else, some new kernel dev will get excited writing a > oneliner several years from now that removes the redundant return; I'm > getting exited about the patch just thinking about it. I put this seemingly superfluous return statement, due an additional patch which adds code in between the end of this branch and the final return statement. We could change the final return to always return 0, since all errors are now handled on their own, to make things more obvious. > Anyway, thanks for noticing this and submitting a patch Christian, > beyond the above nitpick, everything looks good to me. I'm going to > merge this into selinux/stable-6.9 with a stable marking and assuming > all goes well I'll send this up to Linus in a few days. > > Thanks! > > > } > > > > return err; > > -- > > 2.43.0 > > -- > paul-moore.com
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index d18581d741e8..7e9aa5d151b4 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -2125,7 +2125,6 @@ static struct file_system_type sel_fs_type = { .kill_sb = sel_kill_sb, }; -static struct vfsmount *selinuxfs_mount __ro_after_init; struct path selinux_null __ro_after_init; static int __init init_sel_fs(void) @@ -2147,18 +2146,21 @@ static int __init init_sel_fs(void) return err; } - selinux_null.mnt = selinuxfs_mount = kern_mount(&sel_fs_type); - if (IS_ERR(selinuxfs_mount)) { + selinux_null.mnt = kern_mount(&sel_fs_type); + if (IS_ERR(selinux_null.mnt)) { pr_err("selinuxfs: could not mount!\n"); - err = PTR_ERR(selinuxfs_mount); - selinuxfs_mount = NULL; + err = PTR_ERR(selinux_null.mnt); + selinux_null.mnt = NULL; + return err; } + selinux_null.dentry = d_hash_and_lookup(selinux_null.mnt->mnt_root, &null_name); if (IS_ERR(selinux_null.dentry)) { pr_err("selinuxfs: could not lookup null!\n"); err = PTR_ERR(selinux_null.dentry); selinux_null.dentry = NULL; + return err; } return err;