Re: [RFC PATCH 1/3] selinux: refactor sidtab conversion
On Tue, Nov 13, 2018 at 8:53 AM Ondrej Mosnacek wrote: > This is a purely cosmetic change that encapsulates the three-step sidtab > conversion logic (shutdown -> clone -> map) into a single function > defined in sidtab.c (as opposed to services.c). > > Signed-off-by: Ondrej Mosnacek > --- > security/selinux/ss/services.c | 22 +-- > security/selinux/ss/sidtab.c | 50 -- > security/selinux/ss/sidtab.h | 11 > 3 files changed, 42 insertions(+), 41 deletions(-) Merged into selinux/next with some whitespace fixes (inherited from code you cut n' pasted). Please remember to run your patches through scripts/checkpatch.pl before submission. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
ruct policydb *p, char *str, struct context *context,
> gfp_t gfp_mask);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 12e414394530..ccad4334f99d 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb
> *pol,
>
> ctx->type = typdatum->value;
>
> - rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
> - if (rc)
> - goto out;
> + if (!pol->mls_enabled) {
> + rc = -EINVAL;
> + if ((def_sid == SECSID_NULL || !oldc) && (*p) != '\0')
> + goto out;
> + } else if (!oldc) {
> + /*
> +* If a def_sid is provided and no MLS field is present,
> +* copy the MLS field of the associated default context.
> +* Used for upgrading to MLS systems where objects may lack
> +* MLS fields.
> +*/
> + struct context *defcon;
> +
> + rc = -EINVAL;
> + if (def_sid == SECSID_NULL)
> + goto out;
> +
> + defcon = sidtab_search(sidtabp, def_sid);
> + if (!defcon)
> + goto out;
> +
> + rc = mls_context_cpy(ctx, defcon);
> + if (rc)
> + goto out;
> + } else {
> + rc = mls_context_to_sid(pol, p, ctx);
> + if (rc)
> + goto out;
> + }
>
> /* Check the validity of the new context. */
> rc = -EINVAL;
> --
> 2.17.2
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
> char *scontext,
> > -struct context *context,
> > -struct sidtab *s,
> > -u32 def_sid);
> > +struct context *context);
> >
> > int mls_from_string(struct policydb *p, char *str, struct context
> > *context,
> > gfp_t gfp_mask);
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 12e414394530..ccad4334f99d 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb
> > *pol,
> >
> > ctx->type = typdatum->value;
> >
> > - rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
> > - if (rc)
> > - goto out;
> > + if (!pol->mls_enabled) {
> > + rc = -EINVAL;
> > + if ((def_sid == SECSID_NULL || !oldc) && (*p) != '\0')
> > + goto out;
>
> I don't think this is your bug, but unless I'm mistaken, p could be OOB
> and be dereferenced here. Seems to have been introduced by
> 95ffe194204ae3cef88d0b59be209204bbe9b3be. Likely not caught in testing
> since both Linux distributions and Android enable MLS to use the
> category sets for isolation.
Yep, and we should fix that in v4.20-rcX independent of this patch. I
think if we simply remove the "(*scontext) == '\0'" from the check we
should be okay; I believe the only time we would want to return 0 when
not running a MLS policy would be when there is something in the MLS
portion of the label.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 1/2] selinux: use separate table for initial SID lookup
On Fri, Nov 2, 2018 at 11:35 AM Ondrej Mosnacek wrote:
> On Wed, Oct 31, 2018 at 6:09 PM Stephen Smalley wrote:
> > On 10/31/2018 08:27 AM, Ondrej Mosnacek wrote:
> > > This patch separates the lookup of the initial SIDs into a separate
> > > lookup table (implemented simply by a fixed-size array), in order to
> > > pave the way for improving the process of converting the sidtab to a new
> > > policy during a policy reload.
> > >
> > > The initial SIDs are loaded directly and are skipped during sidtab
> > > conversion, so handling them separately makes things somewhat simpler.
> > > Since there is only a small fixed number of them, they can be stored in
> > > a simple lookup table.
> > >
> > > This patch also moves the fallback-to-unlabeled logic from sidtab.c to
> > > the new helper functions in services.c that now handle the unified
> > > lookup in both sidtab and isidtab, simplifying the sidtab interface.
> > >
> > > Signed-off-by: Ondrej Mosnacek
> > > ---
> > > security/selinux/include/security.h | 3 +
> > > security/selinux/ss/mls.c | 6 +-
> > > security/selinux/ss/mls.h | 2 +-
> > > security/selinux/ss/policydb.c | 24 ++-
> > > security/selinux/ss/policydb.h | 26 ++-
> > > security/selinux/ss/services.c | 238 +++-
> > > security/selinux/ss/services.h | 1 +
> > > security/selinux/ss/sidtab.c| 29 +---
> > > security/selinux/ss/sidtab.h| 3 +-
> > > 9 files changed, 187 insertions(+), 145 deletions(-)
> > >
> > > diff --git a/security/selinux/include/security.h
> > > b/security/selinux/include/security.h
> > > index 23e762d529fa..a1b4b13c2300 100644
> > > --- a/security/selinux/include/security.h
> > > +++ b/security/selinux/include/security.h
> > > @@ -221,6 +221,9 @@ struct extended_perms {
> > > /* definitions of av_decision.flags */
> > > #define AVD_FLAGS_PERMISSIVE0x0001
> > >
> > > +struct context *security_sid_to_context_struct(struct selinux_state
> > > *state,
> > > +u32 sid, int force);
> >
> > This header is for interfaces exposed by the security server (i.e. the
> > policy engine) to the AVC, hooks, and other policy enforcement code. The
> > context structure is private to the security server in order to
> > encapsulate the policy logic and should never be returned directly to
> > code outside of the security server. Technically you aren't actually
> > exposing the structure definition but this interface isn't useful
> > without doing so, so it shouldn't live here.
>
> Another option could be to refine mls_context_to_sid() so it doesn't
> need the sidtab lookup at all, moving that part to the call sites.
> That function has two callers and only one of them can really trigger
> the path with the lookup. I planned to look into doing this later (I
> didn't want to include unnecessary changes in this patchset), but now
> I actually tried doing it and it seems like a good simplification. I
> will fold it under these two patches in v2. After this change the
> helper function won't be needed outside services.c.
>
> >
> > You could make this a services_sid_to_context_struct() interface defined
> > in security/selinux/ss/services.h instead. Or you could keep all of
> > this within the sidtab, just making the isidtab part of its internal
> > state, and moving this logic inside of sidtab_search() instead of
> > splitting it out.
>
> My intention was to not hide too much complexity under sidtab, but
> rethinking it now I agree it would probably make sense to just hide
> isidtab under sidtab. It would need to have a separate insert function
> for initial SIDs (and in the second patch also some logic to switch to
> the new isidtab), but I guess that is less ugly than keeping it
> outside... I'll see if I can make it all a bit nicer.
FWIW, I agree with Stephen about managing the initial sids within the
context of the sidtab; conceptually it just makes sense.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v6] selinux: policydb - fix byte order and alignment issues
On Tue, Oct 23, 2018 at 3:02 AM Ondrej Mosnacek wrote:
> Do the LE conversions before doing the Infiniband-related range checks.
> The incorrect checks are otherwise causing a failure to load any policy
> with an ibendportcon rule on BE systems. This can be reproduced by
> running (on e.g. ppc64):
>
> cat >my_module.cil < (type test_ibendport_t)
> (roletype object_r test_ibendport_t)
> (ibendportcon mlx4_0 1 (system_u object_r test_ibendport_t ((s0) (s0
> EOF
> semodule -i my_module.cil
>
> Also, fix loading/storing the 64-bit subnet prefix for OCON_IBPKEY to
> use a correctly aligned buffer.
>
> Finally, do not use the 'nodebuf' (u32) buffer where 'buf' (__le32)
> should be used instead.
>
> Tested internally on a ppc64 machine with a RHEL 7 kernel with this
> patch applied.
>
> Cc: Daniel Jurgens
> Cc: Eli Cohen
> Cc: James Morris
> Cc: Doug Ledford
> Cc: # 4.13+
> Fixes: a806f7a1616f ("selinux: Create policydb version for Infiniband
> support")
> Signed-off-by: Ondrej Mosnacek
> ---
> security/selinux/ss/policydb.c | 51 --
> 1 file changed, 36 insertions(+), 15 deletions(-)
>
> Changes in v6:
> - use U8_MAX as the limit for ibendport.port value to emphasize that it
>is an 8-bit value
>
> Changes in v5:
> - defer also assignment to 8-bit ibendport.port
>
> Changes in v4:
> - defer assignment to 16-bit struct fields to after the range check
>
> Changes in v3:
> - use separate buffer for the 64-bit subnet_prefix
> - add comments on the byte ordering of subnet_prefix
> - deduplicate the le32_to_cpu() calls from checks
>
> Changes in v2:
> - add reproducer to commit message
> - update e-mail address of James Morris
> - better Cc also the old SELinux ML
You know what they say: sixth time is the charm :)
Merged into selinux/next, thanks all.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] libsepol: fix endianity in ibpkey range checks
On Wed, Oct 17, 2018 at 12:07 PM William Roberts
wrote:
> On Wed, Oct 17, 2018 at 7:48 AM Ondrej Mosnacek wrote:
> >
> > We need to convert from little-endian before dong range checks on the
> > ibpkey port numbers, otherwise we would be checking a wrong value.
> >
> > Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling")
> > Signed-off-by: Ondrej Mosnacek
> > ---
> > libsepol/src/policydb.c | 14 ++
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > index a6d76ca3..dc201e2f 100644
> > --- a/libsepol/src/policydb.c
> > +++ b/libsepol/src/policydb.c
> > @@ -2830,15 +2830,21 @@ static int ocontext_read_selinux(struct
> > policydb_compat_info *info,
> > break;
> > case OCON_IBPKEY:
> > rc = next_entry(buf, fp, sizeof(uint32_t) *
> > 4);
> > - if (rc < 0 || buf[2] > 0x || buf[3] >
> > 0x)
> > + if (rc < 0)
> > return -1;
> >
> > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]);
> > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
>
> Ahh you're assigning a 32 bit value to a 16 bit variable low|high_pkey. I
> think
> you need to assign them to a uint32_t if you want to actually range check
> them.
If you can, give me a chance to look over the kernel changes first. I
doubt I'll see anything objectionable given the review the patches
have already seen, but one never knows.
> > +
> > + if (c->u.ibpkey.low_pkey > 0x ||
> > + c->u.ibpkey.high_pkey > 0x)
> > + return -1;
> > +
> > + /* we want c->u.ibpkey.subnet_prefix in
> > network
> > +* (big-endian) order, just memcpy it */
> > memcpy(>u.ibpkey.subnet_prefix, buf,
> >sizeof(c->u.ibpkey.subnet_prefix));
> >
> > - c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]);
> > - c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
> > -
> > if (context_read_and_validate
> > (>context[0], p, fp))
> > return -1;
> > --
> > 2.17.2
> >
> See job: https://travis-ci.org/SELinuxProject/selinux/jobs/442750208
>
> Build fail with gcc:
>
> policydb.c:2839:31: error: comparison is always false due to limited
> range of data type [-Werror=type-limits]
> if (c->u.ibpkey.low_pkey > 0x ||
>^
> policydb.c:2840:31: error: comparison is always false due to limited
> range of data type [-Werror=type-limits]
> c->u.ibpkey.high_pkey > 0x)
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[GIT PULL] SELinux fixes for v4.19 (#1)
Hi Greg, We've got one SELinux "fix" that I'd like to get into v4.19 if possible. I'm using double quotes on "fix" as this is just an update to the MAINTAINERS file and not a code change. From my perspective, MAINTAINERS updates generally don't warrant inclusion during the -rcX phase, but this is a change to the mailing list location so it seemed prudent to get this in before v4.19 is released. If you don't want this for v4.19 let me know and I'll queue it up for the upcoming merge window. Thanks, -Paul -- The following changes since commit 7e4237faa7213c1cc1d0aa65a44c67ba4729ce9f: selinux: cleanup dentry and inodes on error in selinuxfs (2018-08-07 17:26:25 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20181015 for you to fetch changes up to 073c1a781e4a1217d572506621434cd6d750969b: MAINTAINERS: update the SELinux mailing list location (2018-10-10 01:50:15 -0400) selinux/stable-4.19 PR 20181015 ---- Paul Moore (1): MAINTAINERS: update the SELinux mailing list location MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
gt; > > > then either the new ptrace() api > > > > > > extension should be fixed to allow for this too or the seccomp() > > > > > > way of > > > > > > retrieving the pid - which I really think we want - needs to be > > > > > > fixed to > > > > > > require capable(CAP_SYS_ADMIN) too. > > > > > > The solution where both require ns_capable(CAP_SYS_ADMIN) is - imho > > > > > > - > > > > > > the preferred way to solve this. > > > > > > Everything else will just be confusing. > > > > > > > > > > First you say "broken", then you say "confusing". Which one do you > > > > > mean? > > > > > > > > Both. It's broken in so far as it places a seemingly unnecessary > > > > restriction that could be fixed. You outlined one possible fix yourself > > > > in the link you provided. > > > > > > If by "possible fix" you mean "check whether the seccomp filter is > > > only attached to a single task": That wouldn't fundamentally change > > > the situation, it would only add an additional special case. > > > > > > > And it's confusing in so far as there is a way > > > > via seccomp() to get the fd without said requirement. > > > > > > I don't find it confusing at all. seccomp() and ptrace() are very > > > > Fine, then that's a matter of opinion. I find it counterintuitive that > > you can get an fd without privileges via one interface but not via > > another. > > > > > different situations: When you use seccomp(), infrastructure is > > > > Sure. Note, that this is _one_ of the reasons why I want to make sure we > > keep the native seccomp() only based way of getting an fd without > > forcing userspace to switching to a differnet kernel api. > > > > > already in place for ensuring that your filter is only applied to > > > processes over which you are capable, and propagation is limited by > > > inheritance from your task down. When you use ptrace(), you need a > > > pretty different sort of access check that checks whether you're > > > privileged over ancestors, siblings and so on of the target task. > > > > So, don't get me wrong I'm not arguing against the ptrace() interface in > > general. If this is something that people find useful, fine. But, I > > would like to have a simple single-syscall pure-seccomp() based way of > > getting an fd, i.e. what we have in patch 1 of this series. > > Yeah, I also prefer the seccomp() one. > > > > But thinking about it more, I think that CAP_SYS_ADMIN over the saved > > > current->mm->user_ns of the task that installed the filter (stored as > > > a "struct user_namespace *" in the filter) should be acceptable. > > > > Hm... Why not CAP_SYS_PTRACE? > > Because LSMs like SELinux add extra checks that apply even if you have > CAP_SYS_PTRACE, and this would subvert those. The only capability I > know of that lets you bypass LSM checks by design (if no LSM blocks > the capability itself) is CAP_SYS_ADMIN. > > > One more thing. Citing from [1] > > > > > I think there's a security problem here. Imagine the following scenario: > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses > > > SECCOMP_RET_USER_NOTIF > > > 2. task A forks off a child B > > > 3. task B uses setuid(1) to drop its privileges > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1) > > > or via execve() > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B > > > > Sorry, to be late to the party but would this really pass > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me > > that it would... Doesn't look like it would get past: > > > > tcred = __task_cred(task); > > if (uid_eq(caller_uid, tcred->euid) && > > uid_eq(caller_uid, tcred->suid) && > > uid_eq(caller_uid, tcred->uid) && > > gid_eq(caller_gid, tcred->egid) && > > gid_eq(caller_gid, tcred->sgid) && > > gid_eq(caller_gid, tcred->gid)) > > goto ok; > > if (ptrace_has_cap(tcred->user_ns, mode)) > > goto ok; > > rcu_read_unlock(); > > return -EPERM; > > ok: > > rcu_read_unlock(); > > mm = task->mm; > > if (mm && > > ((get_dumpable(mm) != SUID_DUMP_USER) && > > !ptrace_has_cap(mm->user_ns, mode))) > > return -EPERM; > > Which specific check would prevent task C from attaching to task B? If > the UIDs match, the first "goto ok" executes; and you're dumpable, so > you don't trigger the second "return -EPERM". > > > > 7. because the seccomp filter is shared by task A and task B, task C > > > is now able to influence syscall results for syscalls performed by > > > task A > > > > [1]: > > https://lore.kernel.org/lkml/CAG48ez3R+ZJ1vwGkDfGzKX2mz6f=jjjwso5pcvnh68p+rko...@mail.gmail.com/ -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] MAINTAINERS: update the SELinux mailing list location
On Wed, Oct 10, 2018 at 1:55 AM Paul Moore wrote: > > Signed-off-by: Paul Moore > --- > MAINTAINERS |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Since we want to get everyone on to the new list as soon as possible, I've merged this into the selinux/stable-4.19 branch and I plan on sending this up to Greg in the next day or two barring any objections on the list. > diff --git a/MAINTAINERS b/MAINTAINERS > index 67e4c4f92ba9..fd060218baa8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12775,7 +12775,7 @@ SELINUX SECURITY MODULE > M: Paul Moore > M: Stephen Smalley > M: Eric Paris > -L: selinux@tycho.nsa.gov (moderated for non-subscribers) > +L: seli...@vger.kernel.org > W: https://selinuxproject.org > W: https://github.com/SELinuxProject > T: git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] MAINTAINERS: update the SELinux mailing list location
Signed-off-by: Paul Moore --- MAINTAINERS |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 67e4c4f92ba9..fd060218baa8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12775,7 +12775,7 @@ SELINUX SECURITY MODULE M: Paul Moore M: Stephen Smalley M: Eric Paris -L: selinux@tycho.nsa.gov (moderated for non-subscribers) +L: seli...@vger.kernel.org W: https://selinuxproject.org W: https://github.com/SELinuxProject T: git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] selinux-testsuite: update the dependencies in README.md
The overlayfs tests require setfattr and getfattr which are part of the attr package in Fedora. Signed-off-by: Paul Moore --- README.md |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2c871d3..cf90ef6 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,7 @@ similar dependencies): * netlabel\_tools _(to load NetLabel configuration during `inet_socket` tests)_ * iptables _(to load the `iptables SECMARK` rules during `inet_socket` tests)_ * lksctp-tools-devel _(to build the SCTP test programs)_ +* attr _(tools used by the overlayfs tests)_ On a modern Fedora system you can install these dependencies with the following command: @@ -63,7 +64,8 @@ following command: net-tools \ netlabel_tools \ iptables \ - lksctp-tools-devel + lksctp-tools-devel \ + attr The testsuite requires a pre-existing base policy configuration of SELinux, using either the old example policy or the reference policy as the baseline. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling
On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley wrote: > On 09/25/2018 01:45 AM, Taras Kondratiuk wrote: > > Quoting Paul Moore (2018-09-24 20:46:57) > >> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley > >> wrote: > >>> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote: > >>>> Quoting Stephen Smalley (2018-09-20 07:49:12) > >>>>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote: > >>>>>> Quoting Stephen Smalley (2018-09-19 12:00:33) > >>>>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote: > >> > >> ... > >> > >>>> IMO it would be more consistent if defcontext cover all "unlabeled" > >>>> groups. It seems unlikely to me that somebody who currently uses > >>>> defcontext can somehow rely on mapping invalid labels to unlabeled > >>>> instead of default context. > >>> > >>> Yes, and that seems more consistent with the current documentation in > >>> the mount man page for defcontext=. > >>> > >>> I'd be inclined to change selinux_inode_notifysecctx() to call > >>> security_context_to_sid_default() directly instead of using > >>> selinux_inode_setsecurity() and change security_context_to_sid_core() > >>> and sidtab_search_core() as suggested above to save and use the def_sid > >>> instead of SECINITSID_UNLABELED always (initializing the context def_sid > >>> to SECINITSID_UNLABELED as the default). selinux_inode_setsecurity() we > >>> should leave unchanged, or if we change it at all, it should be more > >>> like the handling in selinux_inode_setxattr(). The notifysecctx hook is > >>> invoked by the filesystem to notify the security module of the file's > >>> existing security context, so in that case we always want the _default > >>> behavior, whereas the setsecurity hook is invoked by the vfs or the > >>> filesystem to set the security context of a file to a new value, so in > >>> that case we would only use the _force interface if the caller had > >>> CAP_MAC_ADMIN. > >>> > >>> Paul, what say you? NB This would be a user-visible behavior change for > >>> mounts specifying defcontext= on xattr filesystems; files with invalid > >>> contexts will then show up with the defcontext value instead of the > >>> unlabeled context. If that's too risky, then we'd need a flag or > >>> something to security_context_to_sid_default() to distinguish the > >>> behaviors and only set it when called from selinux_inode_notifysecctx(). > >> > >> Visible changes like this are always worrisome, even though I think it > >> is safe to assume that the defcontext option is not widely used. I'd > >> feel much better if this change was opt-in. > >> > >> Which brings about it's own problems. We have the policy capability > >> functionality, but that is likely a poor fit for this as the policy > >> capabilities are usually controlled by the Linux distribution while > >> the mount options are set by the system's administrator when the > >> filesystem is mounted. We could add a toggle somewhere in selinuxfs, > >> but I really dislike that idea, and would prefer to find a different > >> solution if possible. I'm not sure how much flak we would get for > >> introducing a new mount option, but perhaps that is the best way to > >> handle this: defcontext would continue to behave as it does now, but > >> new option X would behave as mentioned in this thread. > >> > >> Thoughts? > > > > The new option X will also mean "default context", so it will be pretty > > hard to come up with a different but a sensible name. I'm afraid that > > having two options with hardly distinguishable semantics will be very > > confusing. > > > > What about a kernel config option that modifies the behavior of > > defcontext? > > First, the existing documentation for defcontext= leaves open the door > to the proposed new behavior. From mount(8): > "You can set the default security context for unlabeled files using > defcontext= option. This overrides the value set for unlabeled files in > the policy and requires a filesystem that supports xattr labeling." > > "Unlabeled files" could just mean files without any xattr, or it could > mean all files that either lack an xattr or have an invalid one under > the policy, since both sets of files are currently mapped to the > unlabeled context. This
Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling
On Tue, Sep 25, 2018 at 1:45 AM Taras Kondratiuk wrote: > Quoting Paul Moore (2018-09-24 20:46:57) > > On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley wrote: > > > On 09/20/2018 06:59 PM, Taras Kondratiuk wrote: > > > > Quoting Stephen Smalley (2018-09-20 07:49:12) > > > >> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote: > > > >>> Quoting Stephen Smalley (2018-09-19 12:00:33) > > > >>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote: > > > > ... > > > > > > IMO it would be more consistent if defcontext cover all "unlabeled" > > > > groups. It seems unlikely to me that somebody who currently uses > > > > defcontext can somehow rely on mapping invalid labels to unlabeled > > > > instead of default context. > > > > > > Yes, and that seems more consistent with the current documentation in > > > the mount man page for defcontext=. > > > > > > I'd be inclined to change selinux_inode_notifysecctx() to call > > > security_context_to_sid_default() directly instead of using > > > selinux_inode_setsecurity() and change security_context_to_sid_core() > > > and sidtab_search_core() as suggested above to save and use the def_sid > > > instead of SECINITSID_UNLABELED always (initializing the context def_sid > > > to SECINITSID_UNLABELED as the default). selinux_inode_setsecurity() we > > > should leave unchanged, or if we change it at all, it should be more > > > like the handling in selinux_inode_setxattr(). The notifysecctx hook is > > > invoked by the filesystem to notify the security module of the file's > > > existing security context, so in that case we always want the _default > > > behavior, whereas the setsecurity hook is invoked by the vfs or the > > > filesystem to set the security context of a file to a new value, so in > > > that case we would only use the _force interface if the caller had > > > CAP_MAC_ADMIN. > > > > > > Paul, what say you? NB This would be a user-visible behavior change for > > > mounts specifying defcontext= on xattr filesystems; files with invalid > > > contexts will then show up with the defcontext value instead of the > > > unlabeled context. If that's too risky, then we'd need a flag or > > > something to security_context_to_sid_default() to distinguish the > > > behaviors and only set it when called from selinux_inode_notifysecctx(). > > > > Visible changes like this are always worrisome, even though I think it > > is safe to assume that the defcontext option is not widely used. I'd > > feel much better if this change was opt-in. > > > > Which brings about it's own problems. We have the policy capability > > functionality, but that is likely a poor fit for this as the policy > > capabilities are usually controlled by the Linux distribution while > > the mount options are set by the system's administrator when the > > filesystem is mounted. We could add a toggle somewhere in selinuxfs, > > but I really dislike that idea, and would prefer to find a different > > solution if possible. I'm not sure how much flak we would get for > > introducing a new mount option, but perhaps that is the best way to > > handle this: defcontext would continue to behave as it does now, but > > new option X would behave as mentioned in this thread. > > > > Thoughts? > > The new option X will also mean "default context", so it will be pretty > hard to come up with a different but a sensible name. I'm afraid that > having two options with hardly distinguishable semantics will be very > confusing. > > What about a kernel config option that modifies the behavior of > defcontext? A kernel config option would be going in the wrong direction; that would put it almost completely under the control of the distribution. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling
On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley wrote: > On 09/20/2018 06:59 PM, Taras Kondratiuk wrote: > > Quoting Stephen Smalley (2018-09-20 07:49:12) > >> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote: > >>> Quoting Stephen Smalley (2018-09-19 12:00:33) > >>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote: ... > > IMO it would be more consistent if defcontext cover all "unlabeled" > > groups. It seems unlikely to me that somebody who currently uses > > defcontext can somehow rely on mapping invalid labels to unlabeled > > instead of default context. > > Yes, and that seems more consistent with the current documentation in > the mount man page for defcontext=. > > I'd be inclined to change selinux_inode_notifysecctx() to call > security_context_to_sid_default() directly instead of using > selinux_inode_setsecurity() and change security_context_to_sid_core() > and sidtab_search_core() as suggested above to save and use the def_sid > instead of SECINITSID_UNLABELED always (initializing the context def_sid > to SECINITSID_UNLABELED as the default). selinux_inode_setsecurity() we > should leave unchanged, or if we change it at all, it should be more > like the handling in selinux_inode_setxattr(). The notifysecctx hook is > invoked by the filesystem to notify the security module of the file's > existing security context, so in that case we always want the _default > behavior, whereas the setsecurity hook is invoked by the vfs or the > filesystem to set the security context of a file to a new value, so in > that case we would only use the _force interface if the caller had > CAP_MAC_ADMIN. > > Paul, what say you? NB This would be a user-visible behavior change for > mounts specifying defcontext= on xattr filesystems; files with invalid > contexts will then show up with the defcontext value instead of the > unlabeled context. If that's too risky, then we'd need a flag or > something to security_context_to_sid_default() to distinguish the > behaviors and only set it when called from selinux_inode_notifysecctx(). Visible changes like this are always worrisome, even though I think it is safe to assume that the defcontext option is not widely used. I'd feel much better if this change was opt-in. Which brings about it's own problems. We have the policy capability functionality, but that is likely a poor fit for this as the policy capabilities are usually controlled by the Linux distribution while the mount options are set by the system's administrator when the filesystem is mounted. We could add a toggle somewhere in selinuxfs, but I really dislike that idea, and would prefer to find a different solution if possible. I'm not sure how much flak we would get for introducing a new mount option, but perhaps that is the best way to handle this: defcontext would continue to behave as it does now, but new option X would behave as mentioned in this thread. Thoughts? -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling
On Wed, Sep 19, 2018 at 12:52 PM Taras Kondratiuk wrote:
> When files on NFSv4 server are not properly labeled (label doesn't match
> a policy on a client) they will end up with unlabeled_t type which is
> too generic. We would like to be able to set a default context per
> mount. 'defcontext' mount option looks like a nice solution, but it
> doesn't seem to be fully implemented for native labeling. Default
> context is stored, but is never used.
>
> The patch adds a fallback to a default context if a received context is
> invalid. If the inode context is already initialized, then it is left
> untouched to preserve a context set locally on a client.
>
> Signed-off-by: Taras Kondratiuk
> ---
> security/selinux/hooks.c | 25 -
> 1 file changed, 24 insertions(+), 1 deletion(-)
The idea seems reasonable to me; if we want to treat labeled NFS like
we treat local filesystems we should also support the defcontext mount
option.
However, I wonder if we are better off generalizing some of the
SECURITY_FS_USE_XATTR based code in inode_doinit_with_dentry() such
that it can be used both in selinux_inode_notifysecctx() and in
inode_doinit_with_dentry(). Or maybe we just need a sbsec->def_sid
variant of selinux_inode_setsecurity(). Regardless, the key is the
call to security_context_to_sid_default(), the
selinux_inode_setsecurity() function only calls
security_context_to_sid().
Just in case anyone is wondering, I'm not sure I want to update
selinux_inode_setsecurity() to call security_context_to_sid_default();
at least not without more investigation.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ad9a9b8e9979..f7debe798bf5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6598,7 +6598,30 @@ static void selinux_inode_invalidate_secctx(struct
> inode *inode)
> */
> static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32
> ctxlen)
> {
> - return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx,
> ctxlen, 0);
> + struct superblock_security_struct *sbsec;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx,
> ctxlen, 0);
> +
> + /*
> +* In case of Native labeling with defcontext mount option fall back
> +* to a default SID if received context is invalid.
> +*/
> + if (rc == -EINVAL) {
> + sbsec = inode->i_sb->s_security;
> + if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
> + sbsec->flags & DEFCONTEXT_MNT) {
> + isec = inode->i_security;
> + if (!isec->initialized) {
> + isec->sclass =
> inode_mode_to_security_class(inode->i_mode);
> + isec->sid = sbsec->def_sid;
> + isec->initialized = 1;
> + }
> + rc = 0;
> + }
> + }
> + return rc;
> }
>
> /*
> --
> 2.10.3.dirty
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
FYI: email change
A quick note that my @redhat.com email address is going to stop working in the next day or two, so if you are using my Red Hat email address to reach me please start using my @paul-moore.com address. Everything else, e.g. my community involvement, will remain unaffected. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 5:52 PM Kees Cook wrote: > On Thu, Sep 13, 2018 at 2:38 PM, Paul Moore wrote: > > The infrastructure bits aren't really my concern; in fact I *like* > > that the infrastructure is always exercised, it makes > > testing/debugging easier. I also like the ability to limit the > > user/admin to one LSM at boot time to make support easier; my goal is > > to allow a distro to build support for multiple LSMs without also > > requiring that distro to support *stacked* LSMs > > I see your point, but as soon as SARA and Landlock appear, they'll have: > > depends on SECURITY_STACKING > > and then all distros will enable it and there will be no sensible > runtime way to manage it. If, instead, we make it entirely runtime > now, then a CONFIG can control the default state and we can provide > guidance to how SARA and Landlock should expose their "enable"ness. I question why SARA and LandLock require stacking. While some LSMs may benefit from stacking, e.g. Yama, traditionally each LSM has been able to stand on its own. I think this is a quality that should be preserved. > > (see my earlier > > comments about the difficulty in determining the source of a failed > > operation). > > Agreed. I would hope that audit could help for that case. *stare at blue sky* *also staring at blue sky* Audit can help, but it is independent of the LSMs and not a hard requirement for all, and even when it is enabled the config might not be suitable to provide enough information to be helpful in this case. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 4:58 PM Jordan Glover
wrote:
>
> On Thursday, September 13, 2018 9:12 PM, Paul Moore
> wrote:
>
> > On Thu, Sep 13, 2018 at 11:19 AM Kees Cook keesc...@chromium.org wrote:
> >
> > > On Thu, Sep 13, 2018 at 6:16 AM, Paul Moore p...@paul-moore.com wrote:
> > >
> > > > On Thu, Sep 13, 2018 at 12:19 AM Kees Cook keesc...@chromium.org wrote:
...
> > > > > I don't see a good reason to make this a config. Why shouldn't this
> > > > > always be enabled?
> > > >
> > > > I do. From a user perspective it is sometimes difficult to determine
> > > > the reason behind a failed operation; its is a DAC based denial, the
> > > > LSM, or some other failure? Stacking additional LSMs has the
> > > > potential to make this worse. The boot time configuration adds to the
> > > > complexity.
> > >
> > > Let me try to convince you otherwise. :) The reason I think there's no
> > > need for this is because the only functional change here is how
> > > TOMOYO gets stacked. And in my proposal, we can convert TOMOYO to be
> > > enabled/disabled like LoadPin. Given the configs I showed, stacking
> > > TOMOYO with the other major LSMs becomes a config (and/or boottime)
> > > option.
> > > The changes for TOMOYO are still needed even with SECURITY_STACKING,
> > > and I argue that the other major LSMs remain the same. It's only
> > > infrastructure that has changed. So, I think having SECURITY_STACKING
> > > actually makes things more complex internally (all the ifdefs, weird
> > > enable logic) and for distros ("what's this stacking option", etc?)
> >
> > None of the above deals with the user experience or support burden a
> > distro would have by forcing stacking on. If we make it an option the
> > distros can choose for themselves; picking a kernel build config is
> > not something new to distros, and I think Casey's text adequately
> > explains CONFIG_SECURITY_STACKING in terms that would be sufficient.
>
> CONFIG_SECURITY_STACKING doesn't make any user visible changes on
> itself as it doesn't automatically enable any new LSM. The LSM
> specific configs are place where users/distros make decisions. If
> there is only one LSM enabled to run then there's nothing to stack.
> If someone choose to run two or more LSM in config/boot cmdline
> then we can assume having it stacked is what they wanted. As Kees
> pointed there is already CONFIG_SECURITY_DEFAULT_XXX. In both cases
> CONFIG_SECURITY_STACKING is redundant and only adds burden instead
> of removing it.
See my last response to Kees.
> > I currently have a neutral stance on stacking, making it mandatory
> > pushes me more towards a "no".
>
> This implies that your real concern is something else than
> CONFIG_SECURITY_STACKING which only allows you to ignore the whole
> thing. Please reveal it. There are a lot of people waiting for LSM
> stacking which is several years late and it would be great to
> resolve potential issues earlier rather later.
What? I resent the implication that I'm hiding anything; there are a
lot of fair criticisms you could level at me, but I take offense at
the idea that I'm not being honest here. I've been speaking with
Casey, John, and others about stacking for years, both on-list and
in-person at conferences, and my
neutral-opinion-just-make-it-work-for-everything-and-make-it-optional
stance has been pretty consistent and isn't new.
Also, let's be really clear here: I'm only asking that stacking be
made a build time option (as it is in Casey's patchset). That seems
like a pretty modest ask for something so significant and "several
years late" as you put it.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 5:01 PM Kees Cook wrote:
> On Thu, Sep 13, 2018 at 12:12 PM, Paul Moore wrote:
> > None of the above deals with the user experience or support burden a
> > distro would have by forcing stacking on. If we make it an option the
>
> Just to make sure we're clear here: this series does not provide
> "extreme" stacking: SELinux, AppArmor, and SMACK remain boot-exclusive
> no matter what the CONFIGs.
>
> > distros can choose for themselves; picking a kernel build config is
> > not something new to distros, and I think Casey's text adequately
> > explains CONFIG_SECURITY_STACKING in terms that would be sufficient.
>
> I absolutely want stacking to be configurable, but I want to point out
> that there is no operational difference between
> CONFIG_SECURITY_STACKING=n and CONFIG_SECURITY_STACKING=y in the code
> here:
>
> - all the new accessor and allocation code is exercised in both cases
>
> - with stacking enabled: selinux, apparmor, and smack have an offset
> of 0 into blobs (and only one can be enabled at a time)
>
> - with stacking disabled: selinux, apparmor, and smack have an offset
> of 0 into blobs (and only one can be enabled at a time)
>
> The only behavioral difference is TOMOYO:
>
> 1- with stacking disabled and TOMOYO as the only major LSM, it will
> have a 0 offset into blobs (like above)
>
> 2- with stacking enabled and TOMOYO as the only major LSM, it will
> have a 0 offset into blobs (like above)
>
> 3- with stacking disabled and another major LSM is enabled, TOMOYO
> will be disabled (like always)
>
> 4- with stacking enabled and another major LSM is enabled, TOMOYO will
> have a non-0 offset into blobs and will run after selinux or smack or
> run before apparmor (based on link ordering defined by the Makefile).
Case #3/#4 is what I'm getting at, and I would argue demonstrates an
operational difference that is user visible/configurable.
Unless something has changed and I missed it, you can currently build
all of the LSMs into a single kernel image, and the admin/user can
choose one at boot time. CONFIG_SECURITY_STACKING=y enables the
admin/user to stack LSMs (albeit with restrictions in the current
iteration) and CONFIG_SECURITY_STACKING=n limits the admin/user to a
single LSM (what we have now). I understand that as of this moment we
are talking only about TOMOYO and AppArmor/Smack/SELinux, but everyone
knows that S.A.R.A/SARA and LandLock are going to follow shortly -
that's the whole point of this latest spin, isn't it?
> > I currently have a neutral stance on stacking, making it mandatory
> > pushes me more towards a "no".
>
> This is why I'm trying to explain myself: the infrastructure proposed
> here is always exercised, no matter the CONFIG. From that sense it is
> "mandatory" no matter what the config is. There isn't a reality where
> you could "turn off stacking", because it's not stacking until you
> actually stack something, and that will be disabled by default as I've
> proposed it.
>
> Let me put this another way: if we simply leave off patch 10, we can
> take the other 9 patches (modulo feedback), and we only have to decide
> how to expose "stacking"; all the infrastructure work for supporting
> it is done.
>
> I'm arguing that "security=" is likely insufficient to describe what
> we want, and instead we should focus on individual LSM enablement via
> parameters ("tomoyo.enabled=1"). If _ordering_ becomes an issue, we
> could either use parameter order, or use "security=" again maybe, but
> for now, ordering is already defined by the Makefile (and
> security/security.c).
The infrastructure bits aren't really my concern; in fact I *like*
that the infrastructure is always exercised, it makes
testing/debugging easier. I also like the ability to limit the
user/admin to one LSM at boot time to make support easier; my goal is
to allow a distro to build support for multiple LSMs without also
requiring that distro to support *stacked* LSMs (see my earlier
comments about the difficulty in determining the source of a failed
operation).
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 8:55 AM peter enderborg wrote: > On 09/13/2018 01:11 PM, Michal Hocko wrote: > > On Thu 13-09-18 09:12:04, peter enderborg wrote: > >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > >>> On 2018/09/13 12:02, Paul Moore wrote: > >>>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >>>> wrote: > >>>>> syzbot is hitting warning at str_read() [1] because len parameter can > >>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>>>> this case. > >>>>> > >>>>> [1] > >>>>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>>>> > >>>>> Signed-off-by: Tetsuo Handa > >>>>> Reported-by: syzbot > >>>>> > >>>>> --- > >>>>> security/selinux/ss/policydb.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/security/selinux/ss/policydb.c > >>>>> b/security/selinux/ss/policydb.c > >>>>> index e9394e7..f4eadd3 100644 > >>>>> --- a/security/selinux/ss/policydb.c > >>>>> +++ b/security/selinux/ss/policydb.c > >>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, > >>>>> void *fp, u32 len) > >>>>> if ((len == 0) || (len == (u32)-1)) > >>>>> return -EINVAL; > >>>>> > >>>>> - str = kmalloc(len + 1, flags); > >>>>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>>>> if (!str) > >>>>> return -ENOMEM; > >>>> Thanks for the patch. > >>>> > >>>> My eyes are starting to glaze over a bit chasing down all of the > >>>> different kmalloc() code paths trying to ensure that this always does > >>>> the right thing based on size of the allocation and the different slab > >>>> allocators ... are we sure that this will always return NULL when (len > >>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >>>> configurations? > >>>> > >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > >>> > >>> The only concern would be whether you want allocation failure messages. > >>> I assumed you don't need it because we are returning -ENOMEM to the > >>> caller. > >>> > >> Would it not be better with > >> > >> char *str; > >> > >> if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > >> return -EINVAL; > >> > >> str = kmalloc(len + 1, flags); > >> if (!str) > >> return -ENOMEM; > > I strongly suspect that you want kvmalloc rather than kmalloc here. The > > larger the request the more likely is the allocation to fail. > > > > I am not familiar with the code but I assume this is a root only > > interface so we don't have to worry about nasty users scenario. > > > I don't think we get any big data there at all. Usually less than 32 bytes. > However this data can be in fast path so a vmalloc is not an option. > > And some of the calls are GFP_ATOMC. Based on all the comments it looks like Tetsuo's original patch is probably the best fix right now. I'm going to merge this into selinux/next. Tetsuo, thanks for the patch, and thanks to everyone else for the comments/review. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 3:12 AM peter enderborg wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] > >>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa > >>> Reported-by: syzbot > >>> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c > >>> b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > >>> *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; As long as it's safe, I'd rather leave the maximum allocation limit as a kmalloc internal and let kmalloc return NULL if we try too large of an allocation. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu, Sep 13, 2018 at 2:26 AM Tetsuo Handa wrote: > On 2018/09/13 12:02, Paul Moore wrote: > > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > > wrote: > >> syzbot is hitting warning at str_read() [1] because len parameter can > >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >> this case. > >> > >> [1] > >> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >> > >> Signed-off-by: Tetsuo Handa > >> Reported-by: syzbot > >> --- > >> security/selinux/ss/policydb.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/security/selinux/ss/policydb.c > >> b/security/selinux/ss/policydb.c > >> index e9394e7..f4eadd3 100644 > >> --- a/security/selinux/ss/policydb.c > >> +++ b/security/selinux/ss/policydb.c > >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > >> *fp, u32 len) > >> if ((len == 0) || (len == (u32)-1)) > >> return -EINVAL; > >> > >> - str = kmalloc(len + 1, flags); > >> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >> if (!str) > >> return -ENOMEM; > > > > Thanks for the patch. > > > > My eyes are starting to glaze over a bit chasing down all of the > > different kmalloc() code paths trying to ensure that this always does > > the right thing based on size of the allocation and the different slab > > allocators ... are we sure that this will always return NULL when (len > > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > > configurations? > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > The only concern would be whether you want allocation failure messages. > I assumed you don't need it because we are returning -ENOMEM to the caller. I'm not to worried about the failure messages, returning -ENOMEM should be sufficient in this case. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 11:19 AM Kees Cook wrote: > On Thu, Sep 13, 2018 at 6:16 AM, Paul Moore wrote: > > On Thu, Sep 13, 2018 at 12:19 AM Kees Cook wrote: > >> On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler > >> wrote: > >> > Two proposed security modules require the ability to > >> > share security blobs with existing "major" security modules. > >> > These modules, S.A.R.A and LandLock, provide significantly > >> > different services than SELinux, Smack or AppArmor. Using > >> > either in conjunction with the existing modules is quite > >> > reasonable. S.A.R.A requires access to the cred blob, while > >> > LandLock uses the cred, file and inode blobs. > >> > > >> > The use of the cred, file and inode blobs has been > >> > abstracted in preceding patches in the series. This > >> > patch teaches the affected security modules how to access > >> > the part of the blob set aside for their use in the case > >> > where blobs are shared. The configuration option > >> > CONFIG_SECURITY_STACKING identifies systems where the > >> > blobs may be shared. > >> > > >> > The mechanism for selecting which security modules are > >> > active has been changed to allow non-conflicting "major" > >> > security modules to be used together. At this time the > >> > TOMOYO module can safely be used with any of the others. > >> > The two new modules would be non-conflicting as well. > >> > > >> > Signed-off-by: Casey Schaufler > >> > --- > >> > Documentation/admin-guide/LSM/index.rst | 14 +++-- > >> > include/linux/lsm_hooks.h | 2 +- > >> > security/Kconfig| 81 + > >> > security/apparmor/include/cred.h| 8 +++ > >> > security/apparmor/include/file.h| 9 ++- > >> > security/apparmor/include/lib.h | 4 ++ > >> > security/apparmor/lsm.c | 8 ++- > >> > security/security.c | 30 - > >> > security/selinux/hooks.c| 3 +- > >> > security/selinux/include/objsec.h | 18 +- > >> > security/smack/smack.h | 19 +- > >> > security/smack/smack_lsm.c | 17 +++--- > >> > security/tomoyo/common.h| 12 +++- > >> > security/tomoyo/tomoyo.c| 3 +- > >> > 14 files changed, 200 insertions(+), 28 deletions(-) > > > > ... > > > >> > diff --git a/security/Kconfig b/security/Kconfig > >> > index 22f7664c4977..ed48025ae9e0 100644 > >> > --- a/security/Kconfig > >> > +++ b/security/Kconfig > >> > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS > >> > bool > >> > default n > >> > > >> > +config SECURITY_STACKING > >> > + bool "Security module stacking" > >> > + depends on SECURITY > >> > + help > >> > + Allows multiple major security modules to be stacked. > >> > + Modules are invoked in the order registered with a > >> > + "bail on fail" policy, in which the infrastructure > >> > + will stop processing once a denial is detected. Not > >> > + all modules can be stacked. SELinux, Smack and AppArmor are > >> > + known to be incompatible. User space components may > >> > + have trouble identifying the security module providing > >> > + data in some cases. > >> > + > >> > + If you select this option you will have to select which > >> > + of the stackable modules you wish to be active. The > >> > + "Default security module" will be ignored. The boot line > >> > + "security=" option can be used to specify that one of > >> > + the modules identifed for stacking should be used instead > >> > + of the entire stack. > >> > + > >> > + If you are unsure how to answer this question, answer N. > >> > >> I don't see a good reason to make this a config. Why shouldn't this > >> always be enabled? > > > > I do. From a user perspective it is sometimes difficult to determine > > the reason behind a failed operation; its is a DAC based deni
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 12:19 AM Kees Cook wrote: > On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler > wrote: > > Two proposed security modules require the ability to > > share security blobs with existing "major" security modules. > > These modules, S.A.R.A and LandLock, provide significantly > > different services than SELinux, Smack or AppArmor. Using > > either in conjunction with the existing modules is quite > > reasonable. S.A.R.A requires access to the cred blob, while > > LandLock uses the cred, file and inode blobs. > > > > The use of the cred, file and inode blobs has been > > abstracted in preceding patches in the series. This > > patch teaches the affected security modules how to access > > the part of the blob set aside for their use in the case > > where blobs are shared. The configuration option > > CONFIG_SECURITY_STACKING identifies systems where the > > blobs may be shared. > > > > The mechanism for selecting which security modules are > > active has been changed to allow non-conflicting "major" > > security modules to be used together. At this time the > > TOMOYO module can safely be used with any of the others. > > The two new modules would be non-conflicting as well. > > > > Signed-off-by: Casey Schaufler > > --- > > Documentation/admin-guide/LSM/index.rst | 14 +++-- > > include/linux/lsm_hooks.h | 2 +- > > security/Kconfig| 81 + > > security/apparmor/include/cred.h| 8 +++ > > security/apparmor/include/file.h| 9 ++- > > security/apparmor/include/lib.h | 4 ++ > > security/apparmor/lsm.c | 8 ++- > > security/security.c | 30 - > > security/selinux/hooks.c| 3 +- > > security/selinux/include/objsec.h | 18 +- > > security/smack/smack.h | 19 +- > > security/smack/smack_lsm.c | 17 +++--- > > security/tomoyo/common.h| 12 +++- > > security/tomoyo/tomoyo.c| 3 +- > > 14 files changed, 200 insertions(+), 28 deletions(-) ... > > diff --git a/security/Kconfig b/security/Kconfig > > index 22f7664c4977..ed48025ae9e0 100644 > > --- a/security/Kconfig > > +++ b/security/Kconfig > > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS > > bool > > default n > > > > +config SECURITY_STACKING > > + bool "Security module stacking" > > + depends on SECURITY > > + help > > + Allows multiple major security modules to be stacked. > > + Modules are invoked in the order registered with a > > + "bail on fail" policy, in which the infrastructure > > + will stop processing once a denial is detected. Not > > + all modules can be stacked. SELinux, Smack and AppArmor are > > + known to be incompatible. User space components may > > + have trouble identifying the security module providing > > + data in some cases. > > + > > + If you select this option you will have to select which > > + of the stackable modules you wish to be active. The > > + "Default security module" will be ignored. The boot line > > + "security=" option can be used to specify that one of > > + the modules identifed for stacking should be used instead > > + of the entire stack. > > + > > + If you are unsure how to answer this question, answer N. > > I don't see a good reason to make this a config. Why shouldn't this > always be enabled? I do. From a user perspective it is sometimes difficult to determine the reason behind a failed operation; its is a DAC based denial, the LSM, or some other failure? Stacking additional LSMs has the potential to make this worse. The boot time configuration adds to the complexity. I think we should leave this decision to the individual distros so that they can make their own decision on LSM stacking based on the savviness of their user base and the quality of their support infrastructure. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa wrote: > syzbot is hitting warning at str_read() [1] because len parameter can > become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > this case. > > [1] > https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > > Signed-off-by: Tetsuo Handa > Reported-by: syzbot > --- > security/selinux/ss/policydb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index e9394e7..f4eadd3 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, > u32 len) > if ((len == 0) || (len == (u32)-1)) > return -EINVAL; > > - str = kmalloc(len + 1, flags); > + str = kmalloc(len + 1, flags | __GFP_NOWARN); > if (!str) > return -ENOMEM; Thanks for the patch. My eyes are starting to glaze over a bit chasing down all of the different kmalloc() code paths trying to ensure that this always does the right thing based on size of the allocation and the different slab allocators ... are we sure that this will always return NULL when (len + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator configurations? -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 3/6] selinux: convert to kvmalloc
On Fri, Sep 7, 2018 at 1:50 PM Kent Overstreet wrote: > On Sat, Sep 08, 2018 at 02:08:03AM +0900, Tetsuo Handa wrote: > > On 2018/09/08 1:56, Kent Overstreet wrote: > > > @@ -329,8 +328,7 @@ int avtab_alloc(struct avtab *h, u32 nrules) > > > nslot = MAX_AVTAB_HASH_BUCKETS; > > > mask = nslot - 1; > > > > > > - h->htable = flex_array_alloc(sizeof(struct avtab_node *), nslot, > > > -GFP_KERNEL | __GFP_ZERO); > > > + h->htable = kvmalloc_array(nslot, sizeof(void *), GFP_KERNEL); > > > if (!h->htable) > > > return -ENOMEM; > > > > > > > kvmalloc_array() does not imply __GFP_ZERO. > > Thanks, fixed When you resubmit this patch, please make sure you submit it to the SELinux list (added to the To line above) so that it can be properly reviewed and merged. Thanks. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Fri, Aug 31, 2018 at 11:47 AM Jann Horn wrote: > On Thu, Aug 9, 2018 at 3:56 AM Paul Moore wrote: > > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn wrote: ... > > In the case where we have a MLS policy loaded (pol->mls_enabled != 0) > > and scontext is empty (scontext[0] = '\0'), we could end up returning > > 0 couldn't we? It seems like we might want a quick check for this > > before we parse the low/high portions of the field into the rangep > > array. > > I don't think so. In the first loop iteration, `sensitivity` will be > an empty string, and so the hashtab_search() should return NULL, > leading to -EINVAL. Am I missing something? Looking at this again, no, I think you've got it right. My guess is that I just mistook the NULL sensitivity check at the top of the loop as getting triggered in this case, which isn't the case here. Sorry for the noise. > > As an aside, I believe my other comments on this patch still stand. > > It's a nice improvement but I think there are some other small things > > that need to be addressed. > > Is there anything I need to fix apart from the overly verbose comment > and the unnecessary curly braces? Nope. I wouldn't even bother with that brace/comment changes, those were minor nits and only worth changing if you needed to respin the patch for some other reason. Consider the patch merged, thanks! -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: fix mounting of cgroup2 under older policies
On Tue, Sep 4, 2018 at 6:18 PM Paul Moore wrote:
> On Tue, Sep 4, 2018 at 4:49 PM Stephen Smalley wrote:
> >
> > commit 901ef845fa2469c ("selinux: allow per-file labeling for cgroupfs")
> > broke mounting of cgroup2 under older SELinux policies which lacked
> > a genfscon rule for cgroup2. This prevents mounting of cgroup2 even
> > when SELinux is permissive.
> >
> > Change the handling when there is no genfscon rule in policy to
> > just mark the inode unlabeled and not return an error to the caller.
> > This permits mounting and access if allowed by policy, e.g. to
> > unconfined domains.
> >
> > I also considered changing the behavior of security_genfs_sid() to
> > never return -ENOENT, but the current behavior is relied upon by
> > other callers to perform caller-specific handling.
> >
> > Fixes: 901ef845fa2469c ("selinux: allow per-file labeling for cgroupfs")
> > CC:
> > Reported-by: Dmitry Vyukov
> > Reported-by: Waiman Long
> > Signed-off-by: Stephen Smalley
> > ---
> > security/selinux/hooks.c | 5 +
> > 1 file changed, 5 insertions(+)
>
> Looks like a reasonable approach to me, merged into selinux/next, thanks.
I probably should expand a bit on this as Stephen's stable CC marking
and the patch's inclusion in selinux/next (as opposed to
selinux/stable-4.19) don't quite match. I merged this into the next
branch because I didn't feel that this was a severe enough problem to
warrant immediate inclusion into the stable-4.19 branch and since this
is a user visible change I felt some additional time in the next
branch would be valuable. However, I did leave the CC stable marking
intact so that when this patch does hit Linus tree (expected during
the v4.20 merge window) it should get backported to the various stable
trees.
> As a FYI, since the US holiday and LSS-NA delayed the start of merging
> things into selinux/next I've updated selinux/next on top of v4.19-rc2
> instead of -rc1 this time around.
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f78318af8254..58fee382a3bb 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1508,6 +1508,11 @@ static int selinux_genfs_get_sid(struct dentry
> > *dentry,
> > }
> > rc = security_genfs_sid(_state, sb->s_type->name,
> > path, tclass, sid);
> > + if (rc == -ENOENT) {
> > + /* No match in policy, mark as unlabeled. */
> > + *sid = SECINITSID_UNLABELED;
> > + rc = 0;
> > + }
> > }
> > free_page((unsigned long)buffer);
> > return rc;
> > --
> > 2.14.4
>
> --
> paul moore
> www.paul-moore.com
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: fix mounting of cgroup2 under older policies
On Tue, Sep 4, 2018 at 4:49 PM Stephen Smalley wrote:
>
> commit 901ef845fa2469c ("selinux: allow per-file labeling for cgroupfs")
> broke mounting of cgroup2 under older SELinux policies which lacked
> a genfscon rule for cgroup2. This prevents mounting of cgroup2 even
> when SELinux is permissive.
>
> Change the handling when there is no genfscon rule in policy to
> just mark the inode unlabeled and not return an error to the caller.
> This permits mounting and access if allowed by policy, e.g. to
> unconfined domains.
>
> I also considered changing the behavior of security_genfs_sid() to
> never return -ENOENT, but the current behavior is relied upon by
> other callers to perform caller-specific handling.
>
> Fixes: 901ef845fa2469c ("selinux: allow per-file labeling for cgroupfs")
> CC:
> Reported-by: Dmitry Vyukov
> Reported-by: Waiman Long
> Signed-off-by: Stephen Smalley
> ---
> security/selinux/hooks.c | 5 +
> 1 file changed, 5 insertions(+)
Looks like a reasonable approach to me, merged into selinux/next, thanks.
As a FYI, since the US holiday and LSS-NA delayed the start of merging
things into selinux/next I've updated selinux/next on top of v4.19-rc2
instead of -rc1 this time around.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f78318af8254..58fee382a3bb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1508,6 +1508,11 @@ static int selinux_genfs_get_sid(struct dentry *dentry,
> }
> rc = security_genfs_sid(_state, sb->s_type->name,
> path, tclass, sid);
> + if (rc == -ENOENT) {
> + /* No match in policy, mark as unlabeled. */
> + *sid = SECINITSID_UNLABELED;
> + rc = 0;
> + }
> }
> free_page((unsigned long)buffer);
> return rc;
> --
> 2.14.4
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] SELinux: allow other LSMs to use custom mount args
On Tue, Aug 28, 2018 at 5:32 PM Micah Morton wrote:
> The security_sb_copy_data LSM hook allows LSMs to copy custom string
> name/value args passed to mount_fs() into a temporary buffer (called
> "secdata") that will be accessible to LSM code during the
> security_sb_kern_mount hook further down in mount_fs(). Currently,
> SELinux effectively prevents any other LSMs from copying custom mount
> args into the temporary buffer (and being able to access them during
> security_sb_kern_mount), as it will fail with -EINVAL and print
> "SELinux: unknown mount option" to the kernel message buffer if args it
> doesn't recognize are present in the temporary buffer when
> selinux_sb_kern_mount is called. This change adds an arg to the list of
> those accepted by SELinux during security_sb_kern_mount. SELinux won't
> do anything with this arg besides allow the name/value pair to be passed
> along to any other LSM that is stacked after SELinux.
>
> Developed on v4.18.
>
> Signed-off-by: Micah Morton
> ---
> security/selinux/hooks.c| 7 ++-
> security/selinux/include/security.h | 11 ++-
> 2 files changed, 12 insertions(+), 6 deletions(-)
SELinux patches need to be sent to the SELinux mailing list (CC'd) for
proper review.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2b5ee5fbd652..e70ccc701eb8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -445,6 +445,7 @@ enum {
> Opt_rootcontext = 4,
> Opt_labelsupport = 5,
> Opt_nextmntopt = 6,
> + Opt_lsm_custom_arg = 7,
> };
>
> #define NUM_SEL_MNT_OPTS (Opt_nextmntopt - 1)
> @@ -455,6 +456,7 @@ static const match_table_t tokens = {
> {Opt_defcontext, DEFCONTEXT_STR "%s"},
> {Opt_rootcontext, ROOTCONTEXT_STR "%s"},
> {Opt_labelsupport, LABELSUPP_STR},
> + {Opt_lsm_custom_arg, LSM_CUSTOM_ARG_STR "%s"},
> {Opt_error, NULL},
> };
>
> @@ -1156,6 +1158,8 @@ static int selinux_parse_opts_str(char *options,
> break;
> case Opt_labelsupport:
> break;
> + case Opt_lsm_custom_arg:
> + break;
> default:
> rc = -EINVAL;
> printk(KERN_WARNING "SELinux: unknown mount
> option\n");
> @@ -2758,7 +2762,8 @@ static inline int selinux_option(char *option, int len)
> match_prefix(FSCONTEXT_STR, sizeof(FSCONTEXT_STR)-1, option,
> len) ||
> match_prefix(DEFCONTEXT_STR, sizeof(DEFCONTEXT_STR)-1,
> option, len) ||
> match_prefix(ROOTCONTEXT_STR, sizeof(ROOTCONTEXT_STR)-1,
> option, len) ||
> - match_prefix(LABELSUPP_STR, sizeof(LABELSUPP_STR)-1, option,
> len));
> + match_prefix(LABELSUPP_STR, sizeof(LABELSUPP_STR)-1, option,
> len) ||
> + match_prefix(LSM_CUSTOM_ARG_STR,
> sizeof(LSM_CUSTOM_ARG_STR)-1, option, len));
> }
>
> static inline void take_option(char **to, char *from, int *first, int len)
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h
> index 23e762d529fa..0ead836a0625 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -59,11 +59,12 @@
> #define SE_SBPROC 0x0200
> #define SE_SBGENFS 0x0400
>
> -#define CONTEXT_STR"context="
> -#define FSCONTEXT_STR "fscontext="
> -#define ROOTCONTEXT_STR"rootcontext="
> -#define DEFCONTEXT_STR "defcontext="
> -#define LABELSUPP_STR "seclabel"
> +#define CONTEXT_STR "context="
> +#define FSCONTEXT_STR "fscontext="
> +#define ROOTCONTEXT_STR "rootcontext="
> +#define DEFCONTEXT_STR "defcontext="
> +#define LABELSUPP_STR "seclabel"
> +#define LSM_CUSTOM_ARG_STR "lsm_custom_arg="
>
> struct netlbl_lsm_secattr;
>
> --
> 2.19.0.rc0.228.g281dcd1b4d0-goog
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[GIT PULL] SELinux patches for v4.19
Hi Linus, There are 16 patches in the SELinux PR for v4.19 but really only one that is of any significance. That one patch is by nixiaoming and fixes a few places where we were not properly cleaning up dentry and inode objects in the selinuxfs error handling code. The rest are either printk->pr_* conversions, constification tweaks, and a minor tweak to MAINTAINERS. Everything passes the selinux-testsuite and looks to merge cleanly against your master branch. Please pull, thanks. -Paul -- The following changes since commit ce397d215ccd07b8ae3f71db689aedb85d56ab40: Linux 4.18-rc1 (2018-06-17 08:04:49 +0900) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20180814 for you to fetch changes up to 7e4237faa7213c1cc1d0aa65a44c67ba4729ce9f: selinux: cleanup dentry and inodes on error in selinuxfs (2018-08-07 17:26:25 -0400) selinux/stable-4.18 PR 20180814 Eric Biggers (1): selinux: constify write_op[] Paul Moore (1): MAINTAINERS: update the LSM and SELinux subsystems nixiaoming (1): selinux: cleanup dentry and inodes on error in selinuxfs peter enderborg (13): selinux: Cleanup printk logging in conditional selinux: Cleanup printk logging in ebitmap selinux: Cleanup printk logging in policydb selinux: Cleanup printk logging in hooks selinux: Cleanup printk logging in avtab selinux: Cleanup printk logging in services selinux: Cleanup printk logging in selinuxfs selinux: Cleanup printk logging in netlink selinux: Cleanup printk logging in sidtab selinux: Cleanup printk logging in netport selinux: Cleanup printk logging in netif selinux: Cleanup printk logging in avc selinux: Cleanup printk logging in netnode MAINTAINERS | 1 + security/selinux/avc.c| 2 +- security/selinux/hooks.c | 68 ++--- security/selinux/netif.c | 11 ++--- security/selinux/netlink.c| 2 +- security/selinux/netnode.c| 5 +-- security/selinux/netport.c| 5 +-- security/selinux/selinuxfs.c | 45 +-- security/selinux/ss/avtab.c | 51 +++--- security/selinux/ss/conditional.c | 16 +++ security/selinux/ss/ebitmap.c | 15 +++ security/selinux/ss/policydb.c| 91 ++- security/selinux/ss/services.c| 71 +++--- security/selinux/ss/sidtab.c | 5 +-- 14 files changed, 199 insertions(+), 189 deletions(-) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Fri, Aug 10, 2018 at 7:01 PM Jann Horn wrote:
> On Thu, Aug 9, 2018 at 4:07 AM Paul Moore wrote:
> > On Wed, Aug 8, 2018 at 9:56 PM Paul Moore wrote:
> > > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn wrote:
> > > >
> > > > The intended behavior change for this patch is to reject any MLS strings
> > > > that contain (trailing) garbage if p->mls_enabled is true.
> > > >
> > > > As suggested by Paul Moore, change mls_context_to_sid() so that the two
> > > > parts of the range are extracted before the rest of the parsing. Because
> > > > now we don't have to scan for two different separators simultaneously
> > > > everywhere, we can actually switch to strchr() everywhere instead of the
> > > > open-coded loops that scan for two separators at once.
> > > >
> > > > mls_context_to_sid() used to signal how much of the input string was
> > > > parsed
> > > > by updating `*scontext`. However, there is actually no case in which
> > > > mls_context_to_sid() only parses a subset of the input and still returns
> > > > a success (other than the buggy case with a second '-' in which it
> > > > incorrectly claims to have consumed the entire string). Turn `scontext`
> > > > into a simple pointer argument and stop redundantly checking whether the
> > > > entire input was consumed in string_to_context_struct(). This also lets
> > > > us
> > > > remove the `scontext_len` argument from `string_to_context_struct()`.
> > > >
> > > > Signed-off-by: Jann Horn
> > > > ---
> > > > Refactored version of
> > > > "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> > > > Paul's comments. WDYT?
> > > > I've thrown some inputs at it, and it seems to work.
> > > >
> > > > security/selinux/ss/mls.c | 178 ++---
> > > > security/selinux/ss/mls.h | 2 +-
> > > > security/selinux/ss/services.c | 12 +--
> > > > 3 files changed, 82 insertions(+), 110 deletions(-)
> > >
> > > Thanks for the rework, this looks much better than what we currently
> > > have. I have some comments/questions below ...
> > >
> > > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > > > index 39475fb455bc..2fe459df3c85 100644
> > > > --- a/security/selinux/ss/mls.c
> > > > +++ b/security/selinux/ss/mls.c
> > > > @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct
> > > > context *c)
> > > > /*
> > > > * Set the MLS fields in the security context structure
> > > > * `context' based on the string representation in
> > > > - * the string `*scontext'. Update `*scontext' to
> > > > - * point to the end of the string representation of
> > > > - * the MLS fields.
> > > > + * the string `scontext'.
> > > > *
> > > > * This function modifies the string in place, inserting
> > > > * NULL characters to terminate the MLS fields.
> > > > @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p,
> > > > struct context *c)
> > > > */
> > > > int mls_context_to_sid(struct policydb *pol,
> > > >char oldc,
> > > > - char **scontext,
> > > > + char *scontext,
> > > >struct context *context,
> > > >struct sidtab *s,
> > > >u32 def_sid)
> > > > {
> > > > -
> > > > - char delim;
> > > > - char *scontextp, *p, *rngptr;
> > > > + char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > > > struct level_datum *levdatum;
> > > > struct cat_datum *catdatum, *rngdatum;
> > > > - int l, rc = -EINVAL;
> > > > + int l, rc, i;
> > > > + char *rangep[2];
> > > >
> > > > if (!pol->mls_enabled) {
> > > > - if (def_sid != SECSID_NULL && oldc)
> > > > - *scontext += strlen(*scontext) + 1;
> > > > - return 0;
> > > > + if ((def_sid != SECSID_NULL && oldc) || (*scontext) ==
> > > > '\0')
> > > > + return 0;
> > &g
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Wed, Aug 8, 2018 at 9:56 PM Paul Moore wrote:
>
> On Mon, Aug 6, 2018 at 5:19 PM Jann Horn wrote:
> >
> > The intended behavior change for this patch is to reject any MLS strings
> > that contain (trailing) garbage if p->mls_enabled is true.
> >
> > As suggested by Paul Moore, change mls_context_to_sid() so that the two
> > parts of the range are extracted before the rest of the parsing. Because
> > now we don't have to scan for two different separators simultaneously
> > everywhere, we can actually switch to strchr() everywhere instead of the
> > open-coded loops that scan for two separators at once.
> >
> > mls_context_to_sid() used to signal how much of the input string was parsed
> > by updating `*scontext`. However, there is actually no case in which
> > mls_context_to_sid() only parses a subset of the input and still returns
> > a success (other than the buggy case with a second '-' in which it
> > incorrectly claims to have consumed the entire string). Turn `scontext`
> > into a simple pointer argument and stop redundantly checking whether the
> > entire input was consumed in string_to_context_struct(). This also lets us
> > remove the `scontext_len` argument from `string_to_context_struct()`.
> >
> > Signed-off-by: Jann Horn
> > ---
> > Refactored version of
> > "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> > Paul's comments. WDYT?
> > I've thrown some inputs at it, and it seems to work.
> >
> > security/selinux/ss/mls.c | 178 ++---
> > security/selinux/ss/mls.h | 2 +-
> > security/selinux/ss/services.c | 12 +--
> > 3 files changed, 82 insertions(+), 110 deletions(-)
>
> Thanks for the rework, this looks much better than what we currently
> have. I have some comments/questions below ...
>
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index 39475fb455bc..2fe459df3c85 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct
> > context *c)
> > /*
> > * Set the MLS fields in the security context structure
> > * `context' based on the string representation in
> > - * the string `*scontext'. Update `*scontext' to
> > - * point to the end of the string representation of
> > - * the MLS fields.
> > + * the string `scontext'.
> > *
> > * This function modifies the string in place, inserting
> > * NULL characters to terminate the MLS fields.
> > @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct
> > context *c)
> > */
> > int mls_context_to_sid(struct policydb *pol,
> >char oldc,
> > - char **scontext,
> > + char *scontext,
> >struct context *context,
> >struct sidtab *s,
> >u32 def_sid)
> > {
> > -
> > - char delim;
> > - char *scontextp, *p, *rngptr;
> > + char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > struct level_datum *levdatum;
> > struct cat_datum *catdatum, *rngdatum;
> > - int l, rc = -EINVAL;
> > + int l, rc, i;
> > + char *rangep[2];
> >
> > if (!pol->mls_enabled) {
> > - if (def_sid != SECSID_NULL && oldc)
> > - *scontext += strlen(*scontext) + 1;
> > - return 0;
> > + if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > + return 0;
> > + return -EINVAL;
>
> Why are we simply not always return 0 in the case where MLS is not
> enabled in the policy? The mls_context_to_sid() is pretty much a
> no-op in this case (even more so with your pat regardless of input and
> I worry that returning EINVAL here is a deviation from current
> behavior which could cause problems.
Sorry, I was rephrasing the text above when I accidentally hit send.
While my emails are generally a good source of typos, the above is
pretty bad, so let me try again ...
Why are we simply not always returning 0 in the case where MLS is not
enabled in the policy? The mls_context_to_sid() function is pretty
much a no-op in this case regardless of input and I worry that
returning EINVAL here is a deviation from current behavior which could
cause problems.
> > }
> >
> > /*
> > @@ -261,113 +258,94 @@ int mls_conte
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Mon, Aug 6, 2018 at 5:19 PM Jann Horn wrote:
>
> The intended behavior change for this patch is to reject any MLS strings
> that contain (trailing) garbage if p->mls_enabled is true.
>
> As suggested by Paul Moore, change mls_context_to_sid() so that the two
> parts of the range are extracted before the rest of the parsing. Because
> now we don't have to scan for two different separators simultaneously
> everywhere, we can actually switch to strchr() everywhere instead of the
> open-coded loops that scan for two separators at once.
>
> mls_context_to_sid() used to signal how much of the input string was parsed
> by updating `*scontext`. However, there is actually no case in which
> mls_context_to_sid() only parses a subset of the input and still returns
> a success (other than the buggy case with a second '-' in which it
> incorrectly claims to have consumed the entire string). Turn `scontext`
> into a simple pointer argument and stop redundantly checking whether the
> entire input was consumed in string_to_context_struct(). This also lets us
> remove the `scontext_len` argument from `string_to_context_struct()`.
>
> Signed-off-by: Jann Horn
> ---
> Refactored version of
> "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> Paul's comments. WDYT?
> I've thrown some inputs at it, and it seems to work.
>
> security/selinux/ss/mls.c | 178 ++---
> security/selinux/ss/mls.h | 2 +-
> security/selinux/ss/services.c | 12 +--
> 3 files changed, 82 insertions(+), 110 deletions(-)
Thanks for the rework, this looks much better than what we currently
have. I have some comments/questions below ...
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 39475fb455bc..2fe459df3c85 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct
> context *c)
> /*
> * Set the MLS fields in the security context structure
> * `context' based on the string representation in
> - * the string `*scontext'. Update `*scontext' to
> - * point to the end of the string representation of
> - * the MLS fields.
> + * the string `scontext'.
> *
> * This function modifies the string in place, inserting
> * NULL characters to terminate the MLS fields.
> @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct
> context *c)
> */
> int mls_context_to_sid(struct policydb *pol,
>char oldc,
> - char **scontext,
> + char *scontext,
>struct context *context,
>struct sidtab *s,
>u32 def_sid)
> {
> -
> - char delim;
> - char *scontextp, *p, *rngptr;
> + char *sensitivity, *cur_cat, *next_cat, *rngptr;
> struct level_datum *levdatum;
> struct cat_datum *catdatum, *rngdatum;
> - int l, rc = -EINVAL;
> + int l, rc, i;
> + char *rangep[2];
>
> if (!pol->mls_enabled) {
> - if (def_sid != SECSID_NULL && oldc)
> - *scontext += strlen(*scontext) + 1;
> - return 0;
> + if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> + return 0;
> + return -EINVAL;
Why are we simply not always return 0 in the case where MLS is not
enabled in the policy? The mls_context_to_sid() is pretty much a
no-op in this case (even more so with your pat regardless of input and
I worry that returning EINVAL here is a deviation from current
behavior which could cause problems.
> }
>
> /*
> @@ -261,113 +258,94 @@ int mls_context_to_sid(struct policydb *pol,
> struct context *defcon;
>
> if (def_sid == SECSID_NULL)
> - goto out;
> + return -EINVAL;
>
> defcon = sidtab_search(s, def_sid);
> if (!defcon)
> - goto out;
> + return -EINVAL;
>
> - rc = mls_context_cpy(context, defcon);
> - goto out;
> + return mls_context_cpy(context, defcon);
> }
>
> - /* Extract low sensitivity. */
> - scontextp = p = *scontext;
> - while (*p && *p != ':' && *p != '-')
> - p++;
> -
> - delim = *p;
> - if (delim != '\0')
> - *p++ = '\0';
> + /*
> +* If we're dealing with a range, figure out where the two parts
> +* of the range
Re: [PATCH] selinuxfs: Fix the resource leak in the failed branch of sel_make_inode
On Tue, Aug 7, 2018 at 5:35 PM Paul Moore wrote:
>
> On Sun, Aug 5, 2018 at 5:48 AM nixiaoming wrote:
> > If the resource requested by d_alloc_name is not added to the linked
> > list through d_add, then dput needs to be called to release the
> > subsequent abnormal branch to avoid resource leakage.
> >
> > Add missing dput to selinuxfs.c
> >
> > Signed-off-by: nixiaoming
> > ---
> > security/selinux/selinuxfs.c | 33 +
> > 1 file changed, 25 insertions(+), 8 deletions(-)
>
> Thanks for the quick follow-up on this patch. It looks okay to me,
> assuming my test kernel works correctly (it's building now) I'll merge
> this into selinux/next.
Merged into selinux/next, thanks!
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 79d3709..0b66d728 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -1365,13 +1365,18 @@ static int sel_make_bools(struct selinux_fs_info
> > *fsi)
> >
> > ret = -ENOMEM;
> > inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO |
> > S_IWUSR);
> > - if (!inode)
> > + if (!inode) {
> > + dput(dentry);
> > goto out;
> > + }
> >
> > ret = -ENAMETOOLONG;
> > len = snprintf(page, PAGE_SIZE, "/%s/%s", BOOL_DIR_NAME,
> > names[i]);
> > - if (len >= PAGE_SIZE)
> > + if (len >= PAGE_SIZE) {
> > + dput(dentry);
> > + iput(inode);
> > goto out;
> > + }
> >
> > isec = (struct inode_security_struct *)inode->i_security;
> > ret = security_genfs_sid(fsi->state, "selinuxfs", page,
> > @@ -1586,8 +1591,10 @@ static int sel_make_avc_files(struct dentry *dir)
> > return -ENOMEM;
> >
> > inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode);
> > - if (!inode)
> > + if (!inode) {
> > + dput(dentry);
> > return -ENOMEM;
> > + }
> >
> > inode->i_fop = files[i].ops;
> > inode->i_ino = ++fsi->last_ino;
> > @@ -1632,8 +1639,10 @@ static int sel_make_initcon_files(struct dentry *dir)
> > return -ENOMEM;
> >
> > inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> > - if (!inode)
> > + if (!inode) {
> > + dput(dentry);
> > return -ENOMEM;
> > + }
> >
> > inode->i_fop = _initcon_ops;
> > inode->i_ino = i|SEL_INITCON_INO_OFFSET;
> > @@ -1733,8 +1742,10 @@ static int sel_make_perm_files(char *objclass, int
> > classvalue,
> >
> > rc = -ENOMEM;
> > inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> > - if (!inode)
> > + if (!inode) {
> > + dput(dentry);
> > goto out;
> > + }
> >
> > inode->i_fop = _perm_ops;
> > /* i+1 since perm values are 1-indexed */
> > @@ -1763,8 +1774,10 @@ static int sel_make_class_dir_entries(char
> > *classname, int index,
> > return -ENOMEM;
> >
> > inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> > - if (!inode)
> > + if (!inode) {
> > + dput(dentry);
> > return -ENOMEM;
> > + }
> >
> > inode->i_fop = _class_ops;
> > inode->i_ino = sel_class_to_ino(index);
> > @@ -1838,8 +1851,10 @@ static int sel_make_policycap(struct selinux_fs_info
> > *fsi)
> > return -ENOMEM;
> >
> > inode = sel_make_inode(fsi->sb, S_IFREG | 0444);
> > - if (inode == NULL)
> > + if (inode == NULL) {
> > + dput(dentry);
> > return -ENOMEM;
> > + }
> >
> > inode->i_fop = _policycap_ops;
> > inode->i_ino = iter | SEL_POLICYCAP_INO_OFFSET;
> > @@ -1932,8 +1947,10 @@ static int sel_fill_super(struct s
Re: [PATCH] selinuxfs: Fix the resource leak in the failed branch of sel_make_inode
On Sun, Aug 5, 2018 at 5:48 AM nixiaoming wrote:
> If the resource requested by d_alloc_name is not added to the linked
> list through d_add, then dput needs to be called to release the
> subsequent abnormal branch to avoid resource leakage.
>
> Add missing dput to selinuxfs.c
>
> Signed-off-by: nixiaoming
> ---
> security/selinux/selinuxfs.c | 33 +
> 1 file changed, 25 insertions(+), 8 deletions(-)
Thanks for the quick follow-up on this patch. It looks okay to me,
assuming my test kernel works correctly (it's building now) I'll merge
this into selinux/next.
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 79d3709..0b66d728 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1365,13 +1365,18 @@ static int sel_make_bools(struct selinux_fs_info *fsi)
>
> ret = -ENOMEM;
> inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO |
> S_IWUSR);
> - if (!inode)
> + if (!inode) {
> + dput(dentry);
> goto out;
> + }
>
> ret = -ENAMETOOLONG;
> len = snprintf(page, PAGE_SIZE, "/%s/%s", BOOL_DIR_NAME,
> names[i]);
> - if (len >= PAGE_SIZE)
> + if (len >= PAGE_SIZE) {
> + dput(dentry);
> + iput(inode);
> goto out;
> + }
>
> isec = (struct inode_security_struct *)inode->i_security;
> ret = security_genfs_sid(fsi->state, "selinuxfs", page,
> @@ -1586,8 +1591,10 @@ static int sel_make_avc_files(struct dentry *dir)
> return -ENOMEM;
>
> inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode);
> - if (!inode)
> + if (!inode) {
> + dput(dentry);
> return -ENOMEM;
> + }
>
> inode->i_fop = files[i].ops;
> inode->i_ino = ++fsi->last_ino;
> @@ -1632,8 +1639,10 @@ static int sel_make_initcon_files(struct dentry *dir)
> return -ENOMEM;
>
> inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> - if (!inode)
> + if (!inode) {
> + dput(dentry);
> return -ENOMEM;
> + }
>
> inode->i_fop = _initcon_ops;
> inode->i_ino = i|SEL_INITCON_INO_OFFSET;
> @@ -1733,8 +1742,10 @@ static int sel_make_perm_files(char *objclass, int
> classvalue,
>
> rc = -ENOMEM;
> inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> - if (!inode)
> + if (!inode) {
> + dput(dentry);
> goto out;
> + }
>
> inode->i_fop = _perm_ops;
> /* i+1 since perm values are 1-indexed */
> @@ -1763,8 +1774,10 @@ static int sel_make_class_dir_entries(char *classname,
> int index,
> return -ENOMEM;
>
> inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> - if (!inode)
> + if (!inode) {
> + dput(dentry);
> return -ENOMEM;
> + }
>
> inode->i_fop = _class_ops;
> inode->i_ino = sel_class_to_ino(index);
> @@ -1838,8 +1851,10 @@ static int sel_make_policycap(struct selinux_fs_info
> *fsi)
> return -ENOMEM;
>
> inode = sel_make_inode(fsi->sb, S_IFREG | 0444);
> - if (inode == NULL)
> + if (inode == NULL) {
> + dput(dentry);
> return -ENOMEM;
> + }
>
> inode->i_fop = _policycap_ops;
> inode->i_ino = iter | SEL_POLICYCAP_INO_OFFSET;
> @@ -1932,8 +1947,10 @@ static int sel_fill_super(struct super_block *sb, void
> *data, int silent)
>
> ret = -ENOMEM;
> inode = sel_make_inode(sb, S_IFCHR | S_IRUGO | S_IWUGO);
> - if (!inode)
> + if (!inode) {
> + dput(dentry);
> goto err;
> + }
>
> inode->i_ino = ++fsi->last_ino;
> isec = (struct inode_security_struct *)inode->i_security;
> --
> 2.10.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: stricter parsing in mls_context_to_sid()
On Fri, Aug 3, 2018 at 5:36 AM Jann Horn wrote:
>
> mls_context_to_sid incorrectly accepted MLS context strings that are
> followed by a dash and trailing garbage.
>
> Before this change, the following command works:
>
> # mount -t tmpfs -o 'context=system_u:object_r:tmp_t:s0-s0:c0-BLAH' \
> none mount
>
> After this change, it fails with the following error message in dmesg:
>
> SELinux: security_context_str_to_sid(system_u:object_r:tmp_t:s0-s0:c0-BLAH)
> failed for (dev tmpfs, type tmpfs) errno=-22
>
> This is not an important bug; but it is a small quirk that was useful for
> exploiting a vulnerability in fusermount.
>
> This patch does not change the behavior when the policy does not have MLS
> enabled.
>
> Signed-off-by: Jann Horn
> ---
> security/selinux/ss/mls.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Ooof. mls_context_to_sid() isn't exactly the most elegant function is
it? I suppose some of that is due to the label format, but it still
seems like we can do better. However, before I jump into that let me
say that speaking strictly about your patch, yes, it does look correct
to me.
What I'm wonder is if we rework/reorder some of the parser to remove
some of the ordering specific (e.g. "l == 0") and reduce some
redundancy ... be patient with me for a moment ...
The while loops immediately following the "Extract low sensitivity"
and "Extract high sensitivity" comments are basically the same,
including NULL'ing the delimiter if necessary, the only difference is
the additional '-' delimiter in the low sensitivity search.
The only *legal* place for a '-' in the MLS portion of the label is to
separate the low/high portions.
What if we were to do a quick search for the low/high separator before
extracting the low sensitivity and stored low/high pointers for later
use? e.g.
rangep[0] = *scontext;
rangep[1] = strchr(rangep[0], '-');
if (rangep[1])
rangep[1]++ = '\0';
... we could then move the "Extract X sensitivity" into the main for
loop as well remove all of the '-' special case parsing checks, e.g.
for (l = 0; l < 2; l++) {
scontextp = rangep[l];
if (!scontextp)
break;
while (*p && *p != ':')
p++;
delim = *p;
if (delim != '\0')
*p++ = '\0';
/* extract the level (use existing code) */
/* extract the category set, if present (use existing code) */
/* no need to worry about the '-' delimiter */
}
I *believe* that should work. I think. Does that make sense?
Yes, I do understand that this likely isn't quite as performant as the
existing approach due to that initial strchr(), but I think the code
will be much cleaner and less fragile. If we feel the need to claw
back some performance there are some other things in here would could
probably work on (e.g. imagine an ebitmap_set_range()).
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 39475fb455bc..2c73d612d2ee 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -344,7 +344,7 @@ int mls_context_to_sid(struct policydb *pol,
> break;
> }
> }
> - if (delim == '-') {
> + if (delim == '-' && l == 0) {
> /* Extract high sensitivity. */
> scontextp = p;
> while (*p && *p != ':')
> --
> 2.18.0.597.ga71716f1ad-goog
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: maybe resource leak in security/selinux/selinuxfs.c
On Wed, Aug 1, 2018 at 5:39 AM Nixiaoming wrote:
>
> advisory:
> 1 After creating dentry in d_alloc_name, should I call dput to release
> resources before the exception exit?
> 2 After calling the new_inode to create an inode, should the inode resource
> be released before the exception exit?
>
> If the dentry and inode resources need to be actively released, there are
> multiple resource leaks in security/selinux/selinuxfs.c.
Hello. Sorry for the delay in responding, comments inline ...
> Example:
> Linux master branch v4.18-rc5
> The function sel_make_avc_files in security/selinux/selinuxfs.c.
>
> 1566 static int sel_make_avc_files(struct dentry *dir)
> ...
> 1580 for (i = 0; i < ARRAY_SIZE(files); i++) {
> 1581 struct inode *inode;
> 1582 struct dentry *dentry;
> 1583
> 1584 dentry = d_alloc_name(dir, files[i].name);
> 1585 if (!dentry)
> /*Resource leak: when i!=0, the release action of dentry and inode resources
> is missing*/
I don't understand the leak in this case? If d_alloc_name() fails on
a partially constructed /sys/fs/selinux/avc directory the previously
allocated dentry/inodes are not lost or leaked, d_alloc_name()
attached them to the parent dentry.
> 1586 return -ENOMEM;
> 1587
> 1588 inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode);
> 1589 if (!inode)
> /*Resource leak: missing dput(dentry)*/
> /*Resource leak: when i!=0, the release action of dentry and inode resources
> is missing*/
Yes, in this case it does look like we are missing a call to dput().
Would you like to submit a patch to fix this as well as the others in
selinuxfs.c?
> 1590 return -ENOMEM;
> 1591
> 1592 inode->i_fop = files[i].ops;
> 1593 inode->i_ino = ++fsi->last_ino;
> 1594 d_add(dentry, inode);
> 1595 }
> 1596
> 1597 return 0;
> 1598 }
>
> There are similar resource leaking functions:
> Sel_make_bools
> Sel_make_avc_files
> Sel_make_initcon_files
> Sel_make_perm_files
> Sel_make_class_dir_entries
> Sel_make_policycap
> Sel_fill_super
> Sel_make_policy_nodes
> Sel_make_classes
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: constify write_op[]
On Tue, Jul 17, 2018 at 1:45 PM Eric Biggers wrote:
>
> write_op[] is never modified, so make it 'const'.
>
> Signed-off-by: Eric Biggers
> ---
> security/selinux/selinuxfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged, thanks.
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index ab77da649b77..fb25396d2966 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -767,7 +767,7 @@ static ssize_t sel_write_relabel(struct file *file, char
> *buf, size_t size);
> static ssize_t sel_write_user(struct file *file, char *buf, size_t size);
> static ssize_t sel_write_member(struct file *file, char *buf, size_t size);
>
> -static ssize_t (*write_op[])(struct file *, char *, size_t) = {
> +static ssize_t (*const write_op[])(struct file *, char *, size_t) = {
> [SEL_ACCESS] = sel_write_access,
> [SEL_CREATE] = sel_write_create,
> [SEL_RELABEL] = sel_write_relabel,
> --
> 2.18.0.203.gfac676dfb9-goog
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[RFC PATCH v3] ipv6: make ipv6_renew_options() interrupt/kernel safe
From: Paul Moore
At present the ipv6_renew_options_kern() function ends up calling into
access_ok() which is problematic if done from inside an interrupt as
access_ok() calls WARN_ON_IN_IRQ() on some (all?) architectures
(x86-64 is affected). Example warning/backtrace is shown below:
WARNING: CPU: 1 PID: 3144 at lib/usercopy.c:11 _copy_from_user+0x85/0x90
...
Call Trace:
ipv6_renew_option+0xb2/0xf0
ipv6_renew_options+0x26a/0x340
ipv6_renew_options_kern+0x2c/0x40
calipso_req_setattr+0x72/0xe0
netlbl_req_setattr+0x126/0x1b0
selinux_netlbl_inet_conn_request+0x80/0x100
selinux_inet_conn_request+0x6d/0xb0
security_inet_conn_request+0x32/0x50
tcp_conn_request+0x35f/0xe00
? __lock_acquire+0x250/0x16c0
? selinux_socket_sock_rcv_skb+0x1ae/0x210
? tcp_rcv_state_process+0x289/0x106b
tcp_rcv_state_process+0x289/0x106b
? tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_rcv+0xc82/0xcf0
ip6_input_finish+0x10d/0x690
ip6_input+0x45/0x1e0
? ip6_rcv_finish+0x1d0/0x1d0
ipv6_rcv+0x32b/0x880
? ip6_make_skb+0x1e0/0x1e0
__netif_receive_skb_core+0x6f2/0xdf0
? process_backlog+0x85/0x250
? process_backlog+0x85/0x250
? process_backlog+0xec/0x250
process_backlog+0xec/0x250
net_rx_action+0x153/0x480
__do_softirq+0xd9/0x4f7
do_softirq_own_stack+0x2a/0x40
...
While not present in the backtrace, ipv6_renew_option() ends up calling
access_ok() via the following chain:
access_ok()
_copy_from_user()
copy_from_user()
ipv6_renew_option()
The fix presented in this patch is to perform the userspace copy
earlier in the call chain such that it is only called when the option
data is actually coming from userspace; that place is
do_ipv6_setsockopt(). Not only does this solve the problem seen in
the backtrace above, it also allows us to simplify the code quite a
bit by removing ipv6_renew_options_kern() completely. We also take
this opportunity to cleanup ipv6_renew_options()/ipv6_renew_option()
a small amount as well.
This patch is heavily based on a rough patch by Al Viro. I've taken
his original patch, converted a kmemdup() call in do_ipv6_setsockopt()
to a memdup_user() call, made better use of the e_inval jump target in
the same function, and cleaned up the use ipv6_renew_option() by
ipv6_renew_options().
CC: Al Viro
Signed-off-by: Paul Moore
--
v3:
- fix typo in ipv6_renew_option() spotted by David Miller
v2:
- handle opt == NULL properly in ipv6_renew_options()
---
include/net/ipv6.h |9
net/ipv6/calipso.c |9 +---
net/ipv6/exthdrs.c | 111 --
net/ipv6/ipv6_sockglue.c | 27 ---
4 files changed, 53 insertions(+), 103 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
struct ipv6_txoptions *opt,
int newtype,
- struct ipv6_opt_hdr __user *newopt,
- int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
- struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr *newopt,
- int newoptlen);
+ struct ipv6_opt_hdr *newopt);
struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
struct ipv6_txoptions *opt);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct
ipv6_opt_hdr *hop)
{
struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
- txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-hop, hop ? ipv6_optlen(hop) : 0);
+ txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
txopt_put(old);
if (IS_ERR(txopts))
return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
if (IS_ERR(new))
return PTR_ERR(new);
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
+ txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
kfree(new);
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
if (calipso_opt_del(req_inet->ipv6_opt->hopopt, ))
return; /* Nothing to do */
- txopts = ipv6_renew_options_kern(sk, req_in
Re: [RFC PATCH v2] ipv6: make ipv6_renew_options() interrupt/kernel safe
On Wed, Jul 4, 2018 at 1:29 AM David Miller wrote:
> From: Paul Moore
> Date: Mon, 02 Jul 2018 14:20:52 -0400
>
> > -static int ipv6_renew_option(void *ohdr,
> > - struct ipv6_opt_hdr __user *newopt, int
> > newoptlen,
> > - int inherit,
> > - struct ipv6_opt_hdr **hdr,
> > - char **p)
> > +static void ipv6_renew_option(int renewtype,
> > + struct ipv6_opt_hdr **dest,
> > + struct ipv6_opt_hdr *old,
> > + struct ipv6_opt_hdr *new,
> > + int newtype, char **p)
> > {
> ...
> > + p += CMSG_ALIGN(ipv6_optlen(*dest));
>
> I don't think this actually advances the pointer in the caller,
> you need something like:
>
> *p += CMSG_ALIGN(ipv6_optlen(*dest));
Yep, my mistake (typo); thanks for catching it. Rebuilding a test
kernel now ...
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Use pr_fmt to prefix "SELinux: "
On Mon, Jul 2, 2018 at 10:10 PM Joe Perches wrote: > On Mon, 2018-07-02 at 21:15 -0400, Paul Moore wrote: > > On Mon, Jul 2, 2018 at 5:31 PM Joe Perches wrote: > > > On Mon, 2018-07-02 at 16:51 -0400, Paul Moore wrote: > > > > On Wed, Jun 20, 2018 at 2:39 AM Joe Perches wrote: > > > > > pr_fmt can be used with the pr_ macros to prefix > > > > > arbitrary content to logging messages. > > > > > > > > > > So add '#define pr_fmt(fmt) "SELinux: " fmt' to selinux files > > > > > that use pr_ and remove embedded "SELinux: " prefixes > > > > > from the format strings. > > > [] > > > > What tree did you base your patch on? > > > > > > next-20180619 on the day it was sent. > > > > > > > Please base SELinux patches > > > > either on the SELinux tree or Linus' tree. The SELinux tree can be > > > > found at the links below: > > > > > > > > * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > > > > * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git > > > > > > It's almost always better to use selinux/next. > > > Why should this patch be different? > > > > Yes, that's what I said, please base your patch against the selinux/next > > branch. > > Again, this patch was based on that branch on the date the > patch was sent. > > It still applies today to -next. It doesn't apply cleanly to the selinux/next branch, I believe because there are other trees which have made changes under security/selinux/* (I believe there was some fs related work from David Howells that touched some SELinux/LSM code). As I said earlier, if you want me to apply your patches to the SELinux tree, please base them either against the selinux/next tree or against Linus' tree. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Use pr_fmt to prefix "SELinux: "
On Mon, Jul 2, 2018 at 5:31 PM Joe Perches wrote:
> On Mon, 2018-07-02 at 16:51 -0400, Paul Moore wrote:
> > On Wed, Jun 20, 2018 at 2:39 AM Joe Perches wrote:
> > > pr_fmt can be used with the pr_ macros to prefix
> > > arbitrary content to logging messages.
> > >
> > > So add '#define pr_fmt(fmt) "SELinux: " fmt' to selinux files
> > > that use pr_ and remove embedded "SELinux: " prefixes
> > > from the format strings.
>
> []
> > > @@ -1644,13 +1640,12 @@ static int inode_doinit_with_dentry(struct inode
> > > *inode, struct dentry *opt_dent
> > >
> > > if (rc == -EINVAL) {
> > > if (printk_ratelimit())
> > > - pr_notice("SELinux:
> > > inode=%lu on dev=%s was found to have an invalid "
> > > - "context=%s.
> > > This indicates you may need to relabel the inode or the "
> > > - "filesystem in
> > > question.\n", ino, dev, context);
> > > + pr_notice("inode=%lu on
> > > dev=%s was found to have an invalid context=%s. This indicates you may
> > > need to relabel the inode or the filesystem in question.\n",
> > > + ino, dev,
> > > context);
> >
> > Please split up lines like this. I realize that there isn't much room
> > left, but this message wraps a silly amount in my 80-char terminal; if
> > you need to wrap, please limit it to a word or two.
> >
> > To stop the argument before it starts, I don't care what checkpatch.pl
> > says about splitting printk format strings like this.
>
> I do, so does Linus. It's also specified in CodingStyle.
>
> 2) Breaking long lines and strings
> --
> []
> never break user-visible strings such as
> printk messages, because that breaks the ability to grep for them.
> --
>
> Likely this would also use pr_notice_ratelimited(etc...)
> to reduce indentation a bit too.
>
> Probably these should be output on 2 lines as the output
> dmesg line length is extremely long.
That line is too long. Split it please.
> > What tree did you base your patch on?
>
> next-20180619 on the day it was sent.
>
> > Please base SELinux patches
> > either on the SELinux tree or Linus' tree. The SELinux tree can be
> > found at the links below:
> >
> > * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
> > * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
>
> It's almost always better to use selinux/next.
> Why should this patch be different?
Yes, that's what I said, please base your patch against the selinux/next branch.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: Use pr_fmt to prefix "SELinux: "
On Wed, Jun 20, 2018 at 2:39 AM Joe Perches wrote:
> pr_fmt can be used with the pr_ macros to prefix
> arbitrary content to logging messages.
>
> So add '#define pr_fmt(fmt) "SELinux: " fmt' to selinux files
> that use pr_ and remove embedded "SELinux: " prefixes
> from the format strings.
>
> Miscellanea:
>
> o Coalesce formats and realign arguments
> o Add missing space to a coalesced format
> o Remove "SELinux: " from SEL_MOUNT_FAIL_MSG as that is directly
> used only in pr_warn
> o Add missing terminating \n to some formats
> o Consistently use single space after logging prefixes
>
> Signed-off-by: Joe Perches
> ---
> security/selinux/avc.c| 7 ++-
> security/selinux/hooks.c | 124
> +-
> security/selinux/netif.c | 7 ++-
> security/selinux/netlink.c| 7 ++-
> security/selinux/netnode.c| 4 +-
> security/selinux/netport.c| 4 +-
> security/selinux/selinuxfs.c | 33 +-
> security/selinux/ss/avtab.c | 61 +--
> security/selinux/ss/conditional.c | 18 +++---
> security/selinux/ss/ebitmap.c | 20 +++---
> security/selinux/ss/policydb.c| 123 +
> security/selinux/ss/services.c| 110 +
> security/selinux/ss/sidtab.c | 12 ++--
> 13 files changed, 247 insertions(+), 283 deletions(-)
...
> @@ -1644,13 +1640,12 @@ static int inode_doinit_with_dentry(struct inode
> *inode, struct dentry *opt_dent
>
> if (rc == -EINVAL) {
> if (printk_ratelimit())
> - pr_notice("SELinux: inode=%lu
> on dev=%s was found to have an invalid "
> - "context=%s. This
> indicates you may need to relabel the inode or the "
> - "filesystem in
> question.\n", ino, dev, context);
> + pr_notice("inode=%lu on
> dev=%s was found to have an invalid context=%s. This indicates you may need
> to relabel the inode or the filesystem in question.\n",
> + ino, dev, context);
Please split up lines like this. I realize that there isn't much room
left, but this message wraps a silly amount in my 80-char terminal; if
you need to wrap, please limit it to a word or two.
To stop the argument before it starts, I don't care what checkpatch.pl
says about splitting printk format strings like this. Split the line
please.
> @@ -3124,9 +3117,9 @@ static int selinux_validate_for_sb_reconfigure(struct
> fs_context *fc)
> rc = security_context_str_to_sid(_state,
> mount_options[i],
> , GFP_KERNEL);
> if (rc) {
> - pr_warn("SELinux: security_context_str_to_sid"
> - "(%s) failed for (dev %s, type %s)
> errno=%d\n",
> - mount_options[i], sb->s_id, sb->s_type->name,
> rc);
> + pr_warn("security_context_str_to_sid(%s) failed for
> (dev %s, type %s) errno=%d\n",
> + mount_options[i],
> + sb->s_id, sb->s_type->name, rc);
> goto inval;
> }
What tree did you base your patch on? Please base SELinux patches
either on the SELinux tree or Linus' tree. The SELinux tree can be
found at the links below:
* git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
* https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[RFC PATCH v2] ipv6: make ipv6_renew_options() interrupt/kernel safe
From: Paul Moore
At present the ipv6_renew_options_kern() function ends up calling into
access_ok() which is problematic if done from inside an interrupt as
access_ok() calls WARN_ON_IN_IRQ() on some (all?) architectures
(x86-64 is affected). Example warning/backtrace is shown below:
WARNING: CPU: 1 PID: 3144 at lib/usercopy.c:11 _copy_from_user+0x85/0x90
...
Call Trace:
ipv6_renew_option+0xb2/0xf0
ipv6_renew_options+0x26a/0x340
ipv6_renew_options_kern+0x2c/0x40
calipso_req_setattr+0x72/0xe0
netlbl_req_setattr+0x126/0x1b0
selinux_netlbl_inet_conn_request+0x80/0x100
selinux_inet_conn_request+0x6d/0xb0
security_inet_conn_request+0x32/0x50
tcp_conn_request+0x35f/0xe00
? __lock_acquire+0x250/0x16c0
? selinux_socket_sock_rcv_skb+0x1ae/0x210
? tcp_rcv_state_process+0x289/0x106b
tcp_rcv_state_process+0x289/0x106b
? tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_rcv+0xc82/0xcf0
ip6_input_finish+0x10d/0x690
ip6_input+0x45/0x1e0
? ip6_rcv_finish+0x1d0/0x1d0
ipv6_rcv+0x32b/0x880
? ip6_make_skb+0x1e0/0x1e0
__netif_receive_skb_core+0x6f2/0xdf0
? process_backlog+0x85/0x250
? process_backlog+0x85/0x250
? process_backlog+0xec/0x250
process_backlog+0xec/0x250
net_rx_action+0x153/0x480
__do_softirq+0xd9/0x4f7
do_softirq_own_stack+0x2a/0x40
...
While not present in the backtrace, ipv6_renew_option() ends up calling
access_ok() via the following chain:
access_ok()
_copy_from_user()
copy_from_user()
ipv6_renew_option()
The fix presented in this patch is to perform the userspace copy
earlier in the call chain such that it is only called when the option
data is actually coming from userspace; that place is
do_ipv6_setsockopt(). Not only does this solve the problem seen in
the backtrace above, it also allows us to simplify the code quite a
bit by removing ipv6_renew_options_kern() completely. We also take
this opportunity to cleanup ipv6_renew_options()/ipv6_renew_option()
a small amount as well.
This patch is heavily based on a rough patch by Al Viro. I've taken
his original patch, converted a kmemdup() call in do_ipv6_setsockopt()
to a memdup_user() call, made better use of the e_inval jump target in
the same function, and cleaned up the use ipv6_renew_option() by
ipv6_renew_options().
CC: Al Viro
Signed-off-by: Paul Moore
--
v2:
- handle opt == NULL properly in ipv6_renew_options()
---
include/net/ipv6.h |9
net/ipv6/calipso.c |9 +---
net/ipv6/exthdrs.c | 111 --
net/ipv6/ipv6_sockglue.c | 27 ---
4 files changed, 53 insertions(+), 103 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
struct ipv6_txoptions *opt,
int newtype,
- struct ipv6_opt_hdr __user *newopt,
- int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
- struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr *newopt,
- int newoptlen);
+ struct ipv6_opt_hdr *newopt);
struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
struct ipv6_txoptions *opt);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct
ipv6_opt_hdr *hop)
{
struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
- txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-hop, hop ? ipv6_optlen(hop) : 0);
+ txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
txopt_put(old);
if (IS_ERR(txopts))
return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
if (IS_ERR(new))
return PTR_ERR(new);
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
+ txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
kfree(new);
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
if (calipso_opt_del(req_inet->ipv6_opt->hopopt, ))
return; /* Nothing to do */
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-
Re: [RFC PATCH] ipv6: make ipv6_renew_options() interrupt/kernel safe
On July 1, 2018 11:01:04 PM Paul Moore wrote:
> From: Paul Moore
>
> At present the ipv6_renew_options_kern() function ends up calling into
> access_ok() which is problematic if done from inside an interrupt as
> access_ok() calls WARN_ON_IN_IRQ() on some (all?) architectures
> (x86-64 is affected). Example warning/backtrace is shown below:
>
> WARNING: CPU: 1 PID: 3144 at lib/usercopy.c:11 _copy_from_user+0x85/0x90
> ...
> Call Trace:
>
> ipv6_renew_option+0xb2/0xf0
> ipv6_renew_options+0x26a/0x340
> ipv6_renew_options_kern+0x2c/0x40
> calipso_req_setattr+0x72/0xe0
> netlbl_req_setattr+0x126/0x1b0
> selinux_netlbl_inet_conn_request+0x80/0x100
> selinux_inet_conn_request+0x6d/0xb0
> security_inet_conn_request+0x32/0x50
> tcp_conn_request+0x35f/0xe00
> ? __lock_acquire+0x250/0x16c0
> ? selinux_socket_sock_rcv_skb+0x1ae/0x210
> ? tcp_rcv_state_process+0x289/0x106b
> tcp_rcv_state_process+0x289/0x106b
> ? tcp_v6_do_rcv+0x1a7/0x3c0
> tcp_v6_do_rcv+0x1a7/0x3c0
> tcp_v6_rcv+0xc82/0xcf0
> ip6_input_finish+0x10d/0x690
> ip6_input+0x45/0x1e0
> ? ip6_rcv_finish+0x1d0/0x1d0
> ipv6_rcv+0x32b/0x880
> ? ip6_make_skb+0x1e0/0x1e0
> __netif_receive_skb_core+0x6f2/0xdf0
> ? process_backlog+0x85/0x250
> ? process_backlog+0x85/0x250
> ? process_backlog+0xec/0x250
> process_backlog+0xec/0x250
> net_rx_action+0x153/0x480
> __do_softirq+0xd9/0x4f7
> do_softirq_own_stack+0x2a/0x40
>
> ...
>
> While not present in the backtrace, ipv6_renew_option() ends up calling
> access_ok() via the following chain:
>
> access_ok()
> _copy_from_user()
> copy_from_user()
> ipv6_renew_option()
>
> The fix presented in this patch is to perform the userspace copy
> earlier in the call chain such that it is only called when the option
> data is actually coming from userspace; that place is
> do_ipv6_setsockopt(). Not only does this solve the problem seen in
> the backtrace above, it also allows us to simplify the code quite a
> bit by removing ipv6_renew_options_kern() completely. We also take
> this opportunity to cleanup ipv6_renew_options()/ipv6_renew_option()
> a small amount as well.
>
> This patch is heavily based on a rough patch by Al Viro. I've taken
> his original patch, converted a kmemdup() call in do_ipv6_setsockopt()
> to a memdup_user() call, made better use of the e_inval jump target in
> the same function, and cleaned up the use ipv6_renew_option() by
> ipv6_renew_options().
>
> CC: Al Viro
> Signed-off-by: Paul Moore
> ---
> include/net/ipv6.h |9
> net/ipv6/calipso.c |9 +---
> net/ipv6/exthdrs.c | 108 --
> net/ipv6/ipv6_sockglue.c | 27
> 4 files changed, 50 insertions(+), 103 deletions(-)
Hold off on this patch, while it worked for me, I just received a bug report
from Intel's 0day robot that I want to chase down.
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 16475c269749..d02881e4ad1f 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
> struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
>struct ipv6_txoptions *opt,
>int newtype,
> - struct ipv6_opt_hdr __user *newopt,
> - int newoptlen);
> -struct ipv6_txoptions *
> -ipv6_renew_options_kern(struct sock *sk,
> - struct ipv6_txoptions *opt,
> - int newtype,
> - struct ipv6_opt_hdr *newopt,
> - int newoptlen);
> + struct ipv6_opt_hdr *newopt);
> struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
>struct ipv6_txoptions *opt);
>
> diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> index 1323b9679cf7..1c0bb9fb76e6 100644
> --- a/net/ipv6/calipso.c
> +++ b/net/ipv6/calipso.c
> @@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct
> ipv6_opt_hdr *hop)
> {
> struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
>
> - txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
> - hop, hop ? ipv6_optlen(hop) : 0);
> + txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
> txopt_put(old);
> if (IS_ERR(txopts))
> return PTR_ERR(txopts);
> @@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
> if (IS_ERR(new))
> return PTR_ERR(new);
>
> - txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
> - new, new ? ipv6_optlen(new) : 0);
> + txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
>
> kfree(new);
>
> @@ -1260,8 +1258,7 @@ static void ca
[RFC PATCH] ipv6: make ipv6_renew_options() interrupt/kernel safe
From: Paul Moore
At present the ipv6_renew_options_kern() function ends up calling into
access_ok() which is problematic if done from inside an interrupt as
access_ok() calls WARN_ON_IN_IRQ() on some (all?) architectures
(x86-64 is affected). Example warning/backtrace is shown below:
WARNING: CPU: 1 PID: 3144 at lib/usercopy.c:11 _copy_from_user+0x85/0x90
...
Call Trace:
ipv6_renew_option+0xb2/0xf0
ipv6_renew_options+0x26a/0x340
ipv6_renew_options_kern+0x2c/0x40
calipso_req_setattr+0x72/0xe0
netlbl_req_setattr+0x126/0x1b0
selinux_netlbl_inet_conn_request+0x80/0x100
selinux_inet_conn_request+0x6d/0xb0
security_inet_conn_request+0x32/0x50
tcp_conn_request+0x35f/0xe00
? __lock_acquire+0x250/0x16c0
? selinux_socket_sock_rcv_skb+0x1ae/0x210
? tcp_rcv_state_process+0x289/0x106b
tcp_rcv_state_process+0x289/0x106b
? tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_rcv+0xc82/0xcf0
ip6_input_finish+0x10d/0x690
ip6_input+0x45/0x1e0
? ip6_rcv_finish+0x1d0/0x1d0
ipv6_rcv+0x32b/0x880
? ip6_make_skb+0x1e0/0x1e0
__netif_receive_skb_core+0x6f2/0xdf0
? process_backlog+0x85/0x250
? process_backlog+0x85/0x250
? process_backlog+0xec/0x250
process_backlog+0xec/0x250
net_rx_action+0x153/0x480
__do_softirq+0xd9/0x4f7
do_softirq_own_stack+0x2a/0x40
...
While not present in the backtrace, ipv6_renew_option() ends up calling
access_ok() via the following chain:
access_ok()
_copy_from_user()
copy_from_user()
ipv6_renew_option()
The fix presented in this patch is to perform the userspace copy
earlier in the call chain such that it is only called when the option
data is actually coming from userspace; that place is
do_ipv6_setsockopt(). Not only does this solve the problem seen in
the backtrace above, it also allows us to simplify the code quite a
bit by removing ipv6_renew_options_kern() completely. We also take
this opportunity to cleanup ipv6_renew_options()/ipv6_renew_option()
a small amount as well.
This patch is heavily based on a rough patch by Al Viro. I've taken
his original patch, converted a kmemdup() call in do_ipv6_setsockopt()
to a memdup_user() call, made better use of the e_inval jump target in
the same function, and cleaned up the use ipv6_renew_option() by
ipv6_renew_options().
CC: Al Viro
Signed-off-by: Paul Moore
---
include/net/ipv6.h |9
net/ipv6/calipso.c |9 +---
net/ipv6/exthdrs.c | 108 --
net/ipv6/ipv6_sockglue.c | 27
4 files changed, 50 insertions(+), 103 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
struct ipv6_txoptions *opt,
int newtype,
- struct ipv6_opt_hdr __user *newopt,
- int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
- struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr *newopt,
- int newoptlen);
+ struct ipv6_opt_hdr *newopt);
struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
struct ipv6_txoptions *opt);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct
ipv6_opt_hdr *hop)
{
struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
- txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-hop, hop ? ipv6_optlen(hop) : 0);
+ txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
txopt_put(old);
if (IS_ERR(txopts))
return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
if (IS_ERR(new))
return PTR_ERR(new);
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
+ txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
kfree(new);
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
if (calipso_opt_del(req_inet->ipv6_opt->hopopt, ))
return; /* Nothing to do */
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
[GIT PULL] SELinux fixes for v4.18 (#1)
Hi Linus, One fairly straightforward patch to fix a longstanding issue where a process could stall while accessing files in selinuxfs and block everyone else due to a held mutex. The patch passes all our tests and looks to apply cleanly to your current tree. Please pull this as a fix for v4.18. Thanks, -Paul -- The following changes since commit d141136f523a3a6372d22981bdff7a8906f36fea: audit: normalize MAC_POLICY_LOAD record (2018-04-17 17:54:11 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20180629 for you to fetch changes up to 0da74120c5341389b97c4ee27487a97224999ee1: selinux: move user accesses in selinuxfs out of locked regions (2018-06-28 20:39:54 -0400) selinux/stable-4.18 PR 20180629 Jann Horn (1): selinux: move user accesses in selinuxfs out of locked regions security/selinux/selinuxfs.c | 78 +--- 1 file changed, 33 insertions(+), 45 deletions(-) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Thu, Jun 28, 2018 at 8:38 PM Paul Moore wrote:
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley wrote:
> > > On 06/25/2018 12:34 PM, Jann Horn wrote:
> > > > If a user is accessing a file in selinuxfs with a pointer to a userspace
> > > > buffer that is backed by e.g. a userfaultfd, the userspace access can
> > > > stall indefinitely, which can block fsi->mutex if it is held.
> > > >
> > > > For sel_read_policy(), remove the locking, since this method doesn't
> > > > seem
> > > > to access anything that requires locking.
> > > >
> > > > For sel_read_bool(), move the user access below the locked region.
> > > >
> > > > For sel_write_bool() and sel_commit_bools_write(), move the user access
> > > > up above the locked region.
> > > >
> > > > Cc: sta...@vger.kernel.org
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn
> > >
> > > Only question I have is wrt the Fixes line, i.e. was this an issue until
> > > userfaultfd was introduced, and if not,
> > > do we need it to be back-ported any further than the commit which
> > > introduced it.
> >
> > Considering we are talking about v2.6.12 I have to wonder if anyone is
> > bothering with backports for kernels that old. Even the RHEL-5.x
> > based systems are at least on v2.6.18.
> >
> > Regardless, I think this is fine to merge as-is; thanks everyone.
>
> FYI, I did have to remove the "fsi" variable from sel_read_policy() to
> keep the compiler happy. Please double check to make sure your code
> compiles cleanly in the future.
I realize I didn't specify this above ... I merged this into
selinux/stable-4.18; I'm building a test kernel now and if everything
looks okay I'll send it to Linus tomorrow.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Thu, Jun 28, 2018 at 8:23 PM Paul Moore wrote:
> On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley wrote:
> > On 06/25/2018 12:34 PM, Jann Horn wrote:
> > > If a user is accessing a file in selinuxfs with a pointer to a userspace
> > > buffer that is backed by e.g. a userfaultfd, the userspace access can
> > > stall indefinitely, which can block fsi->mutex if it is held.
> > >
> > > For sel_read_policy(), remove the locking, since this method doesn't seem
> > > to access anything that requires locking.
> > >
> > > For sel_read_bool(), move the user access below the locked region.
> > >
> > > For sel_write_bool() and sel_commit_bools_write(), move the user access
> > > up above the locked region.
> > >
> > > Cc: sta...@vger.kernel.org
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Jann Horn
> >
> > Only question I have is wrt the Fixes line, i.e. was this an issue until
> > userfaultfd was introduced, and if not,
> > do we need it to be back-ported any further than the commit which
> > introduced it.
>
> Considering we are talking about v2.6.12 I have to wonder if anyone is
> bothering with backports for kernels that old. Even the RHEL-5.x
> based systems are at least on v2.6.18.
>
> Regardless, I think this is fine to merge as-is; thanks everyone.
FYI, I did have to remove the "fsi" variable from sel_read_policy() to
keep the compiler happy. Please double check to make sure your code
compiles cleanly in the future.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley wrote:
> On 06/25/2018 12:34 PM, Jann Horn wrote:
> > If a user is accessing a file in selinuxfs with a pointer to a userspace
> > buffer that is backed by e.g. a userfaultfd, the userspace access can
> > stall indefinitely, which can block fsi->mutex if it is held.
> >
> > For sel_read_policy(), remove the locking, since this method doesn't seem
> > to access anything that requires locking.
> >
> > For sel_read_bool(), move the user access below the locked region.
> >
> > For sel_write_bool() and sel_commit_bools_write(), move the user access
> > up above the locked region.
> >
> > Cc: sta...@vger.kernel.org
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn
>
> Only question I have is wrt the Fixes line, i.e. was this an issue until
> userfaultfd was introduced, and if not,
> do we need it to be back-ported any further than the commit which introduced
> it.
Considering we are talking about v2.6.12 I have to wonder if anyone is
bothering with backports for kernels that old. Even the RHEL-5.x
based systems are at least on v2.6.18.
Regardless, I think this is fine to merge as-is; thanks everyone.
> Otherwise, you can add my
> Acked-by: Stephen Smalley
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Mon, Jun 25, 2018 at 6:40 PM Jann Horn wrote: > > On Tue, Jun 26, 2018 at 12:36 AM Paul Moore wrote: > > > > On Mon, Jun 25, 2018 at 12:34 PM Jann Horn wrote: > > > If a user is accessing a file in selinuxfs with a pointer to a userspace > > > buffer that is backed by e.g. a userfaultfd, the userspace access can > > > stall indefinitely, which can block fsi->mutex if it is held. > > > > > > For sel_read_policy(), remove the locking, since this method doesn't seem > > > to access anything that requires locking. > > > > Forgive me, I'm thinking about this quickly so I could be very wrong > > here, but isn't the mutex needed to prevent problems in multi-threaded > > apps hitting the same fd at the same time? > > sel_read_policy() operates on a read-only copy of the policy, accessed > via ->private_data, allocated using vmalloc in sel_open_policy() via > security_read_policy(). As far as I can tell, nothing can write to > that read-only copy of the policy. None of the handlers in > sel_policy_ops write - they just mmap as readonly (in which case > you're already reading without locks, by the way) or read. Great, thanks. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
@@ -1280,6 +1274,17 @@ static ssize_t sel_commit_bools_write(struct file
> *filep,
> ssize_t length;
> int new_value;
>
> + if (count >= PAGE_SIZE)
> + return -ENOMEM;
> +
> + /* No partial writes. */
> + if (*ppos != 0)
> + return -EINVAL;
> +
> + page = memdup_user_nul(buf, count);
> + if (IS_ERR(page))
> + return PTR_ERR(page);
> +
> mutex_lock(>mutex);
>
> length = avc_has_perm(_state,
> @@ -1289,22 +1294,6 @@ static ssize_t sel_commit_bools_write(struct file
> *filep,
> if (length)
> goto out;
>
> - length = -ENOMEM;
> - if (count >= PAGE_SIZE)
> - goto out;
> -
> - /* No partial writes. */
> - length = -EINVAL;
> - if (*ppos != 0)
> - goto out;
> -
> - page = memdup_user_nul(buf, count);
> - if (IS_ERR(page)) {
> - length = PTR_ERR(page);
> - page = NULL;
> - goto out;
> - }
> -
> length = -EINVAL;
> if (sscanf(page, "%d", _value) != 1)
> goto out;
> --
> 2.18.0.rc2.346.g013aa6912e-goog
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()
On Sun, Jun 24, 2018 at 3:48 AM David Miller wrote:
>
> From: Al Viro
> Date: Sat, 23 Jun 2018 23:21:07 +0100
>
> > BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> > the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> > simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> > ipv6_renew_options_kern()...
>
> I agree that this makes things a lot simpler.
I had looked at moving the userspace copy up, but feared it was a bit
too invasive. It sounds like you are open to the idea so I'll code
something up.
> One thing that drives me crazy though is this inherit stuff:
>
> > + ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> > + opt ? opt->hopopt : NULL,
>
> Why don't we pass the type into ipv6_renew_option() and have it
> do this pointer dance instead?
>
> That's going to definitely be easier to read.
I agree, that struck me as a little odd. I'll rework that too. I'll
send you guys something this week to take a look at.
Thanks.
> I don't know enough about this code to give feedback about the
> option length handling wrt. copies, sorry.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()
On Sat, Jun 23, 2018 at 8:16 AM David Miller wrote: > > From: Paul Moore > Date: Fri, 22 Jun 2018 17:18:20 -0400 > > > From: Paul Moore > > > > The ipv6_renew_options_kern() function eventually called into > > copy_from_user(), despite it not using any userspace buffers, which > > was problematic as that ended up calling access_ok() which emited > > a warning on x86 (and likely other arches as well). > > > > ipv6_renew_options_kern() > > ipv6_renew_options() > > ipv6_renew_option() > > copy_from_user() > > _copy_from_user() > > access_ok() > > > > The access_ok() check inside _copy_from_user() is obviously the right > > thing to do which means that calling copy_from_user() via > > ipv6_renew_options_kern() is obviously the wrong thing to do. > > Ok, I re-read the code around here. > > access_ok() is not warning because we are calling copy_from_user() > with a kernel pointer. The set_ds(KERNEL_DS) adjusts the > user_addr_max() setting, and thus that check passes. > > The problem is that we are invoking this from an interrupt, and this > triggers the WARN_ON_IN_IRQ() in access_ok(). > > Although I think that WARN_ON_IN_IRQ() is completely unnecessary when > KERNEL_DS is set, the situation that really causes this problem is not > at all clear from your commit message. > > I guess that for now your fix is fine, but I want you to please adjust > the commit message. > > Provide the _full_ annotated kernel backtrace from the warning that > triggers, because this will show the reader that we are in an > interrupt. And explain that being in the interrupt is strictly what > causes this to warn, not that we are using kernel pointers. The > latter is %100 valid when set_fs(KERNEL_DS) is performed. > > Thank you. Okay, so it's the right fix for all the wrong reasons :) Thanks for the correction; I'll fixup the commit subject/description and resend when I'm in front of the system with the patch (later this weekend, early next week). -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()
From: Paul Moore
The ipv6_renew_options_kern() function eventually called into
copy_from_user(), despite it not using any userspace buffers, which
was problematic as that ended up calling access_ok() which emited
a warning on x86 (and likely other arches as well).
ipv6_renew_options_kern()
ipv6_renew_options()
ipv6_renew_option()
copy_from_user()
_copy_from_user()
access_ok()
The access_ok() check inside _copy_from_user() is obviously the right
thing to do which means that calling copy_from_user() via
ipv6_renew_options_kern() is obviously the wrong thing to do. This
patch fixes this by duplicating ipv6_renew_option() in the _kern()
variant, omitting the userspace copies and attributes. The patch
does make an attempt at limiting the duplicated code by moving the
option allocation code into a common helper function. I'm not in
love with this solution, but everything else I could think of seemed
worse.
The ipv6_renew_options_kern() function is an required by the
CALIPSO/RFC5570 code in net/ipv6/calipso.c.
Signed-off-by: Paul Moore
---
net/ipv6/exthdrs.c | 155 +---
1 file changed, 121 insertions(+), 34 deletions(-)
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5bc2bf3733ab..902748acd6fe 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1040,36 +1040,47 @@ static int ipv6_renew_option(void *ohdr,
return 0;
}
+static int ipv6_renew_option_kern(void *ohdr,
+ struct ipv6_opt_hdr *newopt, int newoptlen,
+ int inherit,
+ struct ipv6_opt_hdr **hdr,
+ char **p)
+{
+ if (inherit) {
+ if (ohdr) {
+ memcpy(*p, ohdr,
+ ipv6_optlen((struct ipv6_opt_hdr *)ohdr));
+ *hdr = (struct ipv6_opt_hdr *)*p;
+ *p += CMSG_ALIGN(ipv6_optlen(*hdr));
+ }
+ } else if (newopt) {
+ memcpy(*p, newopt, newoptlen);
+ *hdr = (struct ipv6_opt_hdr *)*p;
+ if (ipv6_optlen(*hdr) > newoptlen)
+ return -EINVAL;
+ *p += CMSG_ALIGN(newoptlen);
+ }
+ return 0;
+}
+
/**
- * ipv6_renew_options - replace a specific ext hdr with a new one.
+ * ipv6_renew_option_alloc - helper function for allocating ipv6_txoptions
*
* @sk: sock from which to allocate memory
* @opt: original options
* @newtype: option type to replace in @opt
- * @newopt: new option of type @newtype to replace (user-mem)
- * @newoptlen: length of @newopt
- *
- * Returns a new set of options which is a copy of @opt with the
- * option type @newtype replaced with @newopt.
+ * @newoptlen: length of the new option
*
- * @opt may be NULL, in which case a new set of options is returned
- * containing just @newopt.
- *
- * @newopt may be NULL, in which case the specified option type is
- * not copied into the new set of options.
- *
- * The new set of options is allocated from the socket option memory
- * buffer of @sk.
+ * This really should only ever be called by ipv6_renew_option() or
+ * ipv6_renew_option_kern().
*/
-struct ipv6_txoptions *
-ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr __user *newopt, int newoptlen)
+static struct ipv6_txoptions *ipv6_renew_option_alloc(struct sock *sk,
+ struct ipv6_txoptions
*opt,
+ int newtype,
+ int newoptlen)
{
int tot_len = 0;
- char *p;
struct ipv6_txoptions *opt2;
- int err;
if (opt) {
if (newtype != IPV6_HOPOPTS && opt->hopopt)
@@ -1082,7 +1093,7 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions
*opt,
tot_len += CMSG_ALIGN(ipv6_optlen(opt->dst1opt));
}
- if (newopt && newoptlen)
+ if (newoptlen)
tot_len += CMSG_ALIGN(newoptlen);
if (!tot_len)
@@ -1096,6 +1107,44 @@ ipv6_renew_options(struct sock *sk, struct
ipv6_txoptions *opt,
memset(opt2, 0, tot_len);
refcount_set(>refcnt, 1);
opt2->tot_len = tot_len;
+
+ return opt2;
+}
+
+/**
+ * ipv6_renew_options - replace a specific ext hdr with a new one.
+ *
+ * @sk: sock from which to allocate memory
+ * @opt: original options
+ * @newtype: option type to replace in @opt
+ * @newopt: new option of type @newtype to replace (user-mem)
+ * @newoptlen: length of @newopt
+ *
+ * Returns a new set of options which is a copy of @opt with the
+ * option type @newtype replaced with @newopt.
+ *
+ * @opt may be NULL, in which case a new set of options is returned
+ * containing ju
Re: [PATCH 11/13] selinux: Cleanup printk logging in netif
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/netif.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
Merged, thanks.
> diff --git a/security/selinux/netif.c b/security/selinux/netif.c
> index ac65f7417413..8c738c189942 100644
> --- a/security/selinux/netif.c
> +++ b/security/selinux/netif.c
> @@ -145,9 +145,8 @@ static int sel_netif_sid_slow(struct net *ns, int
> ifindex, u32 *sid)
>
> dev = dev_get_by_index(ns, ifindex);
> if (unlikely(dev == NULL)) {
> - printk(KERN_WARNING
> - "SELinux: failure in sel_netif_sid_slow(),"
> - " invalid network interface (%d)\n", ifindex);
> + pr_warn("SELinux: failure in %s(), invalid network interface
> (%d)\n",
> + __func__, ifindex);
> return -ENOENT;
> }
>
> @@ -177,10 +176,8 @@ static int sel_netif_sid_slow(struct net *ns, int
> ifindex, u32 *sid)
> spin_unlock_bh(_netif_lock);
> dev_put(dev);
> if (unlikely(ret)) {
> - printk(KERN_WARNING
> - "SELinux: failure in sel_netif_sid_slow(),"
> - " unable to determine network interface label (%d)\n",
> - ifindex);
> + pr_warn("SELinux: failure in %s(), unable to determine
> network interface label (%d)\n",
> + __func__, ifindex);
> kfree(new);
> }
> return ret;
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 12/13] selinux: Cleanup printk logging in avc
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/avc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged, thanks.
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index f3aedf077509..635e5c1e3e48 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -650,7 +650,7 @@ static int avc_latest_notif_update(struct selinux_avc
> *avc,
> spin_lock_irqsave(_lock, flag);
> if (is_insert) {
> if (seqno < avc->avc_cache.latest_notif) {
> - printk(KERN_WARNING "SELinux: avc: seqno %d <
> latest_notif %d\n",
> + pr_warn("SELinux: avc: seqno %d < latest_notif %d\n",
>seqno, avc->avc_cache.latest_notif);
> ret = -EAGAIN;
> }
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/13] selinux: Cleanup printk logging in netport
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/netport.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Merged, thanks.
> diff --git a/security/selinux/netport.c b/security/selinux/netport.c
> index 9ed4c5064a5e..7a141cadbffc 100644
> --- a/security/selinux/netport.c
> +++ b/security/selinux/netport.c
> @@ -173,9 +173,8 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum,
> u32 *sid)
> out:
> spin_unlock_bh(_netport_lock);
> if (unlikely(ret)) {
> - printk(KERN_WARNING
> - "SELinux: failure in sel_netport_sid_slow(),"
> - " unable to determine network port label\n");
> + pr_warn("SELinux: failure in %s(), unable to determine
> network port label\n",
> + __func__);
> kfree(new);
> }
> return ret;
> --
> 2.15.1
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 13/13] selinux: Cleanup printk logging in netnode
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/netnode.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Merged, thanks.
> diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
> index 6dd89b89bc1f..afa0d432436b 100644
> --- a/security/selinux/netnode.c
> +++ b/security/selinux/netnode.c
> @@ -238,9 +238,8 @@ static int sel_netnode_sid_slow(void *addr, u16 family,
> u32 *sid)
> out:
> spin_unlock_bh(_netnode_lock);
> if (unlikely(ret)) {
> - printk(KERN_WARNING
> - "SELinux: failure in sel_netnode_sid_slow(),"
> - " unable to determine network node label\n");
> + pr_warn("SELinux: failure in %s(), unable to determine
> network node label\n",
> + __func__);
> kfree(new);
> }
> return ret;
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 09/13] selinux: Cleanup printk logging in sidtab
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/ss/sidtab.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Merged, thanks.
> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index 5be31b7af225..fd75a12fa8fc 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -214,8 +214,7 @@ int sidtab_context_to_sid(struct sidtab *s,
> }
> sid = s->next_sid++;
> if (context->len)
> - printk(KERN_INFO
> - "SELinux: Context %s is not valid (left unmapped).\n",
> + pr_info("SELinux: Context %s is not valid (left
> unmapped).\n",
>context->str);
> ret = sidtab_insert(s, sid, context);
> if (ret)
> @@ -253,7 +252,7 @@ void sidtab_hash_eval(struct sidtab *h, char *tag)
> }
> }
>
> - printk(KERN_DEBUG "%s: %d entries and %d/%d buckets used, longest "
> + pr_debug("%s: %d entries and %d/%d buckets used, longest "
> "chain length %d\n", tag, h->nel, slots_used, SIDTAB_SIZE,
>max_chain_len);
> }
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 08/13] selinux: Cleanup printk logging in netlink
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/netlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged, thanks.
> diff --git a/security/selinux/netlink.c b/security/selinux/netlink.c
> index 828fb6a4e941..8a8a72507437 100644
> --- a/security/selinux/netlink.c
> +++ b/security/selinux/netlink.c
> @@ -94,7 +94,7 @@ static void selnl_notify(int msgtype, void *data)
> out_kfree_skb:
> kfree_skb(skb);
> oom:
> - printk(KERN_ERR "SELinux: OOM in %s\n", __func__);
> + pr_err("SELinux: OOM in %s\n", __func__);
> goto out;
> }
>
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 07/13] selinux: Cleanup printk logging in selinuxfs
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/selinuxfs.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
Merged, thanks.
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index c0cadbc5f85c..2adfade99945 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -620,7 +620,7 @@ static ssize_t sel_write_context(struct file *file, char
> *buf, size_t size)
>
> length = -ERANGE;
> if (len > SIMPLE_TRANSACTION_LIMIT) {
> - printk(KERN_ERR "SELinux: %s: context size (%u) exceeds "
> + pr_err("SELinux: %s: context size (%u) exceeds "
> "payload max\n", __func__, len);
> goto out;
> }
> @@ -956,7 +956,7 @@ static ssize_t sel_write_create(struct file *file, char
> *buf, size_t size)
>
> length = -ERANGE;
> if (len > SIMPLE_TRANSACTION_LIMIT) {
> - printk(KERN_ERR "SELinux: %s: context size (%u) exceeds "
> + pr_err("SELinux: %s: context size (%u) exceeds "
> "payload max\n", __func__, len);
> goto out;
> }
> @@ -1147,7 +1147,7 @@ static ssize_t sel_write_member(struct file *file, char
> *buf, size_t size)
>
> length = -ERANGE;
> if (len > SIMPLE_TRANSACTION_LIMIT) {
> - printk(KERN_ERR "SELinux: %s: context size (%u) exceeds "
> + pr_err("SELinux: %s: context size (%u) exceeds "
> "payload max\n", __func__, len);
> goto out;
> }
> @@ -1996,7 +1996,7 @@ static int sel_fill_super(struct super_block *sb, void
> *data, int silent)
> goto err;
> return 0;
> err:
> - printk(KERN_ERR "SELinux: %s: failed while creating inodes\n",
> + pr_err("SELinux: %s: failed while creating inodes\n",
> __func__);
>
> selinux_fs_info_free(sb);
> @@ -2046,7 +2046,7 @@ static int __init init_sel_fs(void)
>
> selinux_null.mnt = selinuxfs_mount = kern_mount(_fs_type);
> if (IS_ERR(selinuxfs_mount)) {
> - printk(KERN_ERR "selinuxfs: could not mount!\n");
> + pr_err("selinuxfs: could not mount!\n");
> err = PTR_ERR(selinuxfs_mount);
> selinuxfs_mount = NULL;
> }
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 06/13] selinux: Cleanup printk logging in services
ot;SELinux: Context %s would be invalid if
> enforcing\n",
> + s);
> kfree(s);
> }
> return 0;
> @@ -1962,7 +1961,7 @@ static int convert_context(u32 key,
> c->len, , SECSID_NULL);
> kfree(s);
> if (!rc) {
> - printk(KERN_INFO "SELinux: Context %s became valid
> (mapped).\n",
> + pr_info("SELinux: Context %s became valid
> (mapped).\n",
>c->str);
> /* Replace string with mapped representation. */
> kfree(c->str);
> @@ -1974,7 +1973,7 @@ static int convert_context(u32 key,
> goto out;
> } else {
> /* Other error condition, e.g. ENOMEM. */
> - printk(KERN_ERR "SELinux: Unable to map context %s,
> rc = %d.\n",
> + pr_err("SELinux: Unable to map context %s, rc =
> %d.\n",
>c->str, -rc);
> goto out;
> }
> @@ -2033,7 +2032,7 @@ static int convert_context(u32 key,
> oc = oc->next;
> rc = -EINVAL;
> if (!oc) {
> - printk(KERN_ERR "SELinux: unable to look up"
> + pr_err("SELinux: unable to look up"
> " the initial SIDs list\n");
> goto bad;
> }
> @@ -2065,7 +2064,7 @@ static int convert_context(u32 key,
> context_destroy(c);
> c->str = s;
> c->len = len;
> - printk(KERN_INFO "SELinux: Context %s became invalid (unmapped).\n",
> + pr_info("SELinux: Context %s became invalid (unmapped).\n",
>c->str);
> rc = 0;
> goto out;
> @@ -2170,13 +2169,13 @@ int security_load_policy(struct selinux_state *state,
> void *data, size_t len)
> newpolicydb->len = len;
> /* If switching between different policy types, log MLS status */
> if (policydb->mls_enabled && !newpolicydb->mls_enabled)
> - printk(KERN_INFO "SELinux: Disabling MLS support...\n");
> + pr_info("SELinux: Disabling MLS support...\n");
> else if (!policydb->mls_enabled && newpolicydb->mls_enabled)
> - printk(KERN_INFO "SELinux: Enabling MLS support...\n");
> + pr_info("SELinux: Enabling MLS support...\n");
>
> rc = policydb_load_isids(newpolicydb, );
> if (rc) {
> - printk(KERN_ERR "SELinux: unable to load the initial
> SIDs\n");
> + pr_err("SELinux: unable to load the initial SIDs\n");
> policydb_destroy(newpolicydb);
> goto out;
> }
> @@ -2187,7 +2186,7 @@ int security_load_policy(struct selinux_state *state,
> void *data, size_t len)
>
> rc = security_preserve_bools(state, newpolicydb);
> if (rc) {
> - printk(KERN_ERR "SELinux: unable to preserve booleans\n");
> + pr_err("SELinux: unable to preserve booleans\n");
> goto err;
> }
>
> @@ -2207,7 +2206,7 @@ int security_load_policy(struct selinux_state *state,
> void *data, size_t len)
> args.newp = newpolicydb;
> rc = sidtab_map(, convert_context, );
> if (rc) {
> - printk(KERN_ERR "SELinux: unable to convert the internal"
> + pr_err("SELinux: unable to convert the internal"
> " representation of contexts in the new SID"
> " table\n");
> goto err;
> @@ -2999,7 +2998,7 @@ int security_sid_mls_copy(struct selinux_state *state,
> rc = -EINVAL;
> context1 = sidtab_search(sidtab, sid);
> if (!context1) {
> - printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n",
> + pr_err("SELinux: %s: unrecognized SID %d\n",
> __func__, sid);
> goto out_unlock;
> }
> @@ -3007,7 +3006,7 @@ int security_sid_mls_copy(struct selinux_state *state,
> rc = -EINVAL;
> context2 = sidtab_search(sidtab, mls_sid);
> if (!context2) {
> - printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n",
> + pr_err("SELinux: %s: unrecognized SID %d\n",
> __func__, mls_sid);
> goto out_unlock;
> }
> @@ -3104,14 +3103,14 @@ int security_net_peersid_resolve(struct selinux_state
> *state,
> rc = -EINVAL;
> nlbl_ctx = sidtab_search(sidtab, nlbl_sid);
> if (!nlbl_ctx) {
> - printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n",
> + pr_err("SELinux: %s: unrecognized SID %d\n",
>__func__, nlbl_sid);
> goto out;
> }
> rc = -EINVAL;
> xfrm_ctx = sidtab_search(sidtab, xfrm_sid);
> if (!xfrm_ctx) {
> - printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n",
> + pr_err("SELinux: %s: unrecognized SID %d\n",
>__func__, xfrm_sid);
> goto out;
> }
> @@ -3202,7 +3201,7 @@ int security_get_permissions(struct selinux_state
> *state,
> rc = -EINVAL;
> match = hashtab_search(policydb->p_classes.table, class);
> if (!match) {
> - printk(KERN_ERR "SELinux: %s: unrecognized class %s\n",
> + pr_err("SELinux: %s: unrecognized class %s\n",
> __func__, class);
> goto out;
> }
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 05/13] selinux: Cleanup printk logging in avtab
ol)
>
> rc = next_entry(buf, fp, sizeof(u32));
> if (rc < 0) {
> - printk(KERN_ERR "SELinux: avtab: truncated table\n");
> + pr_err("SELinux: avtab: truncated table\n");
> goto bad;
> }
> nel = le32_to_cpu(buf[0]);
> if (!nel) {
> - printk(KERN_ERR "SELinux: avtab: table is empty\n");
> + pr_err("SELinux: avtab: table is empty\n");
> rc = -EINVAL;
> goto bad;
> }
> @@ -580,9 +581,9 @@ int avtab_read(struct avtab *a, void *fp, struct policydb
> *pol)
> rc = avtab_read_item(a, fp, pol, avtab_insertf, NULL);
> if (rc) {
> if (rc == -ENOMEM)
> - printk(KERN_ERR "SELinux: avtab: out of
> memory\n");
> + pr_err("SELinux: avtab: out of memory\n");
> else if (rc == -EEXIST)
> - printk(KERN_ERR "SELinux: avtab: duplicate
> entry\n");
> + pr_err("SELinux: avtab: duplicate entry\n");
>
> goto bad;
> }
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 02/13] selinux: Cleanup printk logging in ebitmap
On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
wrote:
>
> Replace printk with pr_* to avoid checkpatch warnings.
>
> Signed-off-by: Peter Enderborg
> ---
> security/selinux/ss/ebitmap.c | 15 +++
> 1 file changed, 7 insertions(+), 8 deletions(-)
Merged, thanks.
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 5ae8c61b75bf..8f624f80055b 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -362,7 +362,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> count = le32_to_cpu(buf[2]);
>
> if (mapunit != BITS_PER_U64) {
> - printk(KERN_ERR "SELinux: ebitmap: map size %u does not "
> + pr_err("SELinux: ebitmap: map size %u does not "
>"match my size %zd (high bit was %d)\n",
>mapunit, BITS_PER_U64, e->highbit);
> goto bad;
> @@ -383,19 +383,19 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> for (i = 0; i < count; i++) {
> rc = next_entry(, fp, sizeof(u32));
> if (rc < 0) {
> - printk(KERN_ERR "SELinux: ebitmap: truncated map\n");
> + pr_err("SELinux: ebitmap: truncated map\n");
> goto bad;
> }
> startbit = le32_to_cpu(startbit);
>
> if (startbit & (mapunit - 1)) {
> - printk(KERN_ERR "SELinux: ebitmap start bit (%d) is "
> + pr_err("SELinux: ebitmap start bit (%d) is "
>"not a multiple of the map unit size (%u)\n",
>startbit, mapunit);
> goto bad;
> }
> if (startbit > e->highbit - mapunit) {
> - printk(KERN_ERR "SELinux: ebitmap start bit (%d) is "
> + pr_err("SELinux: ebitmap start bit (%d) is "
>"beyond the end of the bitmap (%u)\n",
>startbit, (e->highbit - mapunit));
> goto bad;
> @@ -405,8 +405,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> struct ebitmap_node *tmp;
> tmp = kmem_cache_zalloc(ebitmap_node_cachep,
> GFP_KERNEL);
> if (!tmp) {
> - printk(KERN_ERR
> - "SELinux: ebitmap: out of memory\n");
> + pr_err("SELinux: ebitmap: out of memory\n");
> rc = -ENOMEM;
> goto bad;
> }
> @@ -418,7 +417,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> e->node = tmp;
> n = tmp;
> } else if (startbit <= n->startbit) {
> - printk(KERN_ERR "SELinux: ebitmap: start bit %d"
> + pr_err("SELinux: ebitmap: start bit %d"
>" comes after start bit %d\n",
>startbit, n->startbit);
> goto bad;
> @@ -426,7 +425,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
>
> rc = next_entry(, fp, sizeof(u64));
> if (rc < 0) {
> - printk(KERN_ERR "SELinux: ebitmap: truncated map\n");
> + pr_err("SELinux: ebitmap: truncated map\n");
> goto bad;
> }
> map = le64_to_cpu(map);
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 04/13] selinux: Cleanup printk logging in hooks
__func__, context, -rc, dev,
> ino);
> }
> @@ -1772,8 +1771,7 @@ static int cred_has_capability(const struct cred *cred,
> sclass = initns ? SECCLASS_CAPABILITY2 : SECCLASS_CAP2_USERNS;
> break;
> default:
> - printk(KERN_ERR
> - "SELinux: out of range capability %d\n", cap);
> + pr_err("SELinux: out of range capability %d\n", cap);
> BUG();
> return -EINVAL;
> }
> @@ -2016,7 +2014,7 @@ static int may_link(struct inode *dir,
> av = DIR__RMDIR;
> break;
> default:
> - printk(KERN_WARNING "SELinux: %s: unrecognized kind %d\n",
> + pr_warn("SELinux: %s: unrecognized kind %d\n",
> __func__, kind);
> return 0;
> }
> @@ -2862,7 +2860,7 @@ static int selinux_sb_remount(struct super_block *sb,
> void *data)
> mount_options[i], ,
> GFP_KERNEL);
> if (rc) {
> - printk(KERN_WARNING "SELinux:
> security_context_str_to_sid"
> + pr_warn("SELinux: security_context_str_to_sid"
>"(%s) failed for (dev %s, type %s) errno=%d\n",
>mount_options[i], sb->s_id, sb->s_type->name,
> rc);
> goto out_free_opts;
> @@ -2901,7 +2899,7 @@ static int selinux_sb_remount(struct super_block *sb,
> void *data)
> free_secdata(secdata);
> return rc;
> out_bad_option:
> - printk(KERN_WARNING "SELinux: unable to change security options "
> + pr_warn("SELinux: unable to change security options "
>"during remount (dev %s, type=%s)\n", sb->s_id,
>sb->s_type->name);
> goto out_free_opts;
> @@ -3343,7 +3341,7 @@ static void selinux_inode_post_setxattr(struct dentry
> *dentry, const char *name,
> rc = security_context_to_sid_force(_state, value, size,
>);
> if (rc) {
> - printk(KERN_ERR "SELinux: unable to map context to SID"
> + pr_err("SELinux: unable to map context to SID"
>"for (%s, %lu), rc=%d\n",
>inode->i_sb->s_id, inode->i_ino, -rc);
> return;
> @@ -4406,7 +4404,7 @@ static int selinux_parse_skb(struct sk_buff *skb,
> struct common_audit_data *ad,
> }
>
> parse_error:
> - printk(KERN_WARNING
> + pr_warn(
>"SELinux: failure in selinux_parse_skb(),"
>" unable to parse packet\n");
> return ret;
> @@ -4449,7 +4447,7 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb,
> u16 family, u32 *sid)
> err = security_net_peersid_resolve(_state, nlbl_sid,
>nlbl_type, xfrm_sid, sid);
> if (unlikely(err)) {
> - printk(KERN_WARNING
> + pr_warn(
>"SELinux: failure in selinux_skb_peerlbl_sid(),"
>" unable to determine packet's peer label\n");
> return -EACCES;
> @@ -7091,11 +7089,11 @@ static __init int selinux_init(void)
> }
>
> if (!selinux_enabled) {
> - printk(KERN_INFO "SELinux: Disabled at boot.\n");
> + pr_info("SELinux: Disabled at boot.\n");
> return 0;
> }
>
> - printk(KERN_INFO "SELinux: Initializing.\n");
> + pr_info("SELinux: Initializing.\n");
>
> memset(_state, 0, sizeof(selinux_state));
> enforcing_set(_state, selinux_enforcing_boot);
> @@ -7131,9 +7129,9 @@ static __init int selinux_init(void)
> panic("SELinux: Unable to register AVC LSM notifier
> callback\n");
>
> if (selinux_enforcing_boot)
> - printk(KERN_DEBUG "SELinux: Starting in enforcing mode\n");
> + pr_debug("SELinux: Starting in enforcing mode\n");
> else
> - printk(KERN_DEBUG "SELinux: Starting in permissive mode\n");
> + pr_debug("SELinux: Starting in permissive mode\n");
>
> return 0;
> }
> @@ -7145,10 +7143,10 @@ static void delayed_superblock_init(struct
> super_block *sb, void *unused)
Re: [PATCH 03/13] selinux: Cleanup printk logging in policydb
On Tue, Jun 19, 2018 at 12:45 PM Joe Perches wrote:
>
> On Tue, 2018-06-19 at 12:41 -0400, Paul Moore wrote:
> > On Tue, Jun 12, 2018 at 4:09 AM Peter Enderborg
> > wrote:
> > >
> > > Replace printk with pr_* to avoid checkpatch warnings and
> > > replace KERN_CONT with 2 longer prints.
> > >
> > > Signed-off-by: Peter Enderborg
> > > ---
> > > security/selinux/ss/policydb.c | 91
> > > +-
> > > 1 file changed, 46 insertions(+), 45 deletions(-)
> >
> > Merged, thank you. While removing the separate KERN_CONT message
> > introduces some duplication, I think that's the right thing to do.
> >
> > > diff --git a/security/selinux/ss/policydb.c
> > > b/security/selinux/ss/policydb.c
> []
> > > @@ -504,7 +504,7 @@ static void hash_eval(struct hashtab *h, const char
> > > *hash_name)
> > > struct hashtab_info info;
> > >
> > > hashtab_stat(h, );
> > > - printk(KERN_DEBUG "SELinux: %s: %d entries and %d/%d buckets
> > > used, "
> > > + pr_debug("SELinux: %s: %d entries and %d/%d buckets used, "
> > >"longest chain length %d\n", hash_name, h->nel,
> > >info.slots_used, h->size, info.max_chain_len);
> > > }
> > > @@ -533,15 +533,17 @@ static int policydb_index(struct policydb *p)
> > > {
> > > int i, rc;
> > >
> > > - printk(KERN_DEBUG "SELinux: %d users, %d roles, %d types, %d
> > > bools",
> > > - p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim,
> > > p->p_bools.nprim);
> > > if (p->mls_enabled)
> > > - printk(KERN_CONT ", %d sens, %d cats", p->p_levels.nprim,
> > > - p->p_cats.nprim);
> > > - printk(KERN_CONT "\n");
> > > + pr_debug("SELinux: %d users, %d roles, %d types, %d
> > > bools, %d sens, %d cats",
> > > +p->p_users.nprim, p->p_roles.nprim,
> > > p->p_types.nprim,
> > > + p->p_bools.nprim, p->p_levels.nprim,
> > > p->p_cats.nprim);
> > > + else
> > > + pr_debug("SELinux: %d users, %d roles, %d types, %d
> > > bools",
> > > +p->p_users.nprim, p->p_roles.nprim,
> > > p->p_types.nprim,
> > > +p->p_bools.nprim);
>
> This lost the terminating newline on each pr_debug
Good catch. I haven't pushed to selinux/next yet, and this is pretty
minor, so I'll just fix that up in the merge.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 03/13] selinux: Cleanup printk logging in policydb
gt; @@ -2304,7 +2305,7 @@ int policydb_read(struct policydb *p, void *fp)
> rc = -EINVAL;
> len = le32_to_cpu(buf[1]);
> if (len != strlen(POLICYDB_STRING)) {
> - printk(KERN_ERR "SELinux: policydb string length %d does not
> "
> + pr_err("SELinux: policydb string length %d does not "
>"match expected length %zu\n",
>len, strlen(POLICYDB_STRING));
> goto bad;
> @@ -2313,14 +2314,14 @@ int policydb_read(struct policydb *p, void *fp)
> rc = -ENOMEM;
> policydb_str = kmalloc(len + 1, GFP_KERNEL);
> if (!policydb_str) {
> - printk(KERN_ERR "SELinux: unable to allocate memory for
> policydb "
> + pr_err("SELinux: unable to allocate memory for policydb "
>"string of length %d\n", len);
> goto bad;
> }
>
> rc = next_entry(policydb_str, fp, len);
> if (rc) {
> - printk(KERN_ERR "SELinux: truncated policydb string
> identifier\n");
> + pr_err("SELinux: truncated policydb string identifier\n");
> kfree(policydb_str);
> goto bad;
> }
> @@ -2328,7 +2329,7 @@ int policydb_read(struct policydb *p, void *fp)
> rc = -EINVAL;
> policydb_str[len] = '\0';
> if (strcmp(policydb_str, POLICYDB_STRING)) {
> - printk(KERN_ERR "SELinux: policydb string %s does not match "
> + pr_err("SELinux: policydb string %s does not match "
>"my string %s\n", policydb_str, POLICYDB_STRING);
> kfree(policydb_str);
> goto bad;
> @@ -2346,7 +2347,7 @@ int policydb_read(struct policydb *p, void *fp)
> p->policyvers = le32_to_cpu(buf[0]);
> if (p->policyvers < POLICYDB_VERSION_MIN ||
> p->policyvers > POLICYDB_VERSION_MAX) {
> - printk(KERN_ERR "SELinux: policydb version %d does not match
> "
> + pr_err("SELinux: policydb version %d does not match "
>"my version range %d-%d\n",
>le32_to_cpu(buf[0]), POLICYDB_VERSION_MIN,
> POLICYDB_VERSION_MAX);
> goto bad;
> @@ -2357,7 +2358,7 @@ int policydb_read(struct policydb *p, void *fp)
>
> rc = -EINVAL;
> if (p->policyvers < POLICYDB_VERSION_MLS) {
> - printk(KERN_ERR "SELinux: security policydb version
> %d "
> + pr_err("SELinux: security policydb version %d "
> "(MLS) not backwards compatible\n",
> p->policyvers);
> goto bad;
> @@ -2381,7 +2382,7 @@ int policydb_read(struct policydb *p, void *fp)
> rc = -EINVAL;
> info = policydb_lookup_compat(p->policyvers);
> if (!info) {
> - printk(KERN_ERR "SELinux: unable to find policy compat info "
> + pr_err("SELinux: unable to find policy compat info "
>"for version %d\n", p->policyvers);
> goto bad;
> }
> @@ -2389,7 +2390,7 @@ int policydb_read(struct policydb *p, void *fp)
> rc = -EINVAL;
> if (le32_to_cpu(buf[2]) != info->sym_num ||
> le32_to_cpu(buf[3]) != info->ocon_num) {
> - printk(KERN_ERR "SELinux: policydb table sizes (%d,%d) do "
> + pr_err("SELinux: policydb table sizes (%d,%d) do "
>"not match mine (%d,%d)\n", le32_to_cpu(buf[2]),
> le32_to_cpu(buf[3]),
>info->sym_num, info->ocon_num);
> @@ -3417,7 +3418,7 @@ int policydb_write(struct policydb *p, void *fp)
> * careful if you ever try to remove this restriction
> */
> if (p->policyvers < POLICYDB_VERSION_AVTAB) {
> - printk(KERN_ERR "SELinux: refusing to write policy version
> %d."
> + pr_err("SELinux: refusing to write policy version %d."
>" Because it is less than version %d\n",
> p->policyvers,
>POLICYDB_VERSION_AVTAB);
> return -EINVAL;
> @@ -3446,7 +3447,7 @@ int policydb_write(struct policydb *p, void *fp)
> /* Write the version, config, and table sizes. */
> info = policydb_lookup_compat(p->policyvers);
> if (!info) {
> - printk(KERN_ERR "SELinux: compatibility lookup failed for
> policy "
> + pr_err("SELinux: compatibility lookup failed for policy "
> "version %d", p->policyvers);
> return -EINVAL;
> }
> --
> 2.15.1
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 01/13] selinux: Cleanup printk logging in conditional
On Wed, Jun 13, 2018 at 2:23 AM peter enderborg wrote: > On 06/12/2018 04:38 PM, Joe Perches wrote: > > On Tue, 2018-06-12 at 10:09 +0200, Peter Enderborg wrote: > >> Replace printk with pr_* to avoid checkpatch warnings. > > I believe it would be nicer to remove the > > "SELinux: " prefix embbeded in each format > > and use a specific > > > > #define pr_fmt(fmt) "SELinux: " fmt > > > > to automatically prefix these formats. > > I cant argument about that, however some of the warnings and debug prints in > this set does not have this > so it will then change the actual output. (And I also think that they should > have a the prefix, but I don't > know why they don't) So I am not sure if it appropriate for a cleanup patch, > it supposed to have no functional change. As others have mentioned, I think this patch is still a step forward so I'm going to go ahead and merge it; thanks Peter. As far as the prefix, or lack of, is concerned, that's probably an oversight that we should fix at some point, but we would need to look at each instance to verify. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] MAINTAINERS: update the LSM and SELinux subsystems
From: Paul Moore The SELinux code, security/selinux/, already has a MAINTAINERS entry so exclude it from the security subsystem entry in an effort to better reflect current practices. Signed-off-by: Paul Moore --- MAINTAINERS |1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9d5eeff51b5f..67e4c4f92ba9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12769,6 +12769,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git W: http://kernsec.org/ S: Supported F: security/ +X: security/selinux/ SELINUX SECURITY MODULE M: Paul Moore ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
selinux/next rebased to v4.18-rc1
A quick note to let you know that I've rebased selinux/next on top of v4.18-rc1, and now that merge window is closed I'm going to be working my way through the patch backlog this week. You may also notice that there is now a README.md in the selinux/master branch with some basic information and links. Eventually I'm going to add administrative information, e.g. the SELinux kernel process[1], to this file so we have it in a central location, but if there is anything else you can think of that should be in this file let me know. [1] http://www.paul-moore.com/blog/d/2017/07/kernel_repo_process.html -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, Jun 13, 2018 at 5:14 PM, Casey Schaufler wrote: > On 6/13/2018 12:57 PM, Paul Moore wrote: >> On Wed, Jun 13, 2018 at 3:30 PM, Joe Perches wrote: >>> On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote: >>>> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches wrote: >>>>> On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: >>>>>> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: >>>>>>> On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: ... >>> If James is not approving or merging security/selinux or >>> security/tomoyo then perhaps the F: entries could be >>> augmented with appropriate X: entries or made specific >>> by using specific entries like: >>> >>> F: security/* >>> F: security/integrity/ >>> F: security/keys/ > > There are already F: entries for security/selinux, security/smack > and security/apparmor so I don't get your point. Perhaps I've interpreted this the wrong way, but I took this to mean that those security subsystems which don't flow through James should use the X: entry to exclude themselves. For example, here is a quick diff to exclude SELinux: diff --git a/MAINTAINERS b/MAINTAINERS index c13b9fb3be0b..dc0b31121459 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12771,6 +12771,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/g> W: http://kernsec.org/ S: Supported F: security/ +X: security/selinux/ SELINUX SECURITY MODULE M: Paul Moore -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, Jun 13, 2018 at 3:30 PM, Joe Perches wrote: > On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote: >> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches wrote: >> > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: >> > > On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: >> > > > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: >> > > > > Joe, in general I really appreciate the fixes you send, but these >> > > > > patches that cross a lot of subsystem boundaries (this isn't the >> > > > > first >> > > > > one that does this) causes unnecessary conflicts in -next and during >> > > > > the merge window. Could you split your patches up from now on >> > > > > please? >> > > > >> > > > Sorry. No. Merge conflicts are inherent in this system. >> > > >> > > Yes, merge conflicts are inherent in this system when one makes a >> > > single change which impacts multiple subsystems, e.g. changing a core >> > > kernel function which is called by multiple subsystems. However, that >> > > isn't what this patch does, it makes a number of self-contained >> > > changes across multiple subsystems; there are no cross-subsystem >> > > dependencies in this patch. You are increasing the likelihood of >> > > conflicts for no good reason; that is why I'm asking you to split this >> > > patch and others like it. >> > >> > No. History shows with high certainty that splitting >> > patches like this across multiple subsystems of a primary >> > subsystem means that the entire patchset is not completely >> > applied. >> >> I think that is due more to a lack of effort on the part of the patch >> author to keep pushing the individual patches. > > Nope. Try again. > > Resistance to change and desire for status quo > occurs in many subsystems. Which gets back to the need for persistence on the part of the patch author. If your solution to a stubborn susbsystem is to go around them by convincing another, potentially unrelated subsystem, to merge the patch then I firmly believe you are doing it wrong. >> > It's _much_ simpler and provides a generic mechanism to >> > get the entire patch applied to send a single patch to the >> > top level subsystem maintainer. >> >> I understand it is simpler for you, but it is more difficult for everyone >> else. > > Not true. I think we are at the agree to disagree stage. The way you have structured this patch it makes it easier for you to submit, but makes it potentially more difficult for me (likely other LSM maintainers too), the -next folks, and Linus. > It's simply a matter of merge resolution being pushed down > where and when necessary. > > See changes like the additions of the SPDX license tags. Please don't even try to suggest that this trivial patch you are proposing is even remotely as significant as the SPDX change. There are always going to be exceptions to every rule, and with each exception there needs to be a solid reason behind the change. The SPDX change had a legitimate reason (legal concern) for doing it the way it was done; this patch isn't close to that level of concern. >> Further, where the LSMs are concerned, there is no "top level >> subsystem maintainer" anymore. SELinux and AppArmor send pull >> requests directly to Linus. > > MAINTAINERS-SECURITY SUBSYSTEM > MAINTAINERS-M: James Morris > MAINTAINERS-M: "Serge E. Hallyn" > MAINTAINERS-L: linux-security-mod...@vger.kernel.org (suggested Cc:) > MAINTAINERS-T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git > MAINTAINERS-W: http://kernsec.org/ > MAINTAINERS-S: Supported > MAINTAINERS:F: security/ > MAINTAINERS- > > If James is not approving or merging security/selinux or > security/tomoyo then perhaps the F: entries could be > augmented with appropriate X: entries or made specific > by using specific entries like: > > F: security/* > F: security/integrity/ > F: security/keys/ That is a good point. I'll put together a patch for selinux/next as soon as the merge window closes. I'll let the other LSMs do as they see fit. As I said previously, I believe the only other LSM that sends directly to Linux is AppArmor. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux-testsuite: Enhance inet_socket tests
On Wed, Jun 13, 2018 at 12:46 PM, Richard Haines
wrote:
> On Tue, 2018-06-12 at 18:02 -0400, Paul Moore wrote:
>> On Fri, Apr 13, 2018 at 6:13 AM, Richard Haines via Selinux
>> wrote:
>> > Enhance the tests as follows:
>> > 1) Determine number of tests to run with current config.
>> > 2) Add CALIPSO STREAM tests (DGRAM not supported in kernel. See
>> > [1]).
>> > 3) Add support for CIPSO TAGS 1 & 2. Closes [2].
>> > 4) Run scripts using /bin/sh.
>> > 5) Shorten sleep time as more tests.
>> >
>> > [1] https://github.com/SELinuxProject/selinux-kernel/issues/24
>> > [2] https://github.com/SELinuxProject/selinux-testsuite/issues/1
>> >
>> > Signed-off-by: Richard Haines
>> > ---
>> > tests/inet_socket/calipso-flush | 5 +
>> > tests/inet_socket/calipso-load | 7 +
>> > tests/inet_socket/cipso-fl-flush| 0
>> > tests/inet_socket/cipso-fl-load | 0
>> > tests/inet_socket/cipso-flush | 0
>> > tests/inet_socket/cipso-load-t1 | 11 +
>> > tests/inet_socket/cipso-load-t2 | 11 +
>> > tests/inet_socket/{cipso-load => cipso-load-t5} | 0
>> > tests/inet_socket/ipsec-flush | 0
>> > tests/inet_socket/ipsec-load| 0
>> > tests/inet_socket/iptables-flush| 0
>> > tests/inet_socket/iptables-load | 0
>> > tests/inet_socket/server.c | 16 +-
>> > tests/inet_socket/test | 348
>> > ++--
>> > 14 files changed, 310 insertions(+), 88 deletions(-)
>> > create mode 100644 tests/inet_socket/calipso-flush
>> > create mode 100644 tests/inet_socket/calipso-load
>> > mode change 100755 => 100644 tests/inet_socket/cipso-fl-flush
>> > mode change 100755 => 100644 tests/inet_socket/cipso-fl-load
>> > mode change 100755 => 100644 tests/inet_socket/cipso-flush
>> > create mode 100644 tests/inet_socket/cipso-load-t1
>> > create mode 100644 tests/inet_socket/cipso-load-t2
>> > rename tests/inet_socket/{cipso-load => cipso-load-t5} (100%)
>> > mode change 100755 => 100644
>> > mode change 100755 => 100644 tests/inet_socket/ipsec-flush
>> > mode change 100755 => 100644 tests/inet_socket/ipsec-load
>> > mode change 100755 => 100644 tests/inet_socket/iptables-flush
>> > mode change 100755 => 100644 tests/inet_socket/iptables-load
>> > mode change 100755 => 100644 tests/inet_socket/test
>>
>> I had to fixup the file mode bits on tests/inet_socket/test, but
>> other
>> than that this looks fine to me, merged. Thanks.
>
> The reason I have not been setting +x on the tests/*/test scripts is
> that the tests/Makefile does it for you. However as all the others are
> set, I'll set +x in future (as you flagged this on the sctp and binder
> patches I sent).
Please do. The issue is that whenever you run the tests it changes
the mode bits from how they are in the git repository. While not
really a problem for people who just take a snapshot of the tests, it
does cause problems for those of us who push/pull from the repo as it
registers as a change (check "git status").
>> I remain a little wary about the reduced sleep times (1s to 0.25s),
>> but I'm never comfortable with arbitrary sleep-and-hope-it-works
>> tricks anyway.
>
> I've been using this value in the SCTP tests for some time and not had
> any problems, that's why I used it for the inet tests (probably better
> to have the client try connecting x times and do away with the wait)
It's working on my test VMs, so from a selfish point of view I'm fine
with it for right now :) My concern isn't from an observed failure
with the change, but rather bad experiences with similar approaches on
other projects. In other words, I'm just being cranky.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches wrote: > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: >> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: >> > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: >> > > Joe, in general I really appreciate the fixes you send, but these >> > > patches that cross a lot of subsystem boundaries (this isn't the first >> > > one that does this) causes unnecessary conflicts in -next and during >> > > the merge window. Could you split your patches up from now on please? >> > >> > Sorry. No. Merge conflicts are inherent in this system. >> >> Yes, merge conflicts are inherent in this system when one makes a >> single change which impacts multiple subsystems, e.g. changing a core >> kernel function which is called by multiple subsystems. However, that >> isn't what this patch does, it makes a number of self-contained >> changes across multiple subsystems; there are no cross-subsystem >> dependencies in this patch. You are increasing the likelihood of >> conflicts for no good reason; that is why I'm asking you to split this >> patch and others like it. > > No. History shows with high certainty that splitting > patches like this across multiple subsystems of a primary > subsystem means that the entire patchset is not completely > applied. I think that is due more to a lack of effort on the part of the patch author to keep pushing the individual patches. > It's _much_ simpler and provides a generic mechanism to > get the entire patch applied to send a single patch to the > top level subsystem maintainer. I understand it is simpler for you, but it is more difficult for everyone else. Further, where the LSMs are concerned, there is no "top level subsystem maintainer" anymore. SELinux and AppArmor send pull requests directly to Linus. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: >> Joe, in general I really appreciate the fixes you send, but these >> patches that cross a lot of subsystem boundaries (this isn't the first >> one that does this) causes unnecessary conflicts in -next and during >> the merge window. Could you split your patches up from now on please? > > Sorry. No. Merge conflicts are inherent in this system. Yes, merge conflicts are inherent in this system when one makes a single change which impacts multiple subsystems, e.g. changing a core kernel function which is called by multiple subsystems. However, that isn't what this patch does, it makes a number of self-contained changes across multiple subsystems; there are no cross-subsystem dependencies in this patch. You are increasing the likelihood of conflicts for no good reason; that is why I'm asking you to split this patch and others like it. > There is just no good way to do this as sending a set > of per subsystem patches guarantees partial application > of the entire set. Please explain why all of these changes need to be made at the same time? Looking quickly at the patch it would appear that each chunk of the patch could be applied independently and the kernel would still build and operate correctly. I'm not suggesting that you decompose this patch with that level of granularity, that would be silly, but splitting this patch (and many of the others you tend to submit) at subsystem boundaries should be possible. Further, as one could see from the responses of the other LSM maintainers, splitting this patch is not just possible, it is desirable. > If you prefer, each sub-subsystem maintainer, at > whatever granularity desired, could apply the patch > to the appropriate subsystem by using > "git am --include=". Or you could split the patch up by subsystem before posting, like so many other developers do already. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Tue, Jun 12, 2018 at 4:32 PM, James Morris wrote: > On Mon, 11 Jun 2018, Casey Schaufler wrote: > >> If you want to break this up by security module I would take >> the Smack part as soon as James does the tree update. If James >> wants to take the whole thing at once you can add my: >> >> Acked-by: Casey Schaufler >> >> for the Smack changes. > > It's probably simplest for me to take them as one patch. I would prefer if the SELinux changes were split into a separate patch. I'm guessing John would probably want the same for the AppArmor patches, but take his work for it, not mine. Joe, in general I really appreciate the fixes you send, but these patches that cross a lot of subsystem boundaries (this isn't the first one that does this) causes unnecessary conflicts in -next and during the merge window. Could you split your patches up from now on please? -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux-testsuite: Enhance inet_socket tests
uest peer
> context.
> -if ( ( $pid = fork() ) == 0 ) {
> -exec "runcon -t test_inet_server_t $basedir/server -n dgram 65535";
> -}
> +if ( ( $pid = fork() ) == 0 ) {
> +exec "runcon -t test_inet_server_t $basedir/server -n dgram 65535";
> +}
>
> -sleep 1;# Give it a moment to initialize
> +select( undef, undef, undef, 0.25 );# Give it a moment to initialize
>
> -# This test now passes.
> -$result = system
> - "runcon -t test_inet_client_t $basedir/client -e nopeer dgram ::1 65535";
> -ok( $result eq 0 );
> +# This test now passes.
> +$result = system
> + "runcon -t test_inet_client_t $basedir/client -e nopeer dgram ::1
> 65535";
> +ok( $result eq 0 );
>
> -# Kill the server.
> -kill TERM, $pid;
> +# Kill the server.
> +kill TERM, $pid;
>
> -# Flush IPSEC configuration.
> -system "$basedir/ipsec-flush";
> +# Flush IPSEC configuration.
> +system "/bin/sh $basedir/ipsec-flush";
> +}
>
> # Load iptables (IPv4 & IPv6) configuration.
> -system "$basedir/iptables-load";
> +system "/bin/sh $basedir/iptables-load";
>
> # Start the stream server.
> if ( ( $pid = fork() ) == 0 ) {
> exec "runcon -t test_inet_server_t -- $basedir/server -n stream 65535";
> }
>
> -sleep 1;# Give it a moment to initialize.
> +select( undef, undef, undef, 0.25 );# Give it a moment to initialize.
>
> # Verify that authorized client can communicate with the server.
> $result = system
> @@ -265,7 +413,7 @@ if ( ( $pid = fork() ) == 0 ) {
> exec "runcon -t test_inet_server_t $basedir/server -n dgram 65535";
> }
>
> -sleep 1;# Give it a moment to initialize
> +select( undef, undef, undef, 0.25 );# Give it a moment to initialize
>
> # Verify that authorized client can communicate with the server.
> $result = system
> @@ -291,6 +439,40 @@ ok( $result >> 8 eq 8 );
> kill TERM, $pid;
>
> # Flush iptables configuration.
> -system "$basedir/iptables-flush";
> +system "/bin/sh $basedir/iptables-flush";
> +
> +if ($test_calipso_stream) {
> +
> +# Load NetLabel configuration for CALIPSO/IPv6 labeling over loopback.
> +system "/bin/sh $basedir/calipso-load";
> +
> +# Start the stream server.
> +if ( ( $pid = fork() ) == 0 ) {
> +exec
> +"runcon -t test_inet_server_t -l s0:c0.c10 $basedir/server stream 65535";
> +}
> +
> +select( undef, undef, undef, 0.25 );# Give it a moment to initialize.
> +
> +# Verify that authorized client can communicate with the server.
> +$result = system
> +"runcon -t test_inet_client_t -l s0:c0.c10 $basedir/client -e
> system_u:object_r:netlabel_peer_t:s0:c0.c10 stream ::1 65535";
> +ok( $result eq 0 );
> +
> +# Verify that authorized client can communicate with the server using
> different valid level.
> +$result = system
> +"runcon -t test_inet_client_t -l s0:c8.c10 $basedir/client -e
> system_u:object_r:netlabel_peer_t:s0:c8.c10 stream ::1 65535";
> +ok( $result eq 0 );
> +
> +# Verify that authorized client cannot communicate with the server using
> invalid level.
> +$result = system
> +"runcon -t test_inet_client_t -l s0:c8.c12 -- $basedir/client stream ::1
> 65535 2>&1";
> +ok( $result >> 8 eq 5 );
> +
> +# Kill the stream server.
> +kill TERM, $pid;
> +
> +system "/bin/sh $basedir/calipso-flush";
> +}
>
> exit;
> --
> 2.14.3
>
>
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: selinux-testsuite inet_socket test failure
On Sun, Jun 10, 2018 at 11:55 AM, Paul Moore wrote: > On Sat, Jun 9, 2018 at 1:12 AM, Sgeeta Dhundale wrote: >> Thank you Paul for looking at the issue. >> Yes I am using RHEL6.9/6.10 and OL6.9/6.10. >> I would wait for the fix, hope it will be fixed soon. > > As a FYI, I believe this is simply an issue to be worked around using > the selinux-testsuites's SELinux policy, I don't believe this is a > problem with the kernel or userspace on RHEL-6.x based systems. > > I have some time set aside on Monday and Tuesday to work on SELinux > policy, I'm hopeful that I'll have a fix then. FYI, this should now be fixed in the selinux-testsuite repository, if you continue to see problems let us know. >> On Fri, Jun 8, 2018 at 10:11 PM, Paul Moore wrote: >>> >>> On Fri, Jun 8, 2018 at 12:35 PM, Paul Moore wrote: >>> > On Fri, Jun 8, 2018 at 9:17 AM, Sgeeta Dhundale >>> > wrote: >>> >> Hi, >>> >> While running selinux testsuits I am seeing some of the inet_socket >>> >> tests >>> >> failure. >>> >> Googled alotbut couldnt see any similler issue reported as such. >>> >> It would be really helpful if you can give some pointer to resolved >>> >> this. >>> >> >>> >> Selinux rpms I am using => >>> >> # rpm -qa | grep selinux >>> >> libselinux-devel-2.0.94-7.el6.x86_64 >>> >> libselinux-utils-2.0.94-7.el6.x86_64 >>> >> selinux-policy-targeted-3.7.19-312.0.1.el6.noarch >>> >> libselinux-2.0.94-7.el6.x86_64 >>> >> selinux-policy-3.7.19-312.0.1.el6.noarch >>> >> libselinux-python-2.0.94-7.el6.x86_64 >>> >> --- >>> >> >>> >> Output snippet of test run >>> >> chcon -R -t test_file_t . >>> >> Running as user root with context >>> >> unconfined_u:unconfined_r:unconfined_t >>> >> >>> >> . >>> >> .. >>> >> dyntrace/test ok >>> >> bounds/test .. ok >>> >> mmap/test ok >>> >> unix_socket/test . ok >>> >> inet_socket/test . >>> >> Dubious, test returned 2 (wstat 512, 0x200) >>> >> Failed 2/33 subtests >>> >> checkreqprot/test ok >>> >> mqueue/test .. skipped: mqueue fileystem not >>> >> supported/mounted >>> >> mac_admin/test ... ok >>> >> infiniband_pkey/test . ok >>> >> infiniband_endport/test .. ok >>> >> >>> >> Test Summary Report >>> >> --- >>> >> inet_socket/test (Wstat: 512 Tests: 33 Failed: 2) >>> >> Failed tests: 7, 9 >>> >> Non-zero exit status: 2 >>> >> Files=46, Tests=325, 54 wallclock secs ( 0.27 usr 0.10 sys + 0.76 >>> >> cusr >>> >> 1.46 csys = 2.59 CPU) >>> >> Result: FAIL >>> >> make: Leaving directory `/root/SELinux/selinux-testsuite-master/tests' >>> >> ASSERT:SELinux-Test run failed, pls check testrun.log file for details >>> >> expected:<0> but was:<1> >>> >> FAILED >>> > >>> > It looks like you are running RHEL-6.x or CentOS-6.x? >>> > >>> > I just ran the tests on my RHEL-6.x test system and saw similar >>> > results, it appears to be the result of the following commit to the >>> > selinux-testsuite: >>> > >>> > commit c618ab669b0c580bb3fa000b168d7d4b5a00c5ee >>> > Author: Stephen Smalley >>> > Date: Thu Oct 26 09:29:37 2017 -0400 >>> > >>> >selinux-testsuite: inet_socket: tighten checking >>> > >>> >As demonstrated by >>> > https://github.com/SELinuxProject/selinux-kernel/issues/3 >>> >the inet_socket tests can "pass" for the wrong reasons. Change the >>> >client program to use different exit codes for different failures, >>> >and change the test script to check the expected exit code for all >>> > tests. >>> >With this change, getting an unexpected peer label causes a test >>> > failure >>> >rather than being treated identically to a permission denial. >>> > >>> >NB This could make the tests more fragile, e.g. it appears that we >>> > encounter >>> >permission denial failures at different points for different tests, >>> > so we >>> >may need to relax the checking somewhat based on testing a wider >>> > range of >>> >older kernels. >>> > >>> >Signed-off-by: Stephen Smalley >>> > >>> > ... I think we may need to take a closer look at what RHEL-6.x based >>> > kernels are currently doing to ensure they are "correct" (I'm going to >>> > assume yes, but that is an assumption), and perhaps update the test >>> > suite to reflect the RHEL-6.x behavior. >>> >>> FYI, I created an issue on GH to track this: >>> >>> * https://github.com/SELinuxProject/selinux-testsuite/issues/37 >>> >>> -- >>> paul moore >>> www.paul-moore.com >> >> >> >> >> -- >> Regards, >> -Sgeeta > > > > -- > paul moore > www.paul-moore.com -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] selinux-testsuite: fix the inet_socket tests on older policy releases
From: Paul Moore Ensure that we apply MCS constraints to the test_inet_server_t domain, this was causing test failures on RHEL-6.x based systems. Thanks to Stephen Smalley and Lukas Vrabec for some off-list discussion related to this problem and its solution. Signed-off-by: Paul Moore --- policy/test_inet_socket.te | 10 ++ 1 file changed, 10 insertions(+) diff --git a/policy/test_inet_socket.te b/policy/test_inet_socket.te index c25900b..428d28e 100644 --- a/policy/test_inet_socket.te +++ b/policy/test_inet_socket.te @@ -33,6 +33,16 @@ corenet_udp_bind_all_nodes(test_inet_server_t) corenet_inout_generic_if(test_inet_server_t) corenet_inout_generic_node(test_inet_server_t) +# We need to ensure that the test domain is MCS constrained. +## newer systems, e.g. Fedora and RHEL >= 7.x +ifdef(`mcs_constrained', ` + mcs_constrained(test_inet_server_t) +') +## older systems, e.g. RHEL == 6.x +ifdef(`mcs_untrusted_proc', ` + mcs_untrusted_proc(test_inet_server_t) +') + # Domain for client process. type test_inet_client_t; domain_type(test_inet_client_t) ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: selinux-testsuite inet_socket test failure
On Sat, Jun 9, 2018 at 1:12 AM, Sgeeta Dhundale wrote: > Thank you Paul for looking at the issue. > Yes I am using RHEL6.9/6.10 and OL6.9/6.10. > I would wait for the fix, hope it will be fixed soon. As a FYI, I believe this is simply an issue to be worked around using the selinux-testsuites's SELinux policy, I don't believe this is a problem with the kernel or userspace on RHEL-6.x based systems. I have some time set aside on Monday and Tuesday to work on SELinux policy, I'm hopeful that I'll have a fix then. > On Fri, Jun 8, 2018 at 10:11 PM, Paul Moore wrote: >> >> On Fri, Jun 8, 2018 at 12:35 PM, Paul Moore wrote: >> > On Fri, Jun 8, 2018 at 9:17 AM, Sgeeta Dhundale >> > wrote: >> >> Hi, >> >> While running selinux testsuits I am seeing some of the inet_socket >> >> tests >> >> failure. >> >> Googled alotbut couldnt see any similler issue reported as such. >> >> It would be really helpful if you can give some pointer to resolved >> >> this. >> >> >> >> Selinux rpms I am using => >> >> # rpm -qa | grep selinux >> >> libselinux-devel-2.0.94-7.el6.x86_64 >> >> libselinux-utils-2.0.94-7.el6.x86_64 >> >> selinux-policy-targeted-3.7.19-312.0.1.el6.noarch >> >> libselinux-2.0.94-7.el6.x86_64 >> >> selinux-policy-3.7.19-312.0.1.el6.noarch >> >> libselinux-python-2.0.94-7.el6.x86_64 >> >> --- >> >> >> >> Output snippet of test run >> >> chcon -R -t test_file_t . >> >> Running as user root with context >> >> unconfined_u:unconfined_r:unconfined_t >> >> >> >> . >> >> .. >> >> dyntrace/test ok >> >> bounds/test .. ok >> >> mmap/test ok >> >> unix_socket/test . ok >> >> inet_socket/test . >> >> Dubious, test returned 2 (wstat 512, 0x200) >> >> Failed 2/33 subtests >> >> checkreqprot/test ok >> >> mqueue/test .. skipped: mqueue fileystem not >> >> supported/mounted >> >> mac_admin/test ... ok >> >> infiniband_pkey/test . ok >> >> infiniband_endport/test .. ok >> >> >> >> Test Summary Report >> >> --- >> >> inet_socket/test (Wstat: 512 Tests: 33 Failed: 2) >> >> Failed tests: 7, 9 >> >> Non-zero exit status: 2 >> >> Files=46, Tests=325, 54 wallclock secs ( 0.27 usr 0.10 sys + 0.76 >> >> cusr >> >> 1.46 csys = 2.59 CPU) >> >> Result: FAIL >> >> make: Leaving directory `/root/SELinux/selinux-testsuite-master/tests' >> >> ASSERT:SELinux-Test run failed, pls check testrun.log file for details >> >> expected:<0> but was:<1> >> >> FAILED >> > >> > It looks like you are running RHEL-6.x or CentOS-6.x? >> > >> > I just ran the tests on my RHEL-6.x test system and saw similar >> > results, it appears to be the result of the following commit to the >> > selinux-testsuite: >> > >> > commit c618ab669b0c580bb3fa000b168d7d4b5a00c5ee >> > Author: Stephen Smalley >> > Date: Thu Oct 26 09:29:37 2017 -0400 >> > >> >selinux-testsuite: inet_socket: tighten checking >> > >> >As demonstrated by >> > https://github.com/SELinuxProject/selinux-kernel/issues/3 >> >the inet_socket tests can "pass" for the wrong reasons. Change the >> >client program to use different exit codes for different failures, >> >and change the test script to check the expected exit code for all >> > tests. >> >With this change, getting an unexpected peer label causes a test >> > failure >> >rather than being treated identically to a permission denial. >> > >> >NB This could make the tests more fragile, e.g. it appears that we >> > encounter >> >permission denial failures at different points for different tests, >> > so we >> >may need to relax the checking somewhat based on testing a wider >> > range of >> >older kernels. >> > >> >Signed-off-by: Stephen Smalley >> > >> > ... I think we may need to take a closer look at what RHEL-6.x based >> > kernels are currently doing to ensure they are "correct" (I'm going to >> > assume yes, but that is an assumption), and perhaps update the test >> > suite to reflect the RHEL-6.x behavior. >> >> FYI, I created an issue on GH to track this: >> >> * https://github.com/SELinuxProject/selinux-testsuite/issues/37 >> >> -- >> paul moore >> www.paul-moore.com > > > > > -- > Regards, > -Sgeeta -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: selinux-testsuite inet_socket test failure
On Fri, Jun 8, 2018 at 12:35 PM, Paul Moore wrote: > On Fri, Jun 8, 2018 at 9:17 AM, Sgeeta Dhundale wrote: >> Hi, >> While running selinux testsuits I am seeing some of the inet_socket tests >> failure. >> Googled alotbut couldnt see any similler issue reported as such. >> It would be really helpful if you can give some pointer to resolved this. >> >> Selinux rpms I am using => >> # rpm -qa | grep selinux >> libselinux-devel-2.0.94-7.el6.x86_64 >> libselinux-utils-2.0.94-7.el6.x86_64 >> selinux-policy-targeted-3.7.19-312.0.1.el6.noarch >> libselinux-2.0.94-7.el6.x86_64 >> selinux-policy-3.7.19-312.0.1.el6.noarch >> libselinux-python-2.0.94-7.el6.x86_64 >> --- >> >> Output snippet of test run >> chcon -R -t test_file_t . >> Running as user root with context unconfined_u:unconfined_r:unconfined_t >> >> . >> .. >> dyntrace/test ok >> bounds/test .. ok >> mmap/test ok >> unix_socket/test . ok >> inet_socket/test . >> Dubious, test returned 2 (wstat 512, 0x200) >> Failed 2/33 subtests >> checkreqprot/test ok >> mqueue/test .. skipped: mqueue fileystem not supported/mounted >> mac_admin/test ... ok >> infiniband_pkey/test . ok >> infiniband_endport/test .. ok >> >> Test Summary Report >> --- >> inet_socket/test (Wstat: 512 Tests: 33 Failed: 2) >> Failed tests: 7, 9 >> Non-zero exit status: 2 >> Files=46, Tests=325, 54 wallclock secs ( 0.27 usr 0.10 sys + 0.76 cusr >> 1.46 csys = 2.59 CPU) >> Result: FAIL >> make: Leaving directory `/root/SELinux/selinux-testsuite-master/tests' >> ASSERT:SELinux-Test run failed, pls check testrun.log file for details >> expected:<0> but was:<1> >> FAILED > > It looks like you are running RHEL-6.x or CentOS-6.x? > > I just ran the tests on my RHEL-6.x test system and saw similar > results, it appears to be the result of the following commit to the > selinux-testsuite: > > commit c618ab669b0c580bb3fa000b168d7d4b5a00c5ee > Author: Stephen Smalley > Date: Thu Oct 26 09:29:37 2017 -0400 > >selinux-testsuite: inet_socket: tighten checking > >As demonstrated by > https://github.com/SELinuxProject/selinux-kernel/issues/3 >the inet_socket tests can "pass" for the wrong reasons. Change the >client program to use different exit codes for different failures, >and change the test script to check the expected exit code for all tests. >With this change, getting an unexpected peer label causes a test failure >rather than being treated identically to a permission denial. > >NB This could make the tests more fragile, e.g. it appears that we > encounter >permission denial failures at different points for different tests, so we >may need to relax the checking somewhat based on testing a wider range of >older kernels. > >Signed-off-by: Stephen Smalley > > ... I think we may need to take a closer look at what RHEL-6.x based > kernels are currently doing to ensure they are "correct" (I'm going to > assume yes, but that is an assumption), and perhaps update the test > suite to reflect the RHEL-6.x behavior. FYI, I created an issue on GH to track this: * https://github.com/SELinuxProject/selinux-testsuite/issues/37 -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: selinux-testsuite inet_socket test failure
On Fri, Jun 8, 2018 at 9:17 AM, Sgeeta Dhundale wrote: > Hi, > While running selinux testsuits I am seeing some of the inet_socket tests > failure. > Googled alotbut couldnt see any similler issue reported as such. > It would be really helpful if you can give some pointer to resolved this. > > Selinux rpms I am using => > # rpm -qa | grep selinux > libselinux-devel-2.0.94-7.el6.x86_64 > libselinux-utils-2.0.94-7.el6.x86_64 > selinux-policy-targeted-3.7.19-312.0.1.el6.noarch > libselinux-2.0.94-7.el6.x86_64 > selinux-policy-3.7.19-312.0.1.el6.noarch > libselinux-python-2.0.94-7.el6.x86_64 > --- > > Output snippet of test run > chcon -R -t test_file_t . > Running as user root with context unconfined_u:unconfined_r:unconfined_t > > . > .. > dyntrace/test ok > bounds/test .. ok > mmap/test ok > unix_socket/test . ok > inet_socket/test . > Dubious, test returned 2 (wstat 512, 0x200) > Failed 2/33 subtests > checkreqprot/test ok > mqueue/test .. skipped: mqueue fileystem not supported/mounted > mac_admin/test ... ok > infiniband_pkey/test . ok > infiniband_endport/test .. ok > > Test Summary Report > --- > inet_socket/test (Wstat: 512 Tests: 33 Failed: 2) > Failed tests: 7, 9 > Non-zero exit status: 2 > Files=46, Tests=325, 54 wallclock secs ( 0.27 usr 0.10 sys + 0.76 cusr > 1.46 csys = 2.59 CPU) > Result: FAIL > make: Leaving directory `/root/SELinux/selinux-testsuite-master/tests' > ASSERT:SELinux-Test run failed, pls check testrun.log file for details > expected:<0> but was:<1> > FAILED It looks like you are running RHEL-6.x or CentOS-6.x? I just ran the tests on my RHEL-6.x test system and saw similar results, it appears to be the result of the following commit to the selinux-testsuite: commit c618ab669b0c580bb3fa000b168d7d4b5a00c5ee Author: Stephen Smalley Date: Thu Oct 26 09:29:37 2017 -0400 selinux-testsuite: inet_socket: tighten checking As demonstrated by https://github.com/SELinuxProject/selinux-kernel/issues/3 the inet_socket tests can "pass" for the wrong reasons. Change the client program to use different exit codes for different failures, and change the test script to check the expected exit code for all tests. With this change, getting an unexpected peer label causes a test failure rather than being treated identically to a permission denial. NB This could make the tests more fragile, e.g. it appears that we encounter permission denial failures at different points for different tests, so we may need to relax the checking somewhat based on testing a wider range of older kernels. Signed-off-by: Stephen Smalley ... I think we may need to take a closer look at what RHEL-6.x based kernels are currently doing to ensure they are "correct" (I'm going to assume yes, but that is an assumption), and perhaps update the test suite to reflect the RHEL-6.x behavior. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH V2] selinux-testsuite: Add SCTP test support
On Fri, Jun 1, 2018 at 3:44 AM, Richard Haines wrote: > The sctp testsuite tests all new sctp SELinux functionality. > > Signed-off-by: Richard Haines > --- > V2 Changes: > Add -v option to test > Add info in README.md regarding lksctp-tools-devel requirements > Fix asconf parameter chunk processing in test > Fix merge error for policy/Makefile > Fix buffer overflow in sctp_asconf_params_client.c Merged with the understanding that this is a *massive* patch and I went rather quickly through parts so I'm sure I may have missed a few things, but it works on my test system now so that's good :) Thanks again for all the time and effort that went into the SCTP patches/tests, I know how difficult stuff like this can be at times. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[GIT PULL] SELinux patches for v4.18
Hi Linus, SELinux is back with a quiet pull request for v4.18. Three patches, all small: two cleanups of the SELinux audit records, and one to migrate to a newly defined type (vm_fault_t). Everything passes our test suite, and as of about five minutes ago it merged cleanly with your tree. Please pull, thanks. -Paul -- The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338: Linux 4.17-rc1 (2018-04-15 18:24:20 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20180605 for you to fetch changes up to d141136f523a3a6372d22981bdff7a8906f36fea: audit: normalize MAC_POLICY_LOAD record (2018-04-17 17:54:11 -0400) selinux/stable-4.18 PR 20180605 Richard Guy Briggs (2): audit: normalize MAC_STATUS record audit: normalize MAC_POLICY_LOAD record Souptick Joarder (1): security: selinux: Change return type to vm_fault_t security/selinux/selinuxfs.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux-testsuite: Add SCTP test support
On Tue, Mar 20, 2018 at 1:48 PM, Richard Haines via Selinux wrote: > The sctp testsuite tests all new sctp SELinux functionality. > > Signed-off-by: Richard Haines Now that the new SELinux userspace is out, I applied this to my test tree and noticed two problems at the start (both easily fixed): * We need to list the lksctp-tools-devel package as a dependency in README.md * Minor merge conflict in policy/Makefile ... actually running the test went rather well, but there was one test failure: test #11, the "asconf parameter chunk processing". Looking a bit closer at the failure, it appears that the address detection code at the top of tests/sctp/test needs to be a bit more robust as 'hostname -I' returns multiple addresses, but they are a mix of IPv4 and IPv6 - all on one interface. I would suggest taking a look at parsing the output of 'ip -o addr show up scope global' and using that instead of 'hostname -I'. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[GIT PULL] SELinux fixes for v4.17 (#2)
Hi Linus, One more small fix for SELinux: a small string length fix found by KASAN. I dislike sending patches this late in the release cycle, but this patch fixes a legitimate problem, is very small, limited in scope, and well understood. There are two threads with more information on the problem, the latest is linked below: * https://marc.info/?t=15272373741=1=2 If you're hesitant to pull this into v4.17 at such a late stage, it probably isn't going to cause major problems as Stephen points out in the thread linked above: "Such a setxattr() call can only be performed by a process with CAP_MAC_ADMIN that is also allowed mac_admin permission in SELinux policy. Consequently, this is never possible on Android (no process is allowed mac_admin permission, always enforcing) and is only possible in Fedora/RHEL for a few domains (if enforcing)." Thanks, -Paul -- The following changes since commit 4152dc91b5932e7fe49a5afed62a068b2f31d196: selinux: correctly handle sa_family cases in selinux_sctp_bind_connect() (2018-05-14 15:20:59 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20180530 for you to fetch changes up to efe3de79e0b52ca281ef6691480c8c68c82a4657: selinux: KASAN: slab-out-of-bounds in xattr_getsecurity (2018-05-29 20:11:19 -0400) selinux/stable-4.17 PR 20180530 Sachin Grover (1): selinux: KASAN: slab-out-of-bounds in xattr_getsecurity security/selinux/ss/services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity
On Wed, May 30, 2018 at 11:23 AM, Stephen Smalley wrote:
> On 05/30/2018 11:19 AM, Paul Moore wrote:
>> On Fri, May 25, 2018 at 4:31 AM, Sachin Grover
>> wrote:
>>> Call trace:
>>> [] dump_backtrace+0x0/0x428
>>> [] show_stack+0x28/0x38
>>> [] dump_stack+0xd4/0x124
>>> [] print_address_description+0x68/0x258
>>> [] kasan_report.part.2+0x228/0x2f0
>>> [] kasan_report+0x5c/0x70
>>> [] check_memory_region+0x12c/0x1c0
>>> [] memcpy+0x34/0x68
>>> [] xattr_getsecurity+0xe0/0x160
>>> [] vfs_getxattr+0xc8/0x120
>>> [] getxattr+0x100/0x2c8
>>> [] SyS_fgetxattr+0x64/0xa0
>>> [] el0_svc_naked+0x24/0x28
>>>
>>> If user get root access and calls security.selinux setxattr() with an
>>> embedded NUL on a file and then if some process performs a getxattr()
>>> on that file with a length greater than the actual length of the string,
>>> it would result in a panic.
>>>
>>> To fix this, add the actual length of the string to the security context
>>> instead of the length passed by the userspace process.
>>>
>>> Signed-off-by: Sachin Grover
>>> ---
>>> security/selinux/ss/services.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Thanks for reporting this and providing a patch. It's small enough,
>> and passes all the regular tests, so I've merged it into
>> selinux/stable-4.17 (adding the stable metadata) and I'm going to send
>> it up to Linus today.
>>
>> If Linus doesn't pull the fix in time for v4.17 I'll send it up during
>> the upcoming merge window.
>
> NB Such a setxattr() call can only be performed by a process with
> CAP_MAC_ADMIN that is also allowed mac_admin permission in SELinux policy.
> Consequently, this is never possible on Android (no process is allowed
> mac_admin permission, always enforcing) and is only possible in Fedora/RHEL
> for a few domains (if enforcing).
Yes the risk is small, and if it wasn't such a trivial and
self-contained patch I probably would have just deferred it for the
merge window, but considering everything I think there is value in
getting this in for v4.17. If Linus decides not to merge this into
v4.17 I think that is okay too.
> Fixes: 9a59daa03df72526d234b91dd3e32ded5aebd3ef ("SELinux: fix sleeping
> allocation in security_context_to_sid")
>
>>
>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>> index 66ea81c..d17f5b4 100644
>>> --- a/security/selinux/ss/services.c
>>> +++ b/security/selinux/ss/services.c
>>> @@ -1434,7 +1434,7 @@ static int security_context_to_sid_core(const char
>>> *scontext, u32 scontext_len,
>>> scontext_len, , def_sid);
>>> if (rc == -EINVAL && force) {
>>> context.str = str;
>>> - context.len = scontext_len;
>>> + context.len = strlen(str) + 1;
>>> str = NULL;
>>> } else if (rc)
>>> goto out_unlock;
>>> --
>>> 1.9.1
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: KASAN: slab-out-of-bounds in xattr_getsecurity
On Fri, May 25, 2018 at 4:31 AM, Sachin Grover wrote:
> Call trace:
> [] dump_backtrace+0x0/0x428
> [] show_stack+0x28/0x38
> [] dump_stack+0xd4/0x124
> [] print_address_description+0x68/0x258
> [] kasan_report.part.2+0x228/0x2f0
> [] kasan_report+0x5c/0x70
> [] check_memory_region+0x12c/0x1c0
> [] memcpy+0x34/0x68
> [] xattr_getsecurity+0xe0/0x160
> [] vfs_getxattr+0xc8/0x120
> [] getxattr+0x100/0x2c8
> [] SyS_fgetxattr+0x64/0xa0
> [] el0_svc_naked+0x24/0x28
>
> If user get root access and calls security.selinux setxattr() with an
> embedded NUL on a file and then if some process performs a getxattr()
> on that file with a length greater than the actual length of the string,
> it would result in a panic.
>
> To fix this, add the actual length of the string to the security context
> instead of the length passed by the userspace process.
>
> Signed-off-by: Sachin Grover
> ---
> security/selinux/ss/services.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks for reporting this and providing a patch. It's small enough,
and passes all the regular tests, so I've merged it into
selinux/stable-4.17 (adding the stable metadata) and I'm going to send
it up to Linus today.
If Linus doesn't pull the fix in time for v4.17 I'll send it up during
the upcoming merge window.
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 66ea81c..d17f5b4 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1434,7 +1434,7 @@ static int security_context_to_sid_core(const char
> *scontext, u32 scontext_len,
> scontext_len, , def_sid);
> if (rc == -EINVAL && force) {
> context.str = str;
> - context.len = scontext_len;
> + context.len = strlen(str) + 1;
> str = NULL;
> } else if (rc)
> goto out_unlock;
> --
> 1.9.1
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] selinux-testsuite: fix the mode bits for the binder tests
From: Paul Moore Signed-off-by: Paul Moore --- tests/binder/test |0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tests/binder/test diff --git a/tests/binder/test b/tests/binder/test old mode 100644 new mode 100755 ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH] selinux-testsuite: fix some style problems in the binder tests
From: Paul Moore Fixes done by 'tools/check-syntax -f'. Signed-off-by: Paul Moore --- tests/binder/test_binder.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/binder/test_binder.c b/tests/binder/test_binder.c index 0d10a58..11fa358 100644 --- a/tests/binder/test_binder.c +++ b/tests/binder/test_binder.c @@ -632,7 +632,7 @@ int main(int argc, char **argv) writebuf.txn.data.ptr.buffer = (uintptr_t) writebuf.txn.data.ptr.offsets = (uintptr_t) + - sizeof(struct flat_binder_object); + sizeof(struct flat_binder_object); bwr.write_buffer = (uintptr_t) bwr.write_size = sizeof(writebuf); ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC V4 PATCH 1/1] selinux-testsuite: Add binder tests
On Tue, May 22, 2018 at 3:52 PM, Paul Moore <p...@paul-moore.com> wrote: > On Tue, May 22, 2018 at 10:35 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote: >> On 05/22/2018 07:37 AM, Richard Haines wrote: >>> Add binder tests. See tests/binder/test_binder.c for details on >>> message flows to test security_binder*() functions. >>> >>> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com> >> >> Passes for me with 4.17-rc5 on F28 and properly skipped on earlier >> Fedora/RHEL. >> >> Acked-by: Stephen Smalley <s...@tycho.nsa.gov> > > Unfortunately none of my test kernels are compiled with > CONFIG_ANDROID_BINDER* at present, but it fails cleanly for me, and I > trust Stephen gave the binder checks a good look. I'll merge this now FYI, due to some technical difficulties Richard's patch was merged with the wrong attribution. I've fixed it now, but you may see that there was a forced update next time you pull from the selinux-testsuite repo, you can disregard this message. Sorry for the noise. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC V4 PATCH 1/1] selinux-testsuite: Add binder tests
On Tue, May 22, 2018 at 10:35 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote: > On 05/22/2018 07:37 AM, Richard Haines wrote: >> Add binder tests. See tests/binder/test_binder.c for details on >> message flows to test security_binder*() functions. >> >> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com> > > Passes for me with 4.17-rc5 on F28 and properly skipped on earlier > Fedora/RHEL. > > Acked-by: Stephen Smalley <s...@tycho.nsa.gov> Unfortunately none of my test kernels are compiled with CONFIG_ANDROID_BINDER* at present, but it fails cleanly for me, and I trust Stephen gave the binder checks a good look. I'll merge this now ... >> +brexit: I found this bit particularly amusing considering your email domain :) -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH ghak81 V3 3/3] audit: collect audit task parameters
- if (!context) > - return; > - > /* Check for system calls that do not go through the exit > * function (e.g., exit_group), then free context block. > * We use GFP_ATOMIC here because we might be doing this > * in the context of the idle thread */ > /* that can happen only if we are called from do_exit() */ > - if (context->in_syscall && context->current_state == > AUDIT_RECORD_CONTEXT) > + if (context && context->in_syscall && > + context->current_state == AUDIT_RECORD_CONTEXT) > audit_log_exit(context, tsk); > + /* Freeing the audit_task_info struct must be performed after > +* audit_log_exit() due to need for loginuid and sessionid. > +*/ > + info = tsk->audit; > + tsk->audit = NULL; > + kmem_cache_free(audit_task_cache, info); > + if (!context) > + return; > if (!list_empty(>killed_trees)) > audit_kill_trees(>killed_trees); > > @@ -2071,8 +2104,8 @@ int audit_set_loginuid(kuid_t loginuid) > sessionid = (unsigned > int)atomic_inc_return(_id); > } > > - task->sessionid = sessionid; > - task->loginuid = loginuid; > + task->audit->sessionid = sessionid; > + task->audit->loginuid = loginuid; > out: > audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, > sessionid, rc); > return rc; > diff --git a/kernel/fork.c b/kernel/fork.c > index cd18448..92ab849 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1713,7 +1713,7 @@ static __latent_entropy struct task_struct > *copy_process( > p->start_time = ktime_get_ns(); > p->real_start_time = ktime_get_boot_ns(); > p->io_context = NULL; > - audit_set_context(p, NULL); > + p->audit = NULL; > cgroup_fork(p); > #ifdef CONFIG_NUMA > p->mempolicy = mpol_dup(p->mempolicy); > -- > 1.8.3.1 -- paul moore www.paul-moore.com
Re: [PATCH ghak81 V3 2/3] audit: normalize loginuid read access
On Wed, May 16, 2018 at 7:55 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
Also merged into audit/next.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f3d3dc6..ef3e189 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,8 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> + audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +395,14 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +615,8 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result = audit_uid_comparator(audit_get_loginuid(tsk),
> + f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2278,14 +2283,15 @@ int audit_signal_info(int sig, struct task_struct *t)
> {
> struct audit_aux_data_pids *axp;
> struct audit_context *ctx = audit_context();
> - kuid_t uid = current_uid(), t_uid = task_uid(t);
> + kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(current);
> - if (uid_valid(current->loginuid))
> - audit_sig_uid = current->loginuid;
> + auid = audit_get_loginuid(current);
> + if (uid_valid(auid))
> + audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> security_task_getsecid(current, _sig_sid);
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 V3 1/3] audit: use new audit_context access funciton for seccomp_actions_logged
On Wed, May 16, 2018 at 7:55 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On the rebase of the following commit on the new seccomp actions_logged
> function, one audit_context access was missed.
>
> commit cdfb6b341f0f2409aba24b84f3b4b2bba50be5c5
> ("audit: use inline function to get audit context")
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into audit/next, thanks for the follow-up.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index cbab0da..f3d3dc6 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2497,7 +2497,7 @@ void audit_seccomp_actions_logged(const char *names,
> const char *old_names,
> if (!audit_enabled)
> return;
>
> - ab = audit_log_start(current->audit_context, GFP_KERNEL,
> + ab = audit_log_start(audit_context(), GFP_KERNEL,
> AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH 00/23] LSM: Full security module stacking
On Wed, May 16, 2018 at 9:05 PM, Casey Schaufler <ca...@schaufler-ca.com> wrote:
> On 5/16/2018 5:19 PM, Paul Moore wrote:
>> On Wed, May 16, 2018 at 1:42 PM, Casey Schaufler <ca...@schaufler-ca.com>
>> wrote:
>>> On 5/15/2018 2:49 PM, James Morris wrote:
>>>> On Tue, 15 May 2018, Casey Schaufler wrote:
>>>>
>>>>> Both SELinux and Smack use netlbl_sock_setattr() in their
>>>>> socket_post_create()
>>>>> hooks to establish the CIPSO to use if nothing else interferes. An
>>>>> unfortunate
>>>>> artifact of the Smack "ambient label" implementation is that the default
>>>>> configuration is going to delete the netlbl attribute for the floor ("_")
>>>>> label. This will conflict with any value that SELinux sets. :( Smack
>>>>> clearly
>>>>> needs to have it's use of netlabel revised, and that is work that's going
>>>>> on
>>>>> in parallel with stacking. That, however, is not an infrastructure issue,
>>>>> it's
>>>>> an issue with how the two modules use the facilities.
>>>> Can this kind of problem be prevented at the API level? i.e. ensure you
>>>> can't accidentally conflict with another LSM's use of the label here?
>>> What we're seeing isn't an accidental conflict. SELinux and Smack
>>> use netlabel differently. SELinux uses netlabel the way it and the
>>> way CIPSO intended, to mark the MLS levels and categories on a
>>> packet. Smack (ab)uses netlabel to put the Smack label directly on
>>> the packet. In most cases a SELinux system won't be sending a CIPSO
>>> header at all, because it won't be using MLS. A Smack system, on
>>> the other hand, sends a CIPSO header unless the label is the
>>> "ambient" label. Further, the label transitions for a process using
>>> both SELinux and Smack will be different. Two system services may
>>> have different SELinux contexts but the same Smack label.
>> Also, to clarify things a bit for those who haven't dug too deeply
>> into the per-packet bits, the label match/conflict is not done using
>> the individual LSM labels, that would be impossible, it is done using
>> the netlbl_lsm_secattr values. The netlbl_lsm_secattr "label" was
>> always intended to be both LSM and protocol agnostic so it lends
>> itself better for this type of comparison.
>
> Paul is correct. What goes into the netlbl_ls_secattr *is* comparable
> across modules, and that's what this patch set uses.
>
>> At least that is what Casey and I discussed. I haven't had a chance
>> to look at the most recent patches in-depth.
>>
>> [ASIDE: I was just digging through the patches to look for some of the
>> networking changes and it appears there is confusion around SELinux's
>> use of secmark and the NetLabel/IPsec "peer" labels. As an example,
>> the term "secmark" should never appear in cipso_ipv4.c; the secmark
>> labels are completely independent to the peer labels.]
>
> ... And in the end it doesn't. There is an intermediate patch that
> uses the secids.secmark, but at that point it's a union. It could
> just as well use secids.smack or secids.selinux.
Okay, thanks. I was going through the patches rather quickly last
night while replying, I'm sure I've missed a lot. I would request
that future versions of the patchset refrain from using the secmark
union field in the CIPSO code, even intermediate patches. The labeled
networking code is already confusing enough. :)
>> [ASIDE #2: Are you ever comparing the packet labels for
>> domains/sockets which are setup for destination specific labeling
>> (NetLabel's "address selector" feature)? I'm probably missing it, but
>> I don't see it in the patches.]
>
> Smack doesn't use the address selector properly, and hence
> the situation will never arise. When I do get a fix in for Smack
> to use address selectors properly I plan to address that case.
> It is more likely to result in a useful system than the current code.
ASIDE #2 came from my thinking that we may be better off comparing the
on-wire labels (more on that below). If we go the way of on-wire
comparison, the address selector path will need to be taken into
consideration, even if Smack doesn't use it today.
>>> The Smack code clearly needs to be revised. It does a horrible job
>>> on single-level hosts. It works, but does not take advantage of the
>>> netlabel facilities well. The ambient label processing is pretty
>>> kludgey. If the Smack use of netlabel were more in
Re: Re: [RFC PATCH] selinux-testsuite: check the "expand-check" setting in semanage.conf
On Wed, May 16, 2018 at 8:48 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote: > On 05/16/2018 03:31 AM, Petr Lautrbach wrote: >> On Tue, May 15, 2018 at 05:03:42PM -0400, Paul Moore wrote: >>> From: Paul Moore <p...@paul-moore.com> >>> >>> If expand-check is non-zero in semanage.conf the policy load will likely >>> fail, >>> try to provide a more helpful error to users running the tests. >>> >>> Signed-off-by: Paul Moore <p...@paul-moore.com> >>> --- >>> policy/Makefile | 12 ++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/policy/Makefile b/policy/Makefile >>> index 8ed5e46..cc022e3 100644 >>> --- a/policy/Makefile >>> +++ b/policy/Makefile >>> @@ -87,6 +87,14 @@ build: $(BUILD_TARGET) >>> load: $(LOAD_TARGET) >>> unload: $(UNLOAD_TARGET) >>> >>> +expand_check: >>> +# Test for "expand-check = 0" in /etc/selinux/semanage.conf >>> +@cat /etc/selinux/semanage.conf | \ >>> +sed -n 's/^[ \t]*expand-check[ \t]*=[ \t]*0/OK/p' | \ >>> +grep -q "OK" || \ >>> +(echo "ERROR: set 'expand-check = 0' in semanage.conf"; \ >>> + /bin/false) >>> + >> >> You can use grep directly: Yes, you're correct. I have an old habit of using sed for regex processing as I used to work on a lot of systems where grep didn't handle regular expressions. Fixed. > And you should probably put the full path for the file in the error message. I figured that comment above (it is displayed when running 'make test') was enough, but adding the full path couldn't hurt. Fixed, and pushed to selinux-testsuite/master. Thanks to both of you for the quick review. -- paul moore www.paul-moore.com
Re: [PATCH 00/23] LSM: Full security module stacking
we handle poorly today because of this (anyone looked at packet forwarding for labeled traffic? have you looked at forwarding labeled traffic across different protocols?). Having thought about this on and off for several years now, I think the only realistic solution may be to repurpose the secmark field as an index into a larger label store where we can store a more traditional security blob (stacked or otherwise). I hold out some hope that we might be able to do something with skb_shared_info; it's common across cloned packets, but I don't think that would be a problem for us. We might even be able to strike a bargain with the netdev folks: "I'll trade you 32-bits in sk_buff for a pointer in skb_shared_info along with the necessary lifecycle hooks." ... fair warning, that last bit is speculation on my part; I'm guessing they are slightly (so very slightly) less picky about skb_shared_info but there is a good chance I'm wrong about that. -- paul moore www.paul-moore.com
