* [PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock @ 2022-01-20 21:49 Scott Mayhew 2022-01-20 21:49 ` [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() Scott Mayhew 2022-01-20 21:49 ` [PATCH RFC v2 2/2] selinux: try to use preparsed sid before calling parse_sid() Scott Mayhew 0 siblings, 2 replies; 16+ messages in thread From: Scott Mayhew @ 2022-01-20 21:49 UTC (permalink / raw) To: selinux, linux-nfs, linux-kernel selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and shouldn't be performing any memory allocations. The first patch fixes this by parsing the sids at the same time the context mount options are being parsed from the mount options string and storing the parsed sids in the selinux_mnt_opts struct. The second patch adds logic to selinux_set_mnt_opts() and selinux_sb_remount() that checks to see if a sid has already been parsed before calling parse_sid(), and adds the parsed sids to the data being copied in selinux_fs_context_dup(). Scott Mayhew (2): selinux: Fix selinux_sb_mnt_opts_compat() selinux: try to use preparsed sid before calling parse_sid() security/selinux/hooks.c | 181 ++++++++++++++++++++++++++++----------- 1 file changed, 129 insertions(+), 52 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-20 21:49 [PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock Scott Mayhew @ 2022-01-20 21:49 ` Scott Mayhew 2022-01-24 21:27 ` Paul Moore 2022-01-27 9:54 ` Ondrej Mosnacek 2022-01-20 21:49 ` [PATCH RFC v2 2/2] selinux: try to use preparsed sid before calling parse_sid() Scott Mayhew 1 sibling, 2 replies; 16+ messages in thread From: Scott Mayhew @ 2022-01-20 21:49 UTC (permalink / raw) To: selinux, linux-nfs, linux-kernel selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and shouldn't be performing any memory allocations. Fix this by parsing the sids at the same time we're chopping up the security mount options string and then using the pre-parsed sids when doing the comparison. Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 36 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 5b6895e4fc29..f27ca9e870c0 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) struct selinux_mnt_opts { const char *fscontext, *context, *rootcontext, *defcontext; + u32 fscontext_sid; + u32 context_sid; + u32 rootcontext_sid; + u32 defcontext_sid; + unsigned short preparsed; }; static void selinux_free_mnt_opts(void *mnt_opts) @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, return 0; } -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, - gfp_t gfp) +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) { int rc = security_context_str_to_sid(&selinux_state, s, - sid, gfp); - if (rc) + sid, GFP_KERNEL); + if (rc && sb != NULL) pr_warn("SELinux: security_context_str_to_sid" "(%s) failed for (dev %s, type %s) errno=%d\n", s, sb->s_id, sb->s_type->name, rc); @@ -673,8 +677,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); if (rc) goto out; if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, @@ -683,8 +686,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->context, &context_sid); if (rc) goto out; if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, @@ -693,8 +695,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); if (rc) goto out; if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, @@ -703,8 +704,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); if (rc) goto out; if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) { struct selinux_mnt_opts *opts = *mnt_opts; bool is_alloc_opts = false; + bool preparse_sid = false; + u32 sid; + int rc; if (token == Opt_seclabel) /* eaten and completely ignored */ @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) is_alloc_opts = true; } + if (selinux_initialized(&selinux_state)) + preparse_sid = true; + switch (token) { case Opt_context: if (opts->context || opts->defcontext) goto err; opts->context = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->context_sid = sid; + opts->preparsed |= CONTEXT_MNT; + } + } break; case Opt_fscontext: if (opts->fscontext) goto err; opts->fscontext = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->fscontext_sid = sid; + opts->preparsed |= FSCONTEXT_MNT; + } + } break; case Opt_rootcontext: if (opts->rootcontext) goto err; opts->rootcontext = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->rootcontext_sid = sid; + opts->preparsed |= ROOTCONTEXT_MNT; + } + } break; case Opt_defcontext: if (opts->context || opts->defcontext) goto err; opts->defcontext = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->defcontext_sid = sid; + opts->preparsed |= DEFCONTEXT_MNT; + } + } break; } @@ -2648,8 +2682,6 @@ static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts) { struct selinux_mnt_opts *opts = mnt_opts; struct superblock_security_struct *sbsec = sb->s_security; - u32 sid; - int rc; /* * Superblock not initialized (i.e. no options) - reject if any @@ -2666,35 +2698,43 @@ static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts) return (sbsec->flags & SE_MNTMASK) ? 1 : 0; if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid)) + if (opts->preparsed & FSCONTEXT_MNT) { + if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, + opts->fscontext_sid)) + return 1; + } else { return 1; + } } if (opts->context) { - rc = parse_sid(sb, opts->context, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid)) + if (opts->preparsed & CONTEXT_MNT) { + if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, + opts->context_sid)) + return 1; + } else { return 1; + } } if (opts->rootcontext) { - struct inode_security_struct *root_isec; + if (opts->preparsed & ROOTCONTEXT_MNT) { + struct inode_security_struct *root_isec; - root_isec = backing_inode_security(sb->s_root); - rc = parse_sid(sb, opts->rootcontext, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid)) + root_isec = backing_inode_security(sb->s_root); + if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, + opts->rootcontext_sid)) + return 1; + } else { return 1; + } } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid)) + if (opts->preparsed & DEFCONTEXT_MNT) { + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, + opts->defcontext_sid)) + return 1; + } else { return 1; + } } return 0; } @@ -2713,14 +2753,14 @@ static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) return 0; if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->fscontext, &sid); if (rc) return rc; if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid)) goto out_bad_option; } if (opts->context) { - rc = parse_sid(sb, opts->context, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->context, &sid); if (rc) return rc; if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid)) @@ -2729,14 +2769,14 @@ static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) if (opts->rootcontext) { struct inode_security_struct *root_isec; root_isec = backing_inode_security(sb->s_root); - rc = parse_sid(sb, opts->rootcontext, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->rootcontext, &sid); if (rc) return rc; if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid)) goto out_bad_option; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->defcontext, &sid); if (rc) return rc; if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid)) -- 2.31.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-20 21:49 ` [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() Scott Mayhew @ 2022-01-24 21:27 ` Paul Moore 2022-01-25 17:30 ` Scott Mayhew 2022-01-27 9:54 ` Ondrej Mosnacek 1 sibling, 1 reply; 16+ messages in thread From: Paul Moore @ 2022-01-24 21:27 UTC (permalink / raw) To: Scott Mayhew; +Cc: selinux, linux-nfs, linux-kernel On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > shouldn't be performing any memory allocations. Fix this by parsing the > sids at the same time we're chopping up the security mount options > string and then using the pre-parsed sids when doing the comparison. > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > 1 file changed, 76 insertions(+), 36 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 5b6895e4fc29..f27ca9e870c0 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) > > struct selinux_mnt_opts { > const char *fscontext, *context, *rootcontext, *defcontext; > + u32 fscontext_sid; > + u32 context_sid; > + u32 rootcontext_sid; > + u32 defcontext_sid; > + unsigned short preparsed; > }; Is the preparsed field strictly necessary? Can't we just write the code to assume that if a given SID field is not SECSID_NULL then it is valid/preparsed? > @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, > return 0; > } > > -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, > - gfp_t gfp) > +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) > { > int rc = security_context_str_to_sid(&selinux_state, s, > - sid, gfp); > - if (rc) > + sid, GFP_KERNEL); > + if (rc && sb != NULL) > pr_warn("SELinux: security_context_str_to_sid" > "(%s) failed for (dev %s, type %s) errno=%d\n", > s, sb->s_id, sb->s_type->name, rc); It seems like it would still be useful to see the warning even when sb is NULL, wouldn't you say? How about something like this: if (rc) pr_warn("SELinux: blah blah blah (dev %s, type %s) blah blah\n", (sb ? sb->s_id : "?"), (sb ? sb->s_type->name : "?")); > @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > { > struct selinux_mnt_opts *opts = *mnt_opts; > bool is_alloc_opts = false; > + bool preparse_sid = false; > + u32 sid; > + int rc; > > if (token == Opt_seclabel) > /* eaten and completely ignored */ > @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > is_alloc_opts = true; > } > > + if (selinux_initialized(&selinux_state)) > + preparse_sid = true; Since there is no looping in selinux_add_opt, and you can only specify one token/option for a given call to this function, it seems like we can do away with preparse_sid and just do the selinux_initialized(...) check directly in the code below, yes? > switch (token) { > case Opt_context: > if (opts->context || opts->defcontext) > goto err; > opts->context = s; > + if (preparse_sid) { > + rc = parse_sid(NULL, s, &sid); > + if (rc == 0) { > + opts->context_sid = sid; > + opts->preparsed |= CONTEXT_MNT; > + } > + } Is there a reason why we need a dedicated sid variable as opposed to passing opt->context_sid as the parameter? For example: rc = parse_sid(NULL, s, &opts->context_sid); -- paul moore paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-24 21:27 ` Paul Moore @ 2022-01-25 17:30 ` Scott Mayhew 2022-01-25 17:45 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Scott Mayhew @ 2022-01-25 17:30 UTC (permalink / raw) To: Paul Moore; +Cc: selinux, linux-nfs, linux-kernel On Mon, 24 Jan 2022, Paul Moore wrote: > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > shouldn't be performing any memory allocations. Fix this by parsing the > > sids at the same time we're chopping up the security mount options > > string and then using the pre-parsed sids when doing the comparison. > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > 1 file changed, 76 insertions(+), 36 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 5b6895e4fc29..f27ca9e870c0 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) > > > > struct selinux_mnt_opts { > > const char *fscontext, *context, *rootcontext, *defcontext; > > + u32 fscontext_sid; > > + u32 context_sid; > > + u32 rootcontext_sid; > > + u32 defcontext_sid; > > + unsigned short preparsed; > > }; > > Is the preparsed field strictly necessary? Can't we just write the > code to assume that if a given SID field is not SECSID_NULL then it is > valid/preparsed? The preparsed field isn't necessary. I'll change it. > > > @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, > > return 0; > > } > > > > -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, > > - gfp_t gfp) > > +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) > > { > > int rc = security_context_str_to_sid(&selinux_state, s, > > - sid, gfp); > > - if (rc) > > + sid, GFP_KERNEL); > > + if (rc && sb != NULL) > > pr_warn("SELinux: security_context_str_to_sid" > > "(%s) failed for (dev %s, type %s) errno=%d\n", > > s, sb->s_id, sb->s_type->name, rc); > > It seems like it would still be useful to see the warning even when sb > is NULL, wouldn't you say? How about something like this: > > if (rc) > pr_warn("SELinux: blah blah blah (dev %s, type %s) blah blah\n", > (sb ? sb->s_id : "?"), > (sb ? sb->s_type->name : "?")); I agree, that would be useful. > > > @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > > { > > struct selinux_mnt_opts *opts = *mnt_opts; > > bool is_alloc_opts = false; > > + bool preparse_sid = false; > > + u32 sid; > > + int rc; > > > > if (token == Opt_seclabel) > > /* eaten and completely ignored */ > > @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > > is_alloc_opts = true; > > } > > > > + if (selinux_initialized(&selinux_state)) > > + preparse_sid = true; > > Since there is no looping in selinux_add_opt, and you can only specify > one token/option for a given call to this function, it seems like we > can do away with preparse_sid and just do the selinux_initialized(...) > check directly in the code below, yes? Will do. > > > switch (token) { > > case Opt_context: > > if (opts->context || opts->defcontext) > > goto err; > > opts->context = s; > > + if (preparse_sid) { > > + rc = parse_sid(NULL, s, &sid); > > + if (rc == 0) { > > + opts->context_sid = sid; > > + opts->preparsed |= CONTEXT_MNT; > > + } > > + } > > Is there a reason why we need a dedicated sid variable as opposed to > passing opt->context_sid as the parameter? For example: > > rc = parse_sid(NULL, s, &opts->context_sid); We don't need a dedicated sid variable. Should I make similar changes in the second patch (get rid of the local sid variable in selinux_sb_remount() and the *context_sid variables in selinux_set_mnt_opts())? Thanks, Scott > > -- > paul moore > paul-moore.com > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-25 17:30 ` Scott Mayhew @ 2022-01-25 17:45 ` Paul Moore 2022-01-25 18:51 ` Scott Mayhew 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2022-01-25 17:45 UTC (permalink / raw) To: Scott Mayhew; +Cc: selinux, linux-nfs, linux-kernel On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > On Mon, 24 Jan 2022, Paul Moore wrote: > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > sids at the same time we're chopping up the security mount options > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > --- > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > 1 file changed, 76 insertions(+), 36 deletions(-) ... > > > switch (token) { > > > case Opt_context: > > > if (opts->context || opts->defcontext) > > > goto err; > > > opts->context = s; > > > + if (preparse_sid) { > > > + rc = parse_sid(NULL, s, &sid); > > > + if (rc == 0) { > > > + opts->context_sid = sid; > > > + opts->preparsed |= CONTEXT_MNT; > > > + } > > > + } > > > > Is there a reason why we need a dedicated sid variable as opposed to > > passing opt->context_sid as the parameter? For example: > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > We don't need a dedicated sid variable. Should I make similar changes > in the second patch (get rid of the local sid variable in > selinux_sb_remount() and the *context_sid variables in > selinux_set_mnt_opts())? Yes please, I should have explicitly mentioned that. Thanks. -- paul moore paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-25 17:45 ` Paul Moore @ 2022-01-25 18:51 ` Scott Mayhew 2022-01-25 22:32 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Scott Mayhew @ 2022-01-25 18:51 UTC (permalink / raw) To: Paul Moore; +Cc: selinux, linux-nfs, linux-kernel On Tue, 25 Jan 2022, Paul Moore wrote: > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > > sids at the same time we're chopping up the security mount options > > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > --- > > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > > 1 file changed, 76 insertions(+), 36 deletions(-) > > ... > > > > > switch (token) { > > > > case Opt_context: > > > > if (opts->context || opts->defcontext) > > > > goto err; > > > > opts->context = s; > > > > + if (preparse_sid) { > > > > + rc = parse_sid(NULL, s, &sid); > > > > + if (rc == 0) { > > > > + opts->context_sid = sid; > > > > + opts->preparsed |= CONTEXT_MNT; > > > > + } > > > > + } > > > > > > Is there a reason why we need a dedicated sid variable as opposed to > > > passing opt->context_sid as the parameter? For example: > > > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > > > We don't need a dedicated sid variable. Should I make similar changes > > in the second patch (get rid of the local sid variable in > > selinux_sb_remount() and the *context_sid variables in > > selinux_set_mnt_opts())? > > Yes please, I should have explicitly mentioned that. Actually, delayed_superblock_init() calls selinux_set_mnt_opts() with mnt_opts == NULL, so there would have to be a lot of checks like if (opts && opts->fscontext_sid) { in the later parts of that function, which is kind of clunky. I can still do it if you want though. -Scott > > Thanks. > > -- > paul moore > paul-moore.com > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-25 18:51 ` Scott Mayhew @ 2022-01-25 22:32 ` Paul Moore 2022-01-26 20:41 ` Scott Mayhew 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2022-01-25 22:32 UTC (permalink / raw) To: Scott Mayhew; +Cc: selinux, linux-nfs, linux-kernel On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <smayhew@redhat.com> wrote: > On Tue, 25 Jan 2022, Paul Moore wrote: > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > > > sids at the same time we're chopping up the security mount options > > > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > > --- > > > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > > > 1 file changed, 76 insertions(+), 36 deletions(-) > > > > ... > > > > > > > switch (token) { > > > > > case Opt_context: > > > > > if (opts->context || opts->defcontext) > > > > > goto err; > > > > > opts->context = s; > > > > > + if (preparse_sid) { > > > > > + rc = parse_sid(NULL, s, &sid); > > > > > + if (rc == 0) { > > > > > + opts->context_sid = sid; > > > > > + opts->preparsed |= CONTEXT_MNT; > > > > > + } > > > > > + } > > > > > > > > Is there a reason why we need a dedicated sid variable as opposed to > > > > passing opt->context_sid as the parameter? For example: > > > > > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > > > > > We don't need a dedicated sid variable. Should I make similar changes > > > in the second patch (get rid of the local sid variable in > > > selinux_sb_remount() and the *context_sid variables in > > > selinux_set_mnt_opts())? > > > > Yes please, I should have explicitly mentioned that. > > Actually, delayed_superblock_init() calls selinux_set_mnt_opts() with > mnt_opts == NULL, so there would have to be a lot of checks like > > if (opts && opts->fscontext_sid) { > > in the later parts of that function, which is kind of clunky. I can > still do it if you want though. I might be misunderstanding your concern, but in selinux_set_mnt_opts() all of the "opts->XXX" if-conditionals are protected by being inside an if-statement that checks to ensure "opts" is not NULL. Am I missing something? -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-25 22:32 ` Paul Moore @ 2022-01-26 20:41 ` Scott Mayhew 2022-01-28 1:53 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Scott Mayhew @ 2022-01-26 20:41 UTC (permalink / raw) To: Paul Moore; +Cc: selinux, linux-nfs, linux-kernel On Tue, 25 Jan 2022, Paul Moore wrote: > On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <smayhew@redhat.com> wrote: > > On Tue, 25 Jan 2022, Paul Moore wrote: > > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > > > > sids at the same time we're chopping up the security mount options > > > > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > > > --- > > > > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > > > > 1 file changed, 76 insertions(+), 36 deletions(-) > > > > > > ... > > > > > > > > > switch (token) { > > > > > > case Opt_context: > > > > > > if (opts->context || opts->defcontext) > > > > > > goto err; > > > > > > opts->context = s; > > > > > > + if (preparse_sid) { > > > > > > + rc = parse_sid(NULL, s, &sid); > > > > > > + if (rc == 0) { > > > > > > + opts->context_sid = sid; > > > > > > + opts->preparsed |= CONTEXT_MNT; > > > > > > + } > > > > > > + } > > > > > > > > > > Is there a reason why we need a dedicated sid variable as opposed to > > > > > passing opt->context_sid as the parameter? For example: > > > > > > > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > > > > > > > We don't need a dedicated sid variable. Should I make similar changes > > > > in the second patch (get rid of the local sid variable in > > > > selinux_sb_remount() and the *context_sid variables in > > > > selinux_set_mnt_opts())? > > > > > > Yes please, I should have explicitly mentioned that. > > > > Actually, delayed_superblock_init() calls selinux_set_mnt_opts() with > > mnt_opts == NULL, so there would have to be a lot of checks like > > > > if (opts && opts->fscontext_sid) { > > > > in the later parts of that function, which is kind of clunky. I can > > still do it if you want though. > > I might be misunderstanding your concern, but in > selinux_set_mnt_opts() all of the "opts->XXX" if-conditionals are > protected by being inside an if-statement that checks to ensure "opts" > is not NULL. Am I missing something? Sorry for being unclear. The parts where the sids are (potentially) being parsed are inside an "if (opts) { ... }" block... but later in the function those sids are used in various tests/assignments. So if we wanted to eliminate the four local context_sid variables (using the variables in opts instead), then there would need to be additional checks to avoid dereferencing opts when it's NULL. In other words, if the local context_sid variables are kept, then the change to selinux_set_mnt_opts() would look like this (option A): ---8<--- @@ -676,36 +676,48 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid); - if (rc) - goto out; + if (opts->fscontext_sid == SECSID_NULL ) { + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); + if (rc) + goto out; + } else + fscontext_sid = opts->fscontext_sid; if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, fscontext_sid)) goto out_double_mount; sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid); - if (rc) - goto out; + if (opts->context_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->context, &context_sid); + if (rc) + goto out; + } else + context_sid = opts->context_sid; if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, context_sid)) goto out_double_mount; sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); - if (rc) - goto out; + if (opts->rootcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); + if (rc) + goto out; + } else + rootcontext_sid = opts->rootcontext_sid; if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, rootcontext_sid)) goto out_double_mount; sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid); - if (rc) - goto out; + if (opts->defcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); + if (rc) + goto out; + } else + defcontext_sid = opts->defcontext_sid; if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, defcontext_sid)) goto out_double_mount; ---8<--- and if the local context_sid variables are removed, the change to selinux_set_mnt_opts() would look like this (option B): ---8<--- @@ -627,8 +627,6 @@ static int selinux_set_mnt_opts(struct super_block *sb, struct dentry *root = sb->s_root; struct selinux_mnt_opts *opts = mnt_opts; struct inode_security_struct *root_isec; - u32 fscontext_sid = 0, context_sid = 0, rootcontext_sid = 0; - u32 defcontext_sid = 0; int rc = 0; mutex_lock(&sbsec->lock); @@ -676,38 +674,50 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid); - if (rc) - goto out; + if (opts->fscontext_sid == SECSID_NULL ) { + rc = parse_sid(sb, opts->fscontext, + &opts->fscontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, - fscontext_sid)) + opts->fscontext_sid)) goto out_double_mount; sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid); - if (rc) - goto out; + if (opts->context_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->context, + &opts->context_sid); + if (rc) + goto out; + } if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, - context_sid)) + opts->context_sid)) goto out_double_mount; sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); - if (rc) - goto out; + if (opts->rootcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->rootcontext, + &opts->rootcontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, - rootcontext_sid)) + opts->rootcontext_sid)) goto out_double_mount; sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid); - if (rc) - goto out; + if (opts->defcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->defcontext, + &opts->defcontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, - defcontext_sid)) + opts->defcontext_sid)) goto out_double_mount; sbsec->flags |= DEFCONTEXT_MNT; } @@ -760,8 +770,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, strcmp(sb->s_type->name, "ramfs") && strcmp(sb->s_type->name, "devpts") && strcmp(sb->s_type->name, "overlay")) { - if (context_sid || fscontext_sid || rootcontext_sid || - defcontext_sid) { + if (opts && (opts->context_sid || opts->fscontext_sid || + opts->rootcontext_sid || opts->defcontext_sid)) { rc = -EACCES; goto out; } @@ -779,12 +789,13 @@ static int selinux_set_mnt_opts(struct super_block *sb, } /* sets the context of the superblock for the fs being mounted. */ - if (fscontext_sid) { - rc = may_context_mount_sb_relabel(fscontext_sid, sbsec, cred); + if (opts && opts->fscontext_sid) { + rc = may_context_mount_sb_relabel(opts->fscontext_sid, + sbsec, cred); if (rc) goto out; - sbsec->sid = fscontext_sid; + sbsec->sid = opts->fscontext_sid; } /* @@ -792,42 +803,43 @@ static int selinux_set_mnt_opts(struct super_block *sb, * sets the label used on all file below the mountpoint, and will set * the superblock context if not already set. */ - if (kern_flags & SECURITY_LSM_NATIVE_LABELS && !context_sid) { + if (kern_flags & SECURITY_LSM_NATIVE_LABELS && + !(opts && opts->context_sid)) { sbsec->behavior = SECURITY_FS_USE_NATIVE; *set_kern_flags |= SECURITY_LSM_NATIVE_LABELS; } - if (context_sid) { - if (!fscontext_sid) { - rc = may_context_mount_sb_relabel(context_sid, sbsec, - cred); + if (opts && opts->context_sid) { + if (!opts->fscontext_sid) { + rc = may_context_mount_sb_relabel(opts->context_sid, + sbsec, cred); if (rc) goto out; - sbsec->sid = context_sid; + sbsec->sid = opts->context_sid; } else { - rc = may_context_mount_inode_relabel(context_sid, sbsec, - cred); + rc = may_context_mount_inode_relabel(opts->context_sid, + sbsec, cred); if (rc) goto out; } - if (!rootcontext_sid) - rootcontext_sid = context_sid; + if (!opts->rootcontext_sid) + opts->rootcontext_sid = opts->context_sid; - sbsec->mntpoint_sid = context_sid; + sbsec->mntpoint_sid = opts->context_sid; sbsec->behavior = SECURITY_FS_USE_MNTPOINT; } - if (rootcontext_sid) { - rc = may_context_mount_inode_relabel(rootcontext_sid, sbsec, - cred); + if (opts && opts->rootcontext_sid) { + rc = may_context_mount_inode_relabel(opts->rootcontext_sid, + sbsec, cred); if (rc) goto out; - root_isec->sid = rootcontext_sid; + root_isec->sid = opts->rootcontext_sid; root_isec->initialized = LABEL_INITIALIZED; } - if (defcontext_sid) { + if (opts && opts->defcontext_sid) { if (sbsec->behavior != SECURITY_FS_USE_XATTR && sbsec->behavior != SECURITY_FS_USE_NATIVE) { rc = -EINVAL; @@ -836,14 +848,14 @@ static int selinux_set_mnt_opts(struct super_block *sb, goto out; } - if (defcontext_sid != sbsec->def_sid) { - rc = may_context_mount_inode_relabel(defcontext_sid, + if (opts->defcontext_sid != sbsec->def_sid) { + rc = may_context_mount_inode_relabel(opts->defcontext_sid, sbsec, cred); if (rc) goto out; } - sbsec->def_sid = defcontext_sid; + sbsec->def_sid = opts->defcontext_sid; } out_set_opts: ---8<--- -Scott > > -- > paul-moore.com > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-26 20:41 ` Scott Mayhew @ 2022-01-28 1:53 ` Paul Moore 0 siblings, 0 replies; 16+ messages in thread From: Paul Moore @ 2022-01-28 1:53 UTC (permalink / raw) To: Scott Mayhew; +Cc: selinux, linux-nfs, linux-kernel On Wed, Jan 26, 2022 at 3:42 PM Scott Mayhew <smayhew@redhat.com> wrote: > On Tue, 25 Jan 2022, Paul Moore wrote: > > On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > On Tue, 25 Jan 2022, Paul Moore wrote: > > > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: ... > Sorry for being unclear. The parts where the sids are (potentially) > being parsed are inside an "if (opts) { ... }" block... but later in the > function those sids are used in various tests/assignments. So if we > wanted to eliminate the four local context_sid variables (using the > variables in opts instead), then there would need to be additional > checks to avoid dereferencing opts when it's NULL. Okay, gotcha. I think keeping the local variables is the right thing to do. My apologies for not noticing that earlier. -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-20 21:49 ` [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() Scott Mayhew 2022-01-24 21:27 ` Paul Moore @ 2022-01-27 9:54 ` Ondrej Mosnacek 2022-01-28 2:25 ` Paul Moore 1 sibling, 1 reply; 16+ messages in thread From: Ondrej Mosnacek @ 2022-01-27 9:54 UTC (permalink / raw) To: Scott Mayhew; +Cc: SElinux list, linux-nfs, Linux kernel mailing list On Thu, Jan 20, 2022 at 10:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > shouldn't be performing any memory allocations. Fix this by parsing the > sids at the same time we're chopping up the security mount options > string and then using the pre-parsed sids when doing the comparison. > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > 1 file changed, 76 insertions(+), 36 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 5b6895e4fc29..f27ca9e870c0 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) > > struct selinux_mnt_opts { > const char *fscontext, *context, *rootcontext, *defcontext; > + u32 fscontext_sid; > + u32 context_sid; > + u32 rootcontext_sid; > + u32 defcontext_sid; > + unsigned short preparsed; > }; > > static void selinux_free_mnt_opts(void *mnt_opts) > @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, > return 0; > } > > -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, > - gfp_t gfp) > +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) > { > int rc = security_context_str_to_sid(&selinux_state, s, > - sid, gfp); > - if (rc) > + sid, GFP_KERNEL); > + if (rc && sb != NULL) > pr_warn("SELinux: security_context_str_to_sid" > "(%s) failed for (dev %s, type %s) errno=%d\n", > s, sb->s_id, sb->s_type->name, rc); > @@ -673,8 +677,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > */ > if (opts) { > if (opts->fscontext) { > - rc = parse_sid(sb, opts->fscontext, &fscontext_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); > if (rc) > goto out; > if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, > @@ -683,8 +686,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > sbsec->flags |= FSCONTEXT_MNT; > } > if (opts->context) { > - rc = parse_sid(sb, opts->context, &context_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->context, &context_sid); > if (rc) > goto out; > if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, > @@ -693,8 +695,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > sbsec->flags |= CONTEXT_MNT; > } > if (opts->rootcontext) { > - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); > if (rc) > goto out; > if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, > @@ -703,8 +704,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > sbsec->flags |= ROOTCONTEXT_MNT; > } > if (opts->defcontext) { > - rc = parse_sid(sb, opts->defcontext, &defcontext_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); > if (rc) > goto out; > if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, > @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > { > struct selinux_mnt_opts *opts = *mnt_opts; > bool is_alloc_opts = false; > + bool preparse_sid = false; > + u32 sid; > + int rc; > > if (token == Opt_seclabel) > /* eaten and completely ignored */ > @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > is_alloc_opts = true; > } > > + if (selinux_initialized(&selinux_state)) > + preparse_sid = true; > + I wonder if we could make this all much simpler by *always* doing the label parsing in selinux_add_opt() and just returning an error when !selinux_initialized(&selinux_state). Before the new mount API, mount options were always passed directly to the mount(2) syscall, so it wasn't possible to pass any SELinux mount options before the SELinux policy was loaded. I don't see why we need to jump through hoops here just to support this pseudo-feature of stashing an unparsed label into an fs_context before policy is loaded... Userspace should never need to do that. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-27 9:54 ` Ondrej Mosnacek @ 2022-01-28 2:25 ` Paul Moore 2022-01-31 12:46 ` Ondrej Mosnacek 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2022-01-28 2:25 UTC (permalink / raw) To: Ondrej Mosnacek Cc: Scott Mayhew, SElinux list, linux-nfs, Linux kernel mailing list On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > I wonder if we could make this all much simpler by *always* doing the > label parsing in selinux_add_opt() and just returning an error when > !selinux_initialized(&selinux_state). Before the new mount API, mount > options were always passed directly to the mount(2) syscall, so it > wasn't possible to pass any SELinux mount options before the SELinux > policy was loaded. I don't see why we need to jump through hoops here > just to support this pseudo-feature of stashing an unparsed label into > an fs_context before policy is loaded... Userspace should never need > to do that. I could agree with that, although part of my mind is a little nervous about the "userspace should *never* ..." because that always seems to bite us. Although I'm struggling to think of a case where userspace would need to set explicit SELinux mount options without having a policy loaded. -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-28 2:25 ` Paul Moore @ 2022-01-31 12:46 ` Ondrej Mosnacek 2022-01-31 16:16 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Ondrej Mosnacek @ 2022-01-31 12:46 UTC (permalink / raw) To: Paul Moore Cc: Scott Mayhew, SElinux list, linux-nfs, Linux kernel mailing list On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > I wonder if we could make this all much simpler by *always* doing the > > label parsing in selinux_add_opt() and just returning an error when > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > options were always passed directly to the mount(2) syscall, so it > > wasn't possible to pass any SELinux mount options before the SELinux > > policy was loaded. I don't see why we need to jump through hoops here > > just to support this pseudo-feature of stashing an unparsed label into > > an fs_context before policy is loaded... Userspace should never need > > to do that. > > I could agree with that, although part of my mind is a little nervous > about the "userspace should *never* ..." because that always seems to > bite us. Although I'm struggling to think of a case where userspace > would need to set explicit SELinux mount options without having a > policy loaded. I get that, but IMO this is enough of an odd "use case" that I wouldn't worry too much. To be affected by this, someone would need to: 1. Use the new mount API, which: a) doesn't even have man pages yet, b) isn't even used by the mount(8) utility yet. 2. Call fsconfig(2) with a SELinux mount option before policy is loaded. 3. Call fsmount(2) with the same fd after policy is loaded. And racing with the policy load doesn't count - that could fail randomly with or without the change. I honestly can't imagine any realistic scenario where someone would do this... -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-31 12:46 ` Ondrej Mosnacek @ 2022-01-31 16:16 ` Paul Moore 2022-02-01 14:38 ` Ondrej Mosnacek 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2022-01-31 16:16 UTC (permalink / raw) To: Ondrej Mosnacek Cc: Scott Mayhew, SElinux list, linux-nfs, Linux kernel mailing list On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > I wonder if we could make this all much simpler by *always* doing the > > > label parsing in selinux_add_opt() and just returning an error when > > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > > options were always passed directly to the mount(2) syscall, so it > > > wasn't possible to pass any SELinux mount options before the SELinux > > > policy was loaded. I don't see why we need to jump through hoops here > > > just to support this pseudo-feature of stashing an unparsed label into > > > an fs_context before policy is loaded... Userspace should never need > > > to do that. > > > > I could agree with that, although part of my mind is a little nervous > > about the "userspace should *never* ..." because that always seems to > > bite us. Although I'm struggling to think of a case where userspace > > would need to set explicit SELinux mount options without having a > > policy loaded. > > I get that, but IMO this is enough of an odd "use case" that I > wouldn't worry too much ... I understand, but seeing as I'm the only one that defends these things with Linus and others lets do this: 1. Fix what we have now using Scott's patches once he incorporates the feedback. 2. Merge another patch (separate patch(set) please!) which does the parsing in selinux_add_opt(). ... this was if we have to revert #2 we still have the fixes in #1. -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-01-31 16:16 ` Paul Moore @ 2022-02-01 14:38 ` Ondrej Mosnacek 2022-02-01 16:19 ` Paul Moore 0 siblings, 1 reply; 16+ messages in thread From: Ondrej Mosnacek @ 2022-02-01 14:38 UTC (permalink / raw) To: Paul Moore Cc: Scott Mayhew, SElinux list, linux-nfs, Linux kernel mailing list On Mon, Jan 31, 2022 at 5:16 PM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > I wonder if we could make this all much simpler by *always* doing the > > > > label parsing in selinux_add_opt() and just returning an error when > > > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > > > options were always passed directly to the mount(2) syscall, so it > > > > wasn't possible to pass any SELinux mount options before the SELinux > > > > policy was loaded. I don't see why we need to jump through hoops here > > > > just to support this pseudo-feature of stashing an unparsed label into > > > > an fs_context before policy is loaded... Userspace should never need > > > > to do that. > > > > > > I could agree with that, although part of my mind is a little nervous > > > about the "userspace should *never* ..." because that always seems to > > > bite us. Although I'm struggling to think of a case where userspace > > > would need to set explicit SELinux mount options without having a > > > policy loaded. > > > > I get that, but IMO this is enough of an odd "use case" that I > > wouldn't worry too much ... > > I understand, but seeing as I'm the only one that defends these things > with Linus and others lets do this: It's not all black and white: https://lore.kernel.org/lkml/Pine.LNX.4.64.0512291322560.3298@g5.osdl.org/ > 1. Fix what we have now using Scott's patches once he incorporates the feedback. > 2. Merge another patch (separate patch(set) please!) which does the > parsing in selinux_add_opt(). > > ... this was if we have to revert #2 we still have the fixes in #1. Sounds good to me. I can prepare the simplification patch. If anyone does come to complain, then by all means, let's revert it. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() 2022-02-01 14:38 ` Ondrej Mosnacek @ 2022-02-01 16:19 ` Paul Moore 0 siblings, 0 replies; 16+ messages in thread From: Paul Moore @ 2022-02-01 16:19 UTC (permalink / raw) To: Ondrej Mosnacek Cc: Scott Mayhew, SElinux list, linux-nfs, Linux kernel mailing list On Tue, Feb 1, 2022 at 9:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Mon, Jan 31, 2022 at 5:16 PM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > I wonder if we could make this all much simpler by *always* doing the > > > > > label parsing in selinux_add_opt() and just returning an error when > > > > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > > > > options were always passed directly to the mount(2) syscall, so it > > > > > wasn't possible to pass any SELinux mount options before the SELinux > > > > > policy was loaded. I don't see why we need to jump through hoops here > > > > > just to support this pseudo-feature of stashing an unparsed label into > > > > > an fs_context before policy is loaded... Userspace should never need > > > > > to do that. > > > > > > > > I could agree with that, although part of my mind is a little nervous > > > > about the "userspace should *never* ..." because that always seems to > > > > bite us. Although I'm struggling to think of a case where userspace > > > > would need to set explicit SELinux mount options without having a > > > > policy loaded. > > > > > > I get that, but IMO this is enough of an odd "use case" that I > > > wouldn't worry too much ... > > > > I understand, but seeing as I'm the only one that defends these things > > with Linus and others lets do this: > > It's not all black and white: > https://lore.kernel.org/lkml/Pine.LNX.4.64.0512291322560.3298@g5.osdl.org/ I made my statement above not to ask your opinion, but rather to make a point. -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH RFC v2 2/2] selinux: try to use preparsed sid before calling parse_sid() 2022-01-20 21:49 [PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock Scott Mayhew 2022-01-20 21:49 ` [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() Scott Mayhew @ 2022-01-20 21:49 ` Scott Mayhew 1 sibling, 0 replies; 16+ messages in thread From: Scott Mayhew @ 2022-01-20 21:49 UTC (permalink / raw) To: selinux, linux-nfs, linux-kernel Avoid unnecessary parsing of sids that have already been parsed via selinux_sb_eat_lsm_opts(). Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- security/selinux/hooks.c | 85 ++++++++++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 24 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f27ca9e870c0..28ba5c8529fa 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -677,36 +677,52 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid); - if (rc) - goto out; + if (opts->preparsed & FSCONTEXT_MNT) + fscontext_sid = opts->fscontext_sid; + else { + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, fscontext_sid)) goto out_double_mount; sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid); - if (rc) - goto out; + if (opts->preparsed & CONTEXT_MNT) + context_sid = opts->context_sid; + else { + rc = parse_sid(sb, opts->context, &context_sid); + if (rc) + goto out; + } if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, context_sid)) goto out_double_mount; sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); - if (rc) - goto out; + if (opts->preparsed & ROOTCONTEXT_MNT) + rootcontext_sid = opts->rootcontext_sid; + else { + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, rootcontext_sid)) goto out_double_mount; sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid); - if (rc) - goto out; + if (opts->preparsed & DEFCONTEXT_MNT) + defcontext_sid = opts->defcontext_sid; + else { + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, defcontext_sid)) goto out_double_mount; @@ -2753,32 +2769,48 @@ static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) return 0; if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &sid); - if (rc) - return rc; + if (opts->preparsed & FSCONTEXT_MNT) + sid = opts->fscontext_sid; + else { + rc = parse_sid(sb, opts->fscontext, &sid); + if (rc) + return rc; + } if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid)) goto out_bad_option; } if (opts->context) { - rc = parse_sid(sb, opts->context, &sid); - if (rc) - return rc; + if (opts->preparsed & CONTEXT_MNT) + sid = opts->context_sid; + else { + rc = parse_sid(sb, opts->context, &sid); + if (rc) + return rc; + } if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid)) goto out_bad_option; } if (opts->rootcontext) { struct inode_security_struct *root_isec; root_isec = backing_inode_security(sb->s_root); - rc = parse_sid(sb, opts->rootcontext, &sid); - if (rc) - return rc; + if (opts->preparsed & ROOTCONTEXT_MNT) + sid = opts->rootcontext_sid; + else { + rc = parse_sid(sb, opts->rootcontext, &sid); + if (rc) + return rc; + } if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid)) goto out_bad_option; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &sid); - if (rc) - return rc; + if (opts->preparsed & DEFCONTEXT_MNT) + sid = opts->defcontext_sid; + else { + rc = parse_sid(sb, opts->defcontext, &sid); + if (rc) + return rc; + } if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid)) goto out_bad_option; } @@ -2877,6 +2909,11 @@ static int selinux_fs_context_dup(struct fs_context *fc, if (!opts->defcontext) return -ENOMEM; } + opts->fscontext_sid = src->fscontext_sid; + opts->context_sid = src->context_sid; + opts->rootcontext_sid = src->rootcontext_sid; + opts->defcontext_sid = src->defcontext_sid; + opts->preparsed = src->preparsed; return 0; } -- 2.31.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-02-01 16:19 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-20 21:49 [PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock Scott Mayhew 2022-01-20 21:49 ` [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() Scott Mayhew 2022-01-24 21:27 ` Paul Moore 2022-01-25 17:30 ` Scott Mayhew 2022-01-25 17:45 ` Paul Moore 2022-01-25 18:51 ` Scott Mayhew 2022-01-25 22:32 ` Paul Moore 2022-01-26 20:41 ` Scott Mayhew 2022-01-28 1:53 ` Paul Moore 2022-01-27 9:54 ` Ondrej Mosnacek 2022-01-28 2:25 ` Paul Moore 2022-01-31 12:46 ` Ondrej Mosnacek 2022-01-31 16:16 ` Paul Moore 2022-02-01 14:38 ` Ondrej Mosnacek 2022-02-01 16:19 ` Paul Moore 2022-01-20 21:49 ` [PATCH RFC v2 2/2] selinux: try to use preparsed sid before calling parse_sid() Scott Mayhew
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).