Re: [PATCH v5 01/38] LSM: Introduce LSM_FLAG_LEGACY_MAJOR
On Tue, Nov 27, 2018 at 2:27 PM Kees Cook wrote:
> On Mon, Nov 26, 2018 at 3:26 PM Casey Schaufler
> wrote:
>
> Hmmm... the "From: Kees..." in the body is missing. Are you using "git
> send-email"?
Not to mention that you are sending (only) to the old SELinux mailing
list. The new list is at seli...@vger.kernel.org, see:
https://lore.kernel.org/selinux/8263f9fa-16f1-1d0a-e391-61d609e50...@tycho.nsa.gov/
>
> >
> > This adds a flag for the current "major" LSMs to distinguish them when
> > we have a universal method for ordering all LSMs. It's called "legacy"
> > since the distinction of "major" will go away in the blob-sharing world.
> >
> > Signed-off-by: Kees Cook
> > Reviewed-by: Casey Schaufler
> > Reviewed-by: John Johansen
> > ---
> > include/linux/lsm_hooks.h | 3 +++
> > security/apparmor/lsm.c| 1 +
> > security/selinux/hooks.c | 1 +
> > security/smack/smack_lsm.c | 1 +
> > security/tomoyo/tomoyo.c | 1 +
> > 5 files changed, 7 insertions(+)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index aaeb7fa24dc4..63c0e102de20 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -2039,8 +2039,11 @@ extern char *lsm_names;
> > extern void security_add_hooks(struct security_hook_list *hooks, int count,
> > char *lsm);
> >
> > +#define LSM_FLAG_LEGACY_MAJOR BIT(0)
> > +
> > struct lsm_info {
> > const char *name; /* Required. */
> > + unsigned long flags;/* Optional: flags describing LSM */
> > int (*init)(void); /* Required. */
> > };
> >
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index 42446a216f3b..2edd35ca5044 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -1728,5 +1728,6 @@ static int __init apparmor_init(void)
> >
> > DEFINE_LSM(apparmor) = {
> > .name = "apparmor",
> > + .flags = LSM_FLAG_LEGACY_MAJOR,
> > .init = apparmor_init,
> > };
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 7ce683259357..56c6f1849c80 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -7209,6 +7209,7 @@ void selinux_complete_init(void)
> > all processes and objects when they are created. */
> > DEFINE_LSM(selinux) = {
> > .name = "selinux",
> > + .flags = LSM_FLAG_LEGACY_MAJOR,
> > .init = selinux_init,
> > };
> >
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 81fb4c1631e9..3639e55b1f4b 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -4891,5 +4891,6 @@ static __init int smack_init(void)
> > */
> > DEFINE_LSM(smack) = {
> > .name = "smack",
> > + .flags = LSM_FLAG_LEGACY_MAJOR,
> > .init = smack_init,
> > };
> > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> > index 1b5b5097efd7..09f7af130d3a 100644
> > --- a/security/tomoyo/tomoyo.c
> > +++ b/security/tomoyo/tomoyo.c
> > @@ -552,5 +552,6 @@ static int __init tomoyo_init(void)
> >
> > DEFINE_LSM(tomoyo) = {
> > .name = "tomoyo",
> > + .flags = LSM_FLAG_LEGACY_MAJOR,
> > .init = tomoyo_init,
> > };
> > --
> > 2.14.5
> >
> >
>
>
> --
> Kees Cook
> ___
> 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.
--
Ondrej Mosnacek
Associate Software Engineer, Security Technologies
Red Hat, Inc.
___
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()
On Tue, Nov 20, 2018 at 10:06 PM Paul Moore wrote:
> On Mon, Nov 12, 2018 at 6:44 AM Ondrej Mosnacek wrote:
> > This function has only two callers, but only one of them actually needs
> > the special logic at the beginning. Factoring this logic out into
> > string_to_context_struct() allows us to drop the arguments 'oldc', 's',
> > and 'def_sid'.
> >
> > Signed-off-by: Ondrej Mosnacek
> > ---
> >
> > Changes in v3:
> > - correct the comment about policy read lock
> >
> > Changes in v2:
> > - also drop unneeded #include's from mls.c
> >
> > security/selinux/ss/mls.c | 49 +-
> > security/selinux/ss/mls.h | 5 +---
> > security/selinux/ss/services.c | 32 +++---
> > 3 files changed, 36 insertions(+), 50 deletions(-)
>
> What was the original motivation for this patch? Is there a performance
> issue?
No, there is no performance issue that I know of. I simply wanted to
move the sidtab_search() reference out of mls.c when I was adding the
sid_to_context/content_to_sid wrappers to services.c. I have now
dropped the wrappers in favor of just rewriting the sidtab functions,
but the mls_context_to_sid() interface looked really awkward to me,
especially considering that the 'ugly' part of the function is really
used by only one caller, so I decided to post the patch anyway.
>
> I'm asking because I'm not really convinced this is an improvement.
> While I agree the number of function arguments is a bordering on "too
> many", I think I like having the logic in mls_context_to_sid() for
> right now.
I disagree, but I don't mind leaving it as it is if that's what you prefer.
>
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index 2fe459df3c85..d1da928a7e77 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -24,10 +24,7 @@
> > #include
> > #include
> > #include
> > -#include "sidtab.h"
> > #include "mls.h"
> > -#include "policydb.h"
> > -#include "services.h"
> >
> > /*
> > * Return the length in bytes for the MLS fields of the
> > @@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct
> > context *c)
> > * This function modifies the string in place, inserting
> > * NULL characters to terminate the MLS fields.
> > *
> > - * If a def_sid is provided and no MLS field is present,
> > - * copy the MLS field of the associated default context.
> > - * Used for upgraded to MLS systems where objects may lack
> > - * MLS fields.
> > - *
> > - * Policy read-lock must be held for sidtab lookup.
> > + * Policy read-lock must be held for policy data lookup.
> > *
> > */
> > int mls_context_to_sid(struct policydb *pol,
> > - char oldc,
> >char *scontext,
> > - struct context *context,
> > - struct sidtab *s,
> > - u32 def_sid)
> > + struct context *context)
> > {
> > char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > struct level_datum *levdatum;
> > @@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
> > int l, rc, i;
> > char *rangep[2];
> >
> > - if (!pol->mls_enabled) {
> > - if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > - return 0;
> > - return -EINVAL;
> > - }
> > -
> > - /*
> > -* No MLS component to the security context, try and map to
> > -* default if provided.
> > -*/
> > - if (!oldc) {
> > - struct context *defcon;
> > -
> > - if (def_sid == SECSID_NULL)
> > - return -EINVAL;
> > -
> > - defcon = sidtab_search(s, def_sid);
> > - if (!defcon)
> > - return -EINVAL;
> > -
> > - return mls_context_cpy(context, defcon);
> > - }
> > -
> > /*
> > * If we're dealing with a range, figure out where the two parts
> > * of the range begin.
> > @@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str,
> > struct context *context,
> > return -EINVAL;
> >
> > tmpstr = kstrdu
Re: [RFC PATCH 1/3] selinux: refactor sidtab conversion
On Tue, Nov 20, 2018 at 10:47 PM Paul Moore wrote: > 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. Damn, I still haven't set up that commit hook... Done now, seems to work fine. Sorry for not having done that sooner and thanks for nagging me about it :) -- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc. ___ 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:
On Mon, Nov 12, 2018 at 7:56 AM Ravi Kumar wrote:
> Hi team ,
>
> On android- with latest kernels 4.14 we are seeing some denials which seem
> to be very much genuine to be address . Where kernel is trying to kill its
> own created process ( might be for maintenance) .
> These are seen in long Stress testing . But I dont see any one adding such
> rule in general so the question is do we see any risk which made us not to
> add such rules ?
>
> 1. avc: denied { kill } for pid=2432 comm="irq/66-90b6300." capability=5
> scontext=u:r:kernel:s0 tcontext=u:r:kernel:s0 tclass=capability permissive=0
> 2. avc: denied { kill } for pid=69 comm="rcuop/6" capability=5
> scontext=u:r:kernel:s0 tcontext=u:r:kernel:s0 tclass=capability permissive=0
> 3. avc: denied { kill } for pid=0 comm="swapper/1" capability=5
> scontext=u:r:kernel:s0 tcontext=u:r:kernel:s0 tclass=capability permissive=0
> 4. avc: denied { kill } for pid=4185 comm="kworker/0:4" capability=5
> scontext=u:r:kernel:s0 tcontext=u:r:kernel:s0 tclass=capability permissive=0
>
> This is self capability any one in kernel context should be able to do such
> operations I guess.
The reference policy does contain a rule that allows this kind of
operations, see:
https://github.com/SELinuxProject/refpolicy/blob/master/policy/modules/kernel/kernel.te#L203
It is also present in the Fedora policy on my system:
$ sesearch -A -s kernel_t -t kernel_t -c capability -p kill
allow kernel_t kernel_t:capability { audit_control audit_write chown
dac_override dac_read_search fowner fsetid ipc_lock ipc_owner kill
lease linux_immutable mknod net_admin net_bind_service net_broadcast
net_raw setfcap setgid setpcap s
etuid sys_admin sys_boot sys_chroot sys_nice sys_pacct sys_ptrace
sys_rawio sys_resource sys_time sys_tty_config };
Therefore I would say it is perfectly fine to add such rule to your
policy as well.
Cheers,
--
Ondrej Mosnacek
Associate Software Engineer, Security Technologies
Red Hat, Inc.
___
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 2/3] selinux: use separate table for initial SID lookup
On Wed, Nov 14, 2018 at 3:08 PM Stephen Smalley wrote: > On 11/14/18 4:45 AM, Ondrej Mosnacek wrote: > > On Tue, Nov 13, 2018 at 10:35 PM Stephen Smalley wrote: > >> On 11/13/18 8:52 AM, Ondrej Mosnacek wrote: > >>> This patch is non-functional and moves handling of initial SIDs into a > >>> separate table. Note that the SIDs stored in the main table are now > >>> shifted by SECINITSID_NUM and converted to/from the actual SIDs > >>> transparently by helper functions. > >> > >> When you say "non-functional", you mean it doesn't make any functional > >> changes, right? As opposed to leaving the kernel in a broken > >> intermediate state until your 3rd patch? > > > > I mean it *should* be non-functional on its own, unless I made a > > mistake. I admit I didn't do very much testing on this patch, but I at > > least checked that I can boot the kernel, load a policy and that > > reported file contexts make sense. > > Commonly non-functional means "not working". I think you mean "this > patch makes no functional changes". Ah, I see, so it was just a language issue :) Indeed I meant the latter. > > > > >> > >> I think you need to double check that all references to > >> state->ss->sidtab are preceded by a check of state->initialized since it > >> could be NULL before first policy load and a system could come up with > >> SELinux enabled but never load a policy. > > > > Well, the original code does not initialize the sidtab field with > > sidtab_init() either so state->ss->sidtab.htable would be NULL (or > > some garbage pointer) after boot and any use of sidtab would cause a > > GPF anyway. > > Prior to the patch, the sidtab is directly embedded in the selinux_ss, > which is static and thus all fields are initialized to 0/NULL. But you > could access the sidtab itself without any problem since it was not a > pointer. Likewise for the policydb. When you change the sidtab to be a > pointer, then you have to deal with the possibility that it will be > NULL. So you might be introducing new situations where we would trigger > a GPF. My point was that even when the sidtab is embedded in selinux_ss, its htable field is a pointer and any sidtab_search[_force]()/sidtab_context_to_sid() calls on an uninitialized sidtab would still trigger a NULL pointer dereference. And outside the policy load code, these are the only functions that are ever used to access the sidtab. > > > > > Looking at the places that reference the sidtab (which are all > > highlighted in the patch because of the switch to pointer type in the > > struct selinux_ss definition), these don't seem to check for > > state->initialized: > > - security_port_sid > > In this case, policydb->ocontexts[OCON_PORT] will be NULL at > initialization, so c will be NULL and we will just set *out_sid to > SECINITSID_PORT and return without ever accessing the sidtab. > > > - security_ib_pkey_sid > > - security_ib_endport_sid > > - security_netif_sid > > - security_node_sid > > These are all similar. > > > - security_sid_mls_copy, called from: > > This one checks state->initialized and returns if not set. > > >- selinux_socket_unix_stream_connect (avc_has_perm is called before) > >- selinux_conn_sid, called from: > > - selinux_sctp_assoc_request (selinux_policycap_extsockclass is > > called before) > > - selinux_inet_conn_request > > - selinux_ip_postroute (selinux_policycap_netpeer is called before) > > - security_net_peersid_resolve, called from: > > This one should always return before taking the policy rwlock when > !state->initialized because you can't have configured network labels > without a policy IIUC (so xfrm_sid and/or nlbl_sid should be NULL), or > regardless, policydb->mls_enabled will be 0 at this point. > > >- selinux_skb_peerlbl_sid, called from: > > - selinux_socket_sock_rcv_skb (selinux_policycap_netpeer is called > > before) > > - selinux_socket_getpeersec_dgram > > - selinux_sctp_assoc_request (again) > > - selinux_inet_conn_request (again) > > - selinux_inet_conn_established > > - selinux_ip_forward (selinux_policycap_netpeer is called before) > > - selinux_ip_postroute (again) > > - selinux_audit_rule_match > > This one can't be called without a prior selinux_audit_rule_init, which > checks state->initialized. > > > - security_netlbl_secattr_to_sid, called from: > > This one checks
Re: [RFC PATCH 2/3] selinux: use separate table for initial SID lookup
On Tue, Nov 13, 2018 at 10:35 PM Stephen Smalley wrote: > On 11/13/18 8:52 AM, Ondrej Mosnacek wrote: > > This patch is non-functional and moves handling of initial SIDs into a > > separate table. Note that the SIDs stored in the main table are now > > shifted by SECINITSID_NUM and converted to/from the actual SIDs > > transparently by helper functions. > > When you say "non-functional", you mean it doesn't make any functional > changes, right? As opposed to leaving the kernel in a broken > intermediate state until your 3rd patch? I mean it *should* be non-functional on its own, unless I made a mistake. I admit I didn't do very much testing on this patch, but I at least checked that I can boot the kernel, load a policy and that reported file contexts make sense. > > I think you need to double check that all references to > state->ss->sidtab are preceded by a check of state->initialized since it > could be NULL before first policy load and a system could come up with > SELinux enabled but never load a policy. Well, the original code does not initialize the sidtab field with sidtab_init() either so state->ss->sidtab.htable would be NULL (or some garbage pointer) after boot and any use of sidtab would cause a GPF anyway. Looking at the places that reference the sidtab (which are all highlighted in the patch because of the switch to pointer type in the struct selinux_ss definition), these don't seem to check for state->initialized: - security_port_sid - security_ib_pkey_sid - security_ib_endport_sid - security_netif_sid - security_node_sid - security_sid_mls_copy, called from: - selinux_socket_unix_stream_connect (avc_has_perm is called before) - selinux_conn_sid, called from: - selinux_sctp_assoc_request (selinux_policycap_extsockclass is called before) - selinux_inet_conn_request - selinux_ip_postroute (selinux_policycap_netpeer is called before) - security_net_peersid_resolve, called from: - selinux_skb_peerlbl_sid, called from: - selinux_socket_sock_rcv_skb (selinux_policycap_netpeer is called before) - selinux_socket_getpeersec_dgram - selinux_sctp_assoc_request (again) - selinux_inet_conn_request (again) - selinux_inet_conn_established - selinux_ip_forward (selinux_policycap_netpeer is called before) - selinux_ip_postroute (again) - selinux_audit_rule_match - security_netlbl_secattr_to_sid, called from: - selinux_netlbl_sidlookup_cached, called from: - selinux_netlbl_skbuff_getsid (netlbl_enabled is called before) - selinux_netlbl_sock_rcv_skb (netlbl_enabled is called before) - security_netlbl_sid_to_secattr, called from: - selinux_netlbl_sock_genattr, called from: - selinux_netlbl_socket_post_create, called from: - selinux_socket_post_create - selinux_netlbl_skbuff_setsid, called from: - selinux_ip_forward (again) - selinux_netlbl_sctp_assoc_request, called from: - selinux_sctp_assoc_request (again) - selinux_netlbl_inet_conn_request, called from: - selinux_inet_conn_request (again) I suppose in some (or all?) of these cases state->initialized might be implicitly always 1, but at least in these it wasn't immediately obvious to me. Either way, such cases would already be buggy before this patch, since they would happily access the policy structure (likely hitting some NULL/invalid pointers) and most likely also dereference the invalid htable pointer in sidtab. > > Aren't you still wasting a slot in the initial SIDs array, or did I miss > something? Yes, I am, because the original code doesn't seem to guard against SECSID_NULL being loaded as initial SID into sidtab. I asked in the other thread whether this is considered a bug, but I didn't get a reply, so I left it sort of bug-to-bug compatible for now... Anyway, it doesn't seem to make much sense to keep that behavior so I will probably just go ahead and add the check to policydb_load_isids() and shrink the table. You can expect it to be resolved in the final patchset. > > > > > This change doesn't make much sense on its own, but it simplifies > > further sidtab overhaul in a succeeding patch. > > > > Signed-off-by: Ondrej Mosnacek > > --- > > security/selinux/ss/policydb.c | 10 ++- > > security/selinux/ss/services.c | 88 ++ > > security/selinux/ss/services.h | 2 +- > > security/selinux/ss/sidtab.c | 158 +++-- > > security/selinux/ss/sidtab.h | 14 +-- > > 5 files changed, 162 insertions(+), 110 deletions(-) > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index f4eadd3f7350..21e4bdbcf994 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
On Wed, Nov 14, 2018 at 4:14 AM Paul Moore wrote:
> On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley wrote:
> > On 11/12/18 6:44 AM, Ondrej Mosnacek wrote:
> > > This function has only two callers, but only one of them actually needs
> > > the special logic at the beginning. Factoring this logic out into
> > > string_to_context_struct() allows us to drop the arguments 'oldc', 's',
> > > and 'def_sid'.
> > >
> > > Signed-off-by: Ondrej Mosnacek
> > > ---
> > >
> > > Changes in v3:
> > > - correct the comment about policy read lock
> > >
> > > Changes in v2:
> > > - also drop unneeded #include's from mls.c
> > >
> > > security/selinux/ss/mls.c | 49 +-
> > > security/selinux/ss/mls.h | 5 +---
> > > security/selinux/ss/services.c | 32 +++---
> > > 3 files changed, 36 insertions(+), 50 deletions(-)
> > >
> > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > > index 2fe459df3c85..d1da928a7e77 100644
> > > --- a/security/selinux/ss/mls.c
> > > +++ b/security/selinux/ss/mls.c
> > > @@ -24,10 +24,7 @@
> > > #include
> > > #include
> > > #include
> > > -#include "sidtab.h"
> > > #include "mls.h"
> > > -#include "policydb.h"
> > > -#include "services.h"
> > >
> > > /*
> > >* Return the length in bytes for the MLS fields of the
> > > @@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct
> > > context *c)
> > >* This function modifies the string in place, inserting
> > >* NULL characters to terminate the MLS fields.
> > >*
> > > - * If a def_sid is provided and no MLS field is present,
> > > - * copy the MLS field of the associated default context.
> > > - * Used for upgraded to MLS systems where objects may lack
> > > - * MLS fields.
> > > - *
> > > - * Policy read-lock must be held for sidtab lookup.
> > > + * Policy read-lock must be held for policy data lookup.
> > >*
> > >*/
> > > int mls_context_to_sid(struct policydb *pol,
> > > -char oldc,
> > > char *scontext,
> > > -struct context *context,
> > > -struct sidtab *s,
> > > -u32 def_sid)
> > > +struct context *context)
> > > {
> > > char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > > struct level_datum *levdatum;
> > > @@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
> > > int l, rc, i;
> > > char *rangep[2];
> > >
> > > - if (!pol->mls_enabled) {
> > > - if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > > - return 0;
> > > - return -EINVAL;
> > > - }
> > > -
> > > - /*
> > > - * No MLS component to the security context, try and map to
> > > - * default if provided.
> > > - */
> > > - if (!oldc) {
> > > - struct context *defcon;
> > > -
> > > - if (def_sid == SECSID_NULL)
> > > - return -EINVAL;
> > > -
> > > - defcon = sidtab_search(s, def_sid);
> > > - if (!defcon)
> > > - return -EINVAL;
> > > -
> > > - return mls_context_cpy(context, defcon);
> > > - }
> > > -
> > > /*
> > >* If we're dealing with a range, figure out where the two parts
> > >* of the range begin.
> > > @@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str,
> > > struct context *context,
> > > return -EINVAL;
> > >
> > > tmpstr = kstrdup(str, gfp_mask);
> > > - if (!tmpstr) {
> > > - rc = -ENOMEM;
> > > - } else {
> > > - rc = mls_context_to_sid(p, ':', tmpstr, context,
> > > - NULL, SECSID_NULL);
> > > - kfree(tmpstr);
> > > - }
> > > + if (!tmpstr)
> > > + return -ENOMEM;
> > >
> > > +
[RFC PATCH 2/3] selinux: use separate table for initial SID lookup
This patch is non-functional and moves handling of initial SIDs into a
separate table. Note that the SIDs stored in the main table are now
shifted by SECINITSID_NUM and converted to/from the actual SIDs
transparently by helper functions.
This change doesn't make much sense on its own, but it simplifies
further sidtab overhaul in a succeeding patch.
Signed-off-by: Ondrej Mosnacek
---
security/selinux/ss/policydb.c | 10 ++-
security/selinux/ss/services.c | 88 ++
security/selinux/ss/services.h | 2 +-
security/selinux/ss/sidtab.c | 158 +++--
security/selinux/ss/sidtab.h | 14 +--
5 files changed, 162 insertions(+), 110 deletions(-)
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..21e4bdbcf994 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab
*s)
if (!c->context[0].user) {
pr_err("SELinux: SID %s was never defined.\n",
c->u.name);
+ sidtab_destroy(s);
+ goto out;
+ }
+ if (c->sid[0] > SECINITSID_NUM) {
+ pr_err("SELinux: Initial SID %s out of range.\n",
+ c->u.name);
+ sidtab_destroy(s);
goto out;
}
- rc = sidtab_insert(s, c->sid[0], &c->context[0]);
+ rc = sidtab_set_initial(s, c->sid[0], &c->context[0]);
if (rc) {
pr_err("SELinux: unable to load initial SID %s.\n",
c->u.name);
+ sidtab_destroy(s);
goto out;
}
}
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 7337db24a6a8..30170d4c567a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct
selinux_state *state,
read_lock(&state->ss->policy_rwlock);
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
if (!user)
tclass = unmap_class(&state->ss->map, orig_tclass);
@@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state,
read_lock(&state->ss->policy_rwlock);
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
rc = -EINVAL;
old_context = sidtab_search(sidtab, old_sid);
@@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct
selinux_state *state,
goto allow;
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state,
goto allow;
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state,
goto allow;
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct
selinux_state *state,
}
read_lock(&state->ss->policy_rwlock);
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
if (force)
context = sidtab_search_force(sidtab, sid);
else
@@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct
selinux_state *state,
}
read_lock(&state->ss->policy_rwlock);
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
rc = string_to_context_struct(policydb, sidtab, scontext2,
&context, def_sid);
if (rc == -EINVAL && force) {
@@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state
*state,
}
policydb = &state->ss->policydb;
- sidtab = &state->ss->sidtab;
+ sidtab = state->ss->sidtab;
scont
[RFC PATCH 3/3] selinux: overhaul sidtab to fix bug and improve performance
Before this patch, during a policy reload the sidtab would become frozen
and trying to map a new context to SID would be unable to add a new
entry to sidtab and fail with -ENOMEM.
Such failures are usually propagated into userspace, which has no way of
distignuishing them from actual allocation failures and thus doesn't
handle them gracefully. Such situation can be triggered e.g. by the
following reproducer:
while true; do load_policy; echo -n .; sleep 0.1; done &
for (( i = 0; i < 1024; i++ )); do
runcon -l s0:c$i echo -n x || break
# or:
# chcon -l s0:c$i || break
done
This patch overhauls the sidtab so it doesn't need to be frozen during
policy reload, thus solving the above problem.
The new SID table leverages the fact that SIDs are allocated
sequentially and are never invalidated and stores them in linear buckets
indexed by a tree structure. This brings several advantages:
1. Fast SID -> context lookup - this lookup can now be done in
logarithmic time complexity (usually in less than 4 array lookups)
and can still be done safely without locking.
2. No need to re-search the whole table on reverse lookup miss - after
acquiring the spinlock only the newly added entries need to be
searched, which means that reverse lookups that end up inserting a
new entry are now about twice as fast.
3. No need to freeze sidtab during policy reload - it is now possible
to handle insertion of new entries even during sidtab conversion.
The tree structure of the new sidtab is able to grow automatically to up
to about 2^31 entries (at which point it should not have more than about
4 tree levels). The old sidtab had a theoretical capacity of almost 2^32
entries, but half of that is still more than enough since by that point
the reverse table lookups would become unusably slow anyway...
The number of entries per tree node is selected automatically so that
each node fits into a single page, which should be the easiest size for
kmalloc() to handle.
Note that currently the new implementation does not have a cache for
recent reverse lookups as the old one had. Since repeated lookups are
quite likely with sidtab, it is an important optimization and I will try
to add it before submitting a final version.
Tested by selinux-testsuite and thoroughly tortured by this simple
stress test:
```
function rand_cat() {
echo $(( $RANDOM % 1024 ))
}
function do_work() {
while true; do
echo -n
"system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
>/sys/fs/selinux/context 2>/dev/null || true
done
}
do_work >/dev/null &
do_work >/dev/null &
do_work >/dev/null &
while load_policy; do echo -n .; sleep 0.1; done
kill %1
kill %2
kill %3
```
Reported-by: Orion Poplawski
Reported-by: Li Kun
Link: https://github.com/SELinuxProject/selinux-kernel/issues/38
Signed-off-by: Ondrej Mosnacek
---
security/selinux/ss/mls.c | 23 +-
security/selinux/ss/mls.h | 3 +-
security/selinux/ss/services.c | 90 +++---
security/selinux/ss/sidtab.c | 514 +++--
security/selinux/ss/sidtab.h | 75 +++--
5 files changed, 391 insertions(+), 314 deletions(-)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..267ae4f9be79 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -436,16 +436,17 @@ int mls_setup_user_range(struct policydb *p,
/*
* Convert the MLS fields in the security context
- * structure `c' from the values specified in the
- * policy `oldp' to the values specified in the policy `newp'.
+ * structure `oldc' from the values specified in the
+ * policy `oldp' to the values specified in the policy `newp',
+ * storing the resulting context in `newc'.
*/
int mls_convert_context(struct policydb *oldp,
struct policydb *newp,
- struct context *c)
+ struct context *oldc,
+ struct context *newc)
{
struct level_datum *levdatum;
struct cat_datum *catdatum;
- struct ebitmap bitmap;
struct ebitmap_node *node;
int l, i;
@@ -455,28 +456,24 @@ int mls_convert_context(struct policydb *oldp,
for (l = 0; l < 2; l++) {
levdatum = hashtab_search(newp->p_levels.table,
sym_name(oldp, SYM_LEVELS,
- c->range.level[l].sens - 1));
+ oldc->range.level[l].sens -
1));
if (!levdatum)
return -EINVAL;
- c->range.level[l].sens = levdatum->level->sens;
+ newc->range.level[l].sens = levdatum->level->sens;
- ebitmap_init(&bitmap);
[RFC PATCH 0/3] Fix ENOMEM errors during policy reload
This patchset is an alternative, hopefully better (but also more risky), solution of the ENOMEM problem ([1]) that I first tried to solve in [2]. In this version I encapsulate the initial SID table within sidtab and also switch back from converting the sidtab in-place to converting into a new sidtab and then just switching the pointer (keeping the code ready for switching to RCU locks). The change is split into three patches for easier review. Some changes done in the first two patches are effectively undone by the last patch, so it might actually make more sense to send the final version as one squashed patch (please let me know which is better for you). The first patch moves the sidtab conversion logic into sidtab.c. This allows hiding sidtab_insert() from sidtab.h in the second patch, where it becomes an internal function. The second patch separates the handling of initial SIDs into a separate lookup table inside sidtab. After this change, the main table always contains N entries with keys from 0 to (N-1). This property is then leveraged in the last patch. Finally, the third patch rewrites the main sidtab to a more efficient implementation that also gracefully handles context conversions during policy reloads, which no longer produces the ENOMEM errors. After applying this patchset, the time it takes to insert new sidtab entries is drastically reduced. I measured the time to populate the table with N new entries by repeatedly writing to /sys/fs/selinux/context. A graph of the results is available at [3]. The SID -> context lookups are now also faster. With the old implementation, these are O(N) once N goes above 128. The new implementation can handle them theoretically in O(log N), but in practice the slope is almost flat, so they are practically almost constant-time. Review and feedback welcome. [1] https://github.com/SELinuxProject/selinux-kernel/issues/38 [2] https://lore.kernel.org/selinux/20181031122718.18735-1-omosn...@redhat.com/ [3] https://docs.google.com/spreadsheets/d/e/2PACX-1vRUArNJR6kckm2SEs4dRZlijNVdCTmsNuWRGe7X3fC01YkBHpxXHnmcssxEiMF3Z7ivtXN2L5MC0ry-/pubhtml Ondrej Mosnacek (3): selinux: refactor sidtab conversion selinux: use separate table for initial SID lookup selinux: overhaul sidtab to fix bug and improve performance security/selinux/ss/mls.c | 23 +- security/selinux/ss/mls.h | 3 +- security/selinux/ss/policydb.c | 10 +- security/selinux/ss/services.c | 188 +-- security/selinux/ss/services.h | 2 +- security/selinux/ss/sidtab.c | 550 - security/selinux/ss/sidtab.h | 90 -- 7 files changed, 498 insertions(+), 368 deletions(-) -- 2.17.2 ___ 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 1/3] selinux: refactor sidtab conversion
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(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 12e414394530..7337db24a6a8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1880,19 +1880,6 @@ int security_change_sid(struct selinux_state *state,
out_sid, false);
}
-/* Clone the SID into the new SID table. */
-static int clone_sid(u32 sid,
-struct context *context,
-void *arg)
-{
- struct sidtab *s = arg;
-
- if (sid > SECINITSID_NUM)
- return sidtab_insert(s, sid, context);
- else
- return 0;
-}
-
static inline int convert_context_handle_invalid_context(
struct selinux_state *state,
struct context *context)
@@ -2186,13 +2173,6 @@ int security_load_policy(struct selinux_state *state,
void *data, size_t len)
goto err;
}
- /* Clone the SID table. */
- sidtab_shutdown(sidtab);
-
- rc = sidtab_map(sidtab, clone_sid, &newsidtab);
- if (rc)
- goto err;
-
/*
* Convert the internal representations of contexts
* in the new SID table.
@@ -2200,7 +2180,7 @@ int security_load_policy(struct selinux_state *state,
void *data, size_t len)
args.state = state;
args.oldp = policydb;
args.newp = newpolicydb;
- rc = sidtab_map(&newsidtab, convert_context, &args);
+ rc = sidtab_convert(sidtab, &newsidtab, convert_context, &args);
if (rc) {
pr_err("SELinux: unable to convert the internal"
" representation of contexts in the new SID"
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index fd75a12fa8fc..e66a2ab3d1c2 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -116,11 +116,11 @@ struct context *sidtab_search_force(struct sidtab *s, u32
sid)
return sidtab_search_core(s, sid, 1);
}
-int sidtab_map(struct sidtab *s,
- int (*apply) (u32 sid,
-struct context *context,
-void *args),
- void *args)
+static int sidtab_map(struct sidtab *s,
+ int (*apply) (u32 sid,
+ struct context *context,
+ void *args),
+ void *args)
{
int i, rc = 0;
struct sidtab_node *cur;
@@ -141,6 +141,37 @@ out:
return rc;
}
+/* Clone the SID into the new SID table. */
+static int clone_sid(u32 sid, struct context *context, void *arg)
+{
+ struct sidtab *s = arg;
+
+ if (sid > SECINITSID_NUM)
+ return sidtab_insert(s, sid, context);
+ else
+ return 0;
+}
+
+int sidtab_convert(struct sidtab *s, struct sidtab *news,
+ int (*convert) (u32 sid,
+ struct context *context,
+ void *args),
+ void *args)
+{
+ unsigned long flags;
+ int rc;
+
+ spin_lock_irqsave(&s->lock, flags);
+ s->shutdown = 1;
+ spin_unlock_irqrestore(&s->lock, flags);
+
+ rc = sidtab_map(s, clone_sid, news);
+ if (rc)
+ return rc;
+
+ return sidtab_map(news, convert, args);
+}
+
static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int
loc)
{
BUG_ON(loc >= SIDTAB_CACHE_LEN);
@@ -295,12 +326,3 @@ void sidtab_set(struct sidtab *dst, struct sidtab *src)
dst->cache[i] = NULL;
spin_unlock_irqrestore(&src->lock, flags);
}
-
-void sidtab_shutdown(struct sidtab *s)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&s->lock, flags);
- s->shutdown = 1;
- spin_unlock_irqrestore(&s->lock, flags);
-}
diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
index a1a1d2617b6f..26c74fe7afc0 100644
--- a/security/selinux/ss/sidtab.h
+++ b/security/selinux/ss/sidtab.h
@@ -37,11 +37,11 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context
*context);
struct context *sidtab_search(struct sidtab *s, u32 sid);
struct context *sidtab_search_force(struct sidtab *s, u32 sid);
-int sidtab_map(struct sidtab *s,
- int (*apply) (u32 sid,
-struct context *context,
-void *args),
- void *arg
[PATCH v3] selinux: simplify mls_context_to_sid()
This function has only two callers, but only one of them actually needs
the special logic at the beginning. Factoring this logic out into
string_to_context_struct() allows us to drop the arguments 'oldc', 's',
and 'def_sid'.
Signed-off-by: Ondrej Mosnacek
---
Changes in v3:
- correct the comment about policy read lock
Changes in v2:
- also drop unneeded #include's from mls.c
security/selinux/ss/mls.c | 49 +-
security/selinux/ss/mls.h | 5 +---
security/selinux/ss/services.c | 32 +++---
3 files changed, 36 insertions(+), 50 deletions(-)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..d1da928a7e77 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -24,10 +24,7 @@
#include
#include
#include
-#include "sidtab.h"
#include "mls.h"
-#include "policydb.h"
-#include "services.h"
/*
* Return the length in bytes for the MLS fields of the
@@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct
context *c)
* This function modifies the string in place, inserting
* NULL characters to terminate the MLS fields.
*
- * If a def_sid is provided and no MLS field is present,
- * copy the MLS field of the associated default context.
- * Used for upgraded to MLS systems where objects may lack
- * MLS fields.
- *
- * Policy read-lock must be held for sidtab lookup.
+ * Policy read-lock must be held for policy data lookup.
*
*/
int mls_context_to_sid(struct policydb *pol,
- char oldc,
char *scontext,
- struct context *context,
- struct sidtab *s,
- u32 def_sid)
+ struct context *context)
{
char *sensitivity, *cur_cat, *next_cat, *rngptr;
struct level_datum *levdatum;
@@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
int l, rc, i;
char *rangep[2];
- if (!pol->mls_enabled) {
- if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
- return 0;
- return -EINVAL;
- }
-
- /*
-* No MLS component to the security context, try and map to
-* default if provided.
-*/
- if (!oldc) {
- struct context *defcon;
-
- if (def_sid == SECSID_NULL)
- return -EINVAL;
-
- defcon = sidtab_search(s, def_sid);
- if (!defcon)
- return -EINVAL;
-
- return mls_context_cpy(context, defcon);
- }
-
/*
* If we're dealing with a range, figure out where the two parts
* of the range begin.
@@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, struct
context *context,
return -EINVAL;
tmpstr = kstrdup(str, gfp_mask);
- if (!tmpstr) {
- rc = -ENOMEM;
- } else {
- rc = mls_context_to_sid(p, ':', tmpstr, context,
- NULL, SECSID_NULL);
- kfree(tmpstr);
- }
+ if (!tmpstr)
+ return -ENOMEM;
+ rc = mls_context_to_sid(p, tmpstr, context);
+ kfree(tmpstr);
return rc;
}
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 67093647576d..e2498f78e100 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range
*r);
int mls_level_isvalid(struct policydb *p, struct mls_level *l);
int mls_context_to_sid(struct policydb *p,
- char oldc,
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;
+ } else if (!oldc) {
+ /*
+* If a def_sid is provided and no MLS field is present,
+* copy the MLS field of the associated default context.
+
[PATCH v2] selinux: simplify mls_context_to_sid()
This function has only two callers, but only one of them actually needs
the special logic at the beginning. Factoring this logic out into
string_to_context_struct() allows us to drop the arguments 'oldc', 's',
and 'def_sid'.
Signed-off-by: Ondrej Mosnacek
---
Changes in v2:
- also drop unneeded #include's from mls.c
security/selinux/ss/mls.c | 47 --
security/selinux/ss/mls.h | 5 +---
security/selinux/ss/services.c | 32 ---
3 files changed, 35 insertions(+), 49 deletions(-)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..f1377f278b5d 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -24,10 +24,7 @@
#include
#include
#include
-#include "sidtab.h"
#include "mls.h"
-#include "policydb.h"
-#include "services.h"
/*
* Return the length in bytes for the MLS fields of the
@@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct
context *c)
* This function modifies the string in place, inserting
* NULL characters to terminate the MLS fields.
*
- * If a def_sid is provided and no MLS field is present,
- * copy the MLS field of the associated default context.
- * Used for upgraded to MLS systems where objects may lack
- * MLS fields.
- *
* Policy read-lock must be held for sidtab lookup.
*
*/
int mls_context_to_sid(struct policydb *pol,
- char oldc,
char *scontext,
- struct context *context,
- struct sidtab *s,
- u32 def_sid)
+ struct context *context)
{
char *sensitivity, *cur_cat, *next_cat, *rngptr;
struct level_datum *levdatum;
@@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
int l, rc, i;
char *rangep[2];
- if (!pol->mls_enabled) {
- if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
- return 0;
- return -EINVAL;
- }
-
- /*
-* No MLS component to the security context, try and map to
-* default if provided.
-*/
- if (!oldc) {
- struct context *defcon;
-
- if (def_sid == SECSID_NULL)
- return -EINVAL;
-
- defcon = sidtab_search(s, def_sid);
- if (!defcon)
- return -EINVAL;
-
- return mls_context_cpy(context, defcon);
- }
-
/*
* If we're dealing with a range, figure out where the two parts
* of the range begin.
@@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, struct
context *context,
return -EINVAL;
tmpstr = kstrdup(str, gfp_mask);
- if (!tmpstr) {
- rc = -ENOMEM;
- } else {
- rc = mls_context_to_sid(p, ':', tmpstr, context,
- NULL, SECSID_NULL);
- kfree(tmpstr);
- }
+ if (!tmpstr)
+ return -ENOMEM;
+ rc = mls_context_to_sid(p, tmpstr, context);
+ kfree(tmpstr);
return rc;
}
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 67093647576d..e2498f78e100 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range
*r);
int mls_level_isvalid(struct policydb *p, struct mls_level *l);
int mls_context_to_sid(struct policydb *p,
- char oldc,
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;
+ } 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 fiel
[PATCH] selinux: simplify mls_context_to_sid()
This function has only two callers, but only one of them actually needs
the special logic at the beginning. Factoring this logic out into
string_to_context_struct() allows us to drop the arguments 'oldc', 's',
and 'def_sid'.
Signed-off-by: Ondrej Mosnacek
---
security/selinux/ss/mls.c | 44 --
security/selinux/ss/mls.h | 5 +---
security/selinux/ss/services.c | 32 ++---
3 files changed, 35 insertions(+), 46 deletions(-)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..587f51657137 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -223,20 +223,12 @@ int mls_context_isvalid(struct policydb *p, struct
context *c)
* This function modifies the string in place, inserting
* NULL characters to terminate the MLS fields.
*
- * If a def_sid is provided and no MLS field is present,
- * copy the MLS field of the associated default context.
- * Used for upgraded to MLS systems where objects may lack
- * MLS fields.
- *
* Policy read-lock must be held for sidtab lookup.
*
*/
int mls_context_to_sid(struct policydb *pol,
- char oldc,
char *scontext,
- struct context *context,
- struct sidtab *s,
- u32 def_sid)
+ struct context *context)
{
char *sensitivity, *cur_cat, *next_cat, *rngptr;
struct level_datum *levdatum;
@@ -244,29 +236,6 @@ int mls_context_to_sid(struct policydb *pol,
int l, rc, i;
char *rangep[2];
- if (!pol->mls_enabled) {
- if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
- return 0;
- return -EINVAL;
- }
-
- /*
-* No MLS component to the security context, try and map to
-* default if provided.
-*/
- if (!oldc) {
- struct context *defcon;
-
- if (def_sid == SECSID_NULL)
- return -EINVAL;
-
- defcon = sidtab_search(s, def_sid);
- if (!defcon)
- return -EINVAL;
-
- return mls_context_cpy(context, defcon);
- }
-
/*
* If we're dealing with a range, figure out where the two parts
* of the range begin.
@@ -364,14 +333,11 @@ int mls_from_string(struct policydb *p, char *str, struct
context *context,
return -EINVAL;
tmpstr = kstrdup(str, gfp_mask);
- if (!tmpstr) {
- rc = -ENOMEM;
- } else {
- rc = mls_context_to_sid(p, ':', tmpstr, context,
- NULL, SECSID_NULL);
- kfree(tmpstr);
- }
+ if (!tmpstr)
+ return -ENOMEM;
+ rc = mls_context_to_sid(p, tmpstr, context);
+ kfree(tmpstr);
return rc;
}
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 67093647576d..e2498f78e100 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range
*r);
int mls_level_isvalid(struct policydb *p, struct mls_level *l);
int mls_context_to_sid(struct policydb *p,
- char oldc,
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;
+ } 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_conte
Re: [PATCH 2/2] selinux: fix ENOMEM errors during policy reload
On Thu, Nov 1, 2018 at 2:15 PM Stephen Smalley wrote: > On 10/31/2018 04:31 PM, Stephen Smalley wrote: > > We'd like to > > replace the policy rwlock with RCU at some point; there is a very old > > patch that tried to do that once before, which eliminated the policy > > write lock altogether (policy switch became a single pointer update), > > but no one has yet taken that back up. > > For reference, here is that old patch that tried converting the policy > rwlock to RCU. It applies on Linux v2.6.9 (yes, it is that old). There > was a more recent effort by Peter Enderborg to convert to RCU to deal > with preempt disable holding, but I had some concerns with the approach > to booleans, see [1] > > Aside from the locking aspects, the other reason I mention it is that I > am unsure of the implications of your model of converting within the > sidtab for a future migration to RCU, since that doesn't seem amenable > to a read-copy-update sequence. I would definitely like to experiment with the RCU conversion at some point. I agree that the new model will probably need to be re-thought again for that, but I don't think it would make it more difficult to do everything right than it would be now. I have another idea how to rewrite the sidtab that should be more RCU-conversion-ready, so maybe I'll even drop this model after all... > > [1] > https://lore.kernel.org/selinux/20180530141104.28569-1-peter.enderb...@sony.com/ -- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc. ___ 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 2/2] selinux: fix ENOMEM errors during policy reload
On Wed, Oct 31, 2018 at 9:29 PM Stephen Smalley wrote:
> On 10/31/2018 08:27 AM, Ondrej Mosnacek wrote:
> > Before this patch, during a policy reload the sidtab would become frozen
> > and trying to map a new context to SID would be unable to add a new
> > entry to sidtab and fail with -ENOMEM.
> >
> > Such failures are usually propagated into userspace, which has no way of
> > distignuishing them from actual allocation failures and thus doesn't
> > handle them gracefully. Such situation can be triggered e.g. by the
> > following reproducer:
> >
> > while true; do load_policy; echo -n .; sleep 0.1; done &
> > for (( i = 0; i < 1024; i++ )); do
> > runcon -l s0:c$i echo -n x || break
> > # or:
> > # chcon -l s0:c$i || break
> > done
> >
> > This patchs overhauls the sidtab so it doesn't need to be frozen during
> > a policy reload, thus solving the above problem.
> >
> > The new SID table entries now contain two slots for the context. One of
> > the slots is used for the lookup and the other one is used during policy
> > reload to store the new converted context. Which slot is used for what
> > is determined by a shared index that is toggled between 0 and 1 when the
> > conversion is completed, together with the switch to the new policy.
> > After the index is toggled, the contexts in the now unused slots are
> > destroyed. The solution also gracefully handles conversion of entries
> > that are added to sidtab while the conversion is in progress.
> >
> > The downside of this solution is that the sidtab now takes up
> > approximately twice the space and half of it is used only during policy
> > reload. On the other hand, this means we do not need to deal with sidtab
> > growing while we are allocating a new one.
> >
> > Reported-by: Orion Poplawski
> > Reported-by: Li Kun
> > Link: https://github.com/SELinuxProject/selinux-kernel/issues/38
> > Signed-off-by: Ondrej Mosnacek
> > ---
> > security/selinux/ss/mls.c | 16 ++---
> > security/selinux/ss/mls.h | 3 +-
> > security/selinux/ss/services.c | 96 +++---
> > security/selinux/ss/sidtab.c | 122 +
> > security/selinux/ss/sidtab.h | 23 ---
> > 5 files changed, 124 insertions(+), 136 deletions(-)
> >
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index cd637ee3fb11..bc3f93732658 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -441,11 +441,11 @@ int mls_setup_user_range(struct policydb *p,
> >*/
> > int mls_convert_context(struct policydb *oldp,
> > struct policydb *newp,
> > - struct context *c)
> > + struct context *oldc,
> > + struct context *newc)
> > {
> > struct level_datum *levdatum;
> > struct cat_datum *catdatum;
> > - struct ebitmap bitmap;
> > struct ebitmap_node *node;
> > int l, i;
> >
> > @@ -455,28 +455,26 @@ int mls_convert_context(struct policydb *oldp,
> > for (l = 0; l < 2; l++) {
> > levdatum = hashtab_search(newp->p_levels.table,
> > sym_name(oldp, SYM_LEVELS,
> > -c->range.level[l].sens -
> > 1));
> > +oldc->range.level[l].sens
> > - 1));
> >
> > if (!levdatum)
> > return -EINVAL;
> > - c->range.level[l].sens = levdatum->level->sens;
> > + newc->range.level[l].sens = levdatum->level->sens;
> >
> > - ebitmap_init(&bitmap);
> > - ebitmap_for_each_positive_bit(&c->range.level[l].cat, node,
> > i) {
> > + ebitmap_for_each_positive_bit(&oldc->range.level[l].cat,
> > node, i) {
> > int rc;
> >
> > catdatum = hashtab_search(newp->p_cats.table,
> > sym_name(oldp, SYM_CATS,
> > i));
> > if (!catdatum)
> > return -EINVAL;
> > - rc = ebitmap_set_bit(&bitmap, catdatum->value - 1, 1);
> > + rc = ebitmap_set_bit(&newc->range.level[l].cat,
> > + c
Re: [PATCH 1/2] selinux: use separate table for initial SID lookup
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.
>
> > +
> > void security_compute_av(struct selinux_state *state,
> >u32 ssid, u32 tsid,
> >u16 tclass, struct av_decision *avd,
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index 2fe459df3c85..cd637ee3fb11 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -235,7 +235,7 @@ int mls_context_to_sid(struct policydb *pol,
> > char oldc,
> > char *scontext,
> > struct context *context,
> > -struct sidtab *s,
> > +struct selinux_state *state,
> > u32 def_sid)
> > {
> > char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > @@ -257,10 +257,10 @@ int mls_context_to_sid(struct policydb *pol,
> > if (!oldc) {
> > struct contex
Re: [PATCH 2/2] selinux: fix ENOMEM errors during policy reload
On Wed, Oct 31, 2018 at 4:24 PM Ondrej Mosnacek wrote:
> On Wed, Oct 31, 2018 at 1:28 PM Ondrej Mosnacek wrote:
> > Before this patch, during a policy reload the sidtab would become frozen
> > and trying to map a new context to SID would be unable to add a new
> > entry to sidtab and fail with -ENOMEM.
> >
> > Such failures are usually propagated into userspace, which has no way of
> > distignuishing them from actual allocation failures and thus doesn't
> > handle them gracefully. Such situation can be triggered e.g. by the
> > following reproducer:
> >
> > while true; do load_policy; echo -n .; sleep 0.1; done &
> > for (( i = 0; i < 1024; i++ )); do
> > runcon -l s0:c$i echo -n x || break
> > # or:
> > # chcon -l s0:c$i || break
> > done
> >
> > This patchs overhauls the sidtab so it doesn't need to be frozen during
> > a policy reload, thus solving the above problem.
> >
> > The new SID table entries now contain two slots for the context. One of
> > the slots is used for the lookup and the other one is used during policy
> > reload to store the new converted context. Which slot is used for what
> > is determined by a shared index that is toggled between 0 and 1 when the
> > conversion is completed, together with the switch to the new policy.
> > After the index is toggled, the contexts in the now unused slots are
> > destroyed. The solution also gracefully handles conversion of entries
> > that are added to sidtab while the conversion is in progress.
> >
> > The downside of this solution is that the sidtab now takes up
> > approximately twice the space and half of it is used only during policy
> > reload. On the other hand, this means we do not need to deal with sidtab
> > growing while we are allocating a new one.
> >
> > Reported-by: Orion Poplawski
> > Reported-by: Li Kun
> > Link: https://github.com/SELinuxProject/selinux-kernel/issues/38
> > Signed-off-by: Ondrej Mosnacek
> > ---
> > security/selinux/ss/mls.c | 16 ++---
> > security/selinux/ss/mls.h | 3 +-
> > security/selinux/ss/services.c | 96 +++---
> > security/selinux/ss/sidtab.c | 122 +
> > security/selinux/ss/sidtab.h | 23 ---
> > 5 files changed, 124 insertions(+), 136 deletions(-)
> >
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index cd637ee3fb11..bc3f93732658 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -441,11 +441,11 @@ int mls_setup_user_range(struct policydb *p,
> > */
> > int mls_convert_context(struct policydb *oldp,
>
> I just realized I should update the comment above this function. I
> staged it for v2, but I will wait for more feedback before sending a
> new version.
>
> > struct policydb *newp,
> > - struct context *c)
> > + struct context *oldc,
> > + struct context *newc)
> > {
> > struct level_datum *levdatum;
> > struct cat_datum *catdatum;
> > - struct ebitmap bitmap;
> > struct ebitmap_node *node;
> > int l, i;
> >
> > @@ -455,28 +455,26 @@ int mls_convert_context(struct policydb *oldp,
> > for (l = 0; l < 2; l++) {
> > levdatum = hashtab_search(newp->p_levels.table,
> > sym_name(oldp, SYM_LEVELS,
> > - c->range.level[l].sens -
> > 1));
> > +
> > oldc->range.level[l].sens - 1));
> >
> > if (!levdatum)
> > return -EINVAL;
> > - c->range.level[l].sens = levdatum->level->sens;
> > + newc->range.level[l].sens = levdatum->level->sens;
> >
> > - ebitmap_init(&bitmap);
> > - ebitmap_for_each_positive_bit(&c->range.level[l].cat, node,
> > i) {
> > + ebitmap_for_each_positive_bit(&oldc->range.level[l].cat,
> > node, i) {
> > int rc;
> >
> > catdatum = hashtab_search(newp->p_cats.table,
> > sym_name(oldp, SYM_CATS,
> > i));
> > if (!catdatum)
> > return -EINVAL;
>
Re: [PATCH 2/2] selinux: fix ENOMEM errors during policy reload
On Wed, Oct 31, 2018 at 1:28 PM Ondrej Mosnacek wrote:
> Before this patch, during a policy reload the sidtab would become frozen
> and trying to map a new context to SID would be unable to add a new
> entry to sidtab and fail with -ENOMEM.
>
> Such failures are usually propagated into userspace, which has no way of
> distignuishing them from actual allocation failures and thus doesn't
> handle them gracefully. Such situation can be triggered e.g. by the
> following reproducer:
>
> while true; do load_policy; echo -n .; sleep 0.1; done &
> for (( i = 0; i < 1024; i++ )); do
> runcon -l s0:c$i echo -n x || break
> # or:
> # chcon -l s0:c$i || break
> done
>
> This patchs overhauls the sidtab so it doesn't need to be frozen during
> a policy reload, thus solving the above problem.
>
> The new SID table entries now contain two slots for the context. One of
> the slots is used for the lookup and the other one is used during policy
> reload to store the new converted context. Which slot is used for what
> is determined by a shared index that is toggled between 0 and 1 when the
> conversion is completed, together with the switch to the new policy.
> After the index is toggled, the contexts in the now unused slots are
> destroyed. The solution also gracefully handles conversion of entries
> that are added to sidtab while the conversion is in progress.
>
> The downside of this solution is that the sidtab now takes up
> approximately twice the space and half of it is used only during policy
> reload. On the other hand, this means we do not need to deal with sidtab
> growing while we are allocating a new one.
>
> Reported-by: Orion Poplawski
> Reported-by: Li Kun
> Link: https://github.com/SELinuxProject/selinux-kernel/issues/38
> Signed-off-by: Ondrej Mosnacek
> ---
> security/selinux/ss/mls.c | 16 ++---
> security/selinux/ss/mls.h | 3 +-
> security/selinux/ss/services.c | 96 +++---
> security/selinux/ss/sidtab.c | 122 +
> security/selinux/ss/sidtab.h | 23 ---
> 5 files changed, 124 insertions(+), 136 deletions(-)
>
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index cd637ee3fb11..bc3f93732658 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -441,11 +441,11 @@ int mls_setup_user_range(struct policydb *p,
> */
> int mls_convert_context(struct policydb *oldp,
I just realized I should update the comment above this function. I
staged it for v2, but I will wait for more feedback before sending a
new version.
> struct policydb *newp,
> - struct context *c)
> + struct context *oldc,
> + struct context *newc)
> {
> struct level_datum *levdatum;
> struct cat_datum *catdatum;
> - struct ebitmap bitmap;
> struct ebitmap_node *node;
> int l, i;
>
> @@ -455,28 +455,26 @@ int mls_convert_context(struct policydb *oldp,
> for (l = 0; l < 2; l++) {
> levdatum = hashtab_search(newp->p_levels.table,
> sym_name(oldp, SYM_LEVELS,
> - c->range.level[l].sens -
> 1));
> + oldc->range.level[l].sens
> - 1));
>
> if (!levdatum)
> return -EINVAL;
> - c->range.level[l].sens = levdatum->level->sens;
> + newc->range.level[l].sens = levdatum->level->sens;
>
> - ebitmap_init(&bitmap);
> - ebitmap_for_each_positive_bit(&c->range.level[l].cat, node,
> i) {
> + ebitmap_for_each_positive_bit(&oldc->range.level[l].cat,
> node, i) {
> int rc;
>
> catdatum = hashtab_search(newp->p_cats.table,
> sym_name(oldp, SYM_CATS,
> i));
> if (!catdatum)
> return -EINVAL;
> - rc = ebitmap_set_bit(&bitmap, catdatum->value - 1, 1);
> + rc = ebitmap_set_bit(&newc->range.level[l].cat,
> +catdatum->value - 1, 1);
> if (rc)
> return rc;
>
> cond_resched();
> }
> - ebitmap_destroy(&c->range.level[l].cat);
> - c->range.level[l].cat = bitmap;
>
[PATCH 1/2] selinux: use separate table for initial SID lookup
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_PERMISSIVE 0x0001
+struct context *security_sid_to_context_struct(struct selinux_state *state,
+ u32 sid, int force);
+
void security_compute_av(struct selinux_state *state,
u32 ssid, u32 tsid,
u16 tclass, struct av_decision *avd,
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..cd637ee3fb11 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -235,7 +235,7 @@ int mls_context_to_sid(struct policydb *pol,
char oldc,
char *scontext,
struct context *context,
- struct sidtab *s,
+ struct selinux_state *state,
u32 def_sid)
{
char *sensitivity, *cur_cat, *next_cat, *rngptr;
@@ -257,10 +257,10 @@ int mls_context_to_sid(struct policydb *pol,
if (!oldc) {
struct context *defcon;
- if (def_sid == SECSID_NULL)
+ if (def_sid == SECSID_NULL || state == NULL)
return -EINVAL;
- defcon = sidtab_search(s, def_sid);
+ defcon = security_sid_to_context_struct(state, def_sid, 0);
if (!defcon)
return -EINVAL;
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 67093647576d..1eca02c8bc5f 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -36,7 +36,7 @@ int mls_context_to_sid(struct policydb *p,
char oldc,
char *scontext,
struct context *context,
- struct sidtab *s,
+ struct selinux_state *state,
u32 def_sid);
int mls_from_string(struct policydb *p, char *str, struct context *context,
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..8f7cd5f6e033 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -892,16 +892,12 @@ void policydb_destroy(struct policydb *p)
* Load the initial SIDs specified in a policy database
* structure into a SID table.
*/
-int policydb_load_isids(struct policydb *p, struct sidtab *s)
+int policydb_load_isids(struct policydb *p, struct isidtab *s)
{
struct ocontext *head, *c;
int rc;
- rc = sidtab_init(s);
- if (rc) {
- pr_err("SELinux: out of memory on SID table init\n");
- goto out;
- }
+ isidtab_init(s);
head = p->ocontexts[OCON_ISID];
for (c = head; c; c = c->next) {
@@ -911,16 +907,30 @@ int policydb_load_isids(struct policydb *p, struct sidtab
*s)
c->u.name);
goto out;
}
+ if (c->sid[0] > SECINITSID_NUM) {
+ pr_err("SELinux: Initial SID %u out of range.\n",
+ (unsigned)c->sid[0]);
+ goto out;
+ }
+ if (s->entries[c->sid[0]].set) {
+ pr_err("SELinux: Duplicit initial SID %u.\n",
+ (unsigned)c->sid[0]);
+ goto out;
+ }
- rc = sidtab_insert(s, c->sid[0], &c-&
[PATCH 0/2] Fix ENOMEM errors during policy reload
This patchset revamps the SID table implementation to fix ENOMEM errors
returned from sidtab_context_to_sid() during policy reload.
The first patch prepares the way for the second one by moving the handling of
initial SIDs to a separate table. This is needed since the second patch will do
the sidtab conversion in-place and handling the initial SIDs would complicate
things too much.
The second patch changes the way that sidtab is transitioned to the new policy
so that it does not need to be frozen for modifications during the conversion
of entries to the new policy.
See individual patches for more details.
Resolves: https://github.com/SELinuxProject/selinux-kernel/issues/38
Testing:
- passed selinux-testsuite
- verified using the reproducer from GH issue
- tested with the following stress test on SMP (with lock debugging enabled):
function rand_cat() {
echo $(( $RANDOM % 1024 ))
}
function do_work() {
while runcon -l s0:c$(rand_cat),c$(rand_cat) echo -n x; do :; done
}
do_work >/dev/null &
do_work >/dev/null &
do_work >/dev/null &
while load_policy; do echo -n .; sleep 0.1; done
kill %1
kill %2
kill %3
--
Ondrej Mosnacek (2):
selinux: use separate table for initial SID lookup
selinux: fix ENOMEM errors during policy reload
security/selinux/include/security.h | 3 +
security/selinux/ss/mls.c | 22 +-
security/selinux/ss/mls.h | 5 +-
security/selinux/ss/policydb.c | 24 ++-
security/selinux/ss/policydb.h | 26 ++-
security/selinux/ss/services.c | 314 +---
security/selinux/ss/services.h | 1 +
security/selinux/ss/sidtab.c| 141 +++--
security/selinux/ss/sidtab.h| 26 +--
9 files changed, 296 insertions(+), 266 deletions(-)
--
2.17.2
___
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 2/2] selinux: fix ENOMEM errors during policy reload
Before this patch, during a policy reload the sidtab would become frozen
and trying to map a new context to SID would be unable to add a new
entry to sidtab and fail with -ENOMEM.
Such failures are usually propagated into userspace, which has no way of
distignuishing them from actual allocation failures and thus doesn't
handle them gracefully. Such situation can be triggered e.g. by the
following reproducer:
while true; do load_policy; echo -n .; sleep 0.1; done &
for (( i = 0; i < 1024; i++ )); do
runcon -l s0:c$i echo -n x || break
# or:
# chcon -l s0:c$i || break
done
This patchs overhauls the sidtab so it doesn't need to be frozen during
a policy reload, thus solving the above problem.
The new SID table entries now contain two slots for the context. One of
the slots is used for the lookup and the other one is used during policy
reload to store the new converted context. Which slot is used for what
is determined by a shared index that is toggled between 0 and 1 when the
conversion is completed, together with the switch to the new policy.
After the index is toggled, the contexts in the now unused slots are
destroyed. The solution also gracefully handles conversion of entries
that are added to sidtab while the conversion is in progress.
The downside of this solution is that the sidtab now takes up
approximately twice the space and half of it is used only during policy
reload. On the other hand, this means we do not need to deal with sidtab
growing while we are allocating a new one.
Reported-by: Orion Poplawski
Reported-by: Li Kun
Link: https://github.com/SELinuxProject/selinux-kernel/issues/38
Signed-off-by: Ondrej Mosnacek
---
security/selinux/ss/mls.c | 16 ++---
security/selinux/ss/mls.h | 3 +-
security/selinux/ss/services.c | 96 +++---
security/selinux/ss/sidtab.c | 122 +
security/selinux/ss/sidtab.h | 23 ---
5 files changed, 124 insertions(+), 136 deletions(-)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index cd637ee3fb11..bc3f93732658 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -441,11 +441,11 @@ int mls_setup_user_range(struct policydb *p,
*/
int mls_convert_context(struct policydb *oldp,
struct policydb *newp,
- struct context *c)
+ struct context *oldc,
+ struct context *newc)
{
struct level_datum *levdatum;
struct cat_datum *catdatum;
- struct ebitmap bitmap;
struct ebitmap_node *node;
int l, i;
@@ -455,28 +455,26 @@ int mls_convert_context(struct policydb *oldp,
for (l = 0; l < 2; l++) {
levdatum = hashtab_search(newp->p_levels.table,
sym_name(oldp, SYM_LEVELS,
- c->range.level[l].sens - 1));
+ oldc->range.level[l].sens -
1));
if (!levdatum)
return -EINVAL;
- c->range.level[l].sens = levdatum->level->sens;
+ newc->range.level[l].sens = levdatum->level->sens;
- ebitmap_init(&bitmap);
- ebitmap_for_each_positive_bit(&c->range.level[l].cat, node, i) {
+ ebitmap_for_each_positive_bit(&oldc->range.level[l].cat, node,
i) {
int rc;
catdatum = hashtab_search(newp->p_cats.table,
sym_name(oldp, SYM_CATS, i));
if (!catdatum)
return -EINVAL;
- rc = ebitmap_set_bit(&bitmap, catdatum->value - 1, 1);
+ rc = ebitmap_set_bit(&newc->range.level[l].cat,
+catdatum->value - 1, 1);
if (rc)
return rc;
cond_resched();
}
- ebitmap_destroy(&c->range.level[l].cat);
- c->range.level[l].cat = bitmap;
}
return 0;
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 1eca02c8bc5f..00c11304f71a 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -46,7 +46,8 @@ int mls_range_set(struct context *context, struct mls_range
*range);
int mls_convert_context(struct policydb *oldp,
struct policydb *newp,
- struct context *context);
+ struct context *oldc,
+ struct context *newc);
int mls_compute_sid(struct policydb *p,
struct context *scontext,
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/service
[PATCH v6] selinux: policydb - fix byte order and alignment issues
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 <
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
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..b63ef865ce1e 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
{
int i, j, rc;
u32 nel, len;
+ __be64 prefixbuf[1];
__le32 buf[3];
struct ocontext *l, *c;
u32 nodebuf[8];
@@ -2217,21 +2218,30 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
goto out;
break;
}
- case OCON_IBPKEY:
- rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
+ case OCON_IBPKEY: {
+ u32 pkey_lo, pkey_hi;
+
+ rc = next_entry(prefixbuf, fp, sizeof(u64));
+ if (rc)
+ goto out;
+
+ /* we need to have subnet_prefix in CPU order */
+ c->u.ibpkey.subnet_prefix =
be64_to_cpu(prefixbuf[0]);
+
+ rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto out;
- c->u.ibpkey.subnet_prefix =
be64_to_cpu(*((__be64 *)nodebuf));
+ pkey_lo = le32_to_cpu(buf[0]);
+ pkey_hi = le32_to_cpu(buf[1]);
- if (nodebuf[2] > 0x ||
- nodebuf[3] > 0x) {
+ if (pkey_lo > U16_MAX || pkey_hi > U16_MAX) {
rc = -EINVAL;
goto out;
}
- c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]);
- c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]);
+ c->u.ibpkey.low_pkey = pkey_lo;
+ c->u.ibpkey.high_pkey = pkey_hi;
rc = context_read_and_validate(&c->context[0],
p,
@@ -2239,7 +2249,10 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
break;
- case OCON_IBENDPORT:
+ }
+ case OCON_IBENDPORT: {
+ u32 port;
+
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto out;
@@ -2249,12 +2262,13 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
- if (buf[1] > 0xff || buf[1] == 0) {
+ port = le32_to_cpu(buf[1]);
+ if (port > U8_MAX || port == 0) {
rc = -EINVAL;
goto out;
}
- c->u.ibendport.port = le32_to_cpu(buf[1]);
+ c->u.ibendport.port = port;
rc = context_read_and_validate(&c->context[0],
p,
@@ -2262,7 +2276,8 @@ static
[PATCH v2] libsepol: add missing ibendport port validity check
The kernel checks if the port is in the range 1-255 when loading an
ibenportcon rule. Add the same check to libsepol.
Fixes: 118c0cd1038e ("libsepol: Add ibendport ocontext handling")
Signed-off-by: Ondrej Mosnacek
---
libsepol/src/policydb.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
Changes in v2:
- use UINT8_MAX as the limit for ibendport.port value to emphasize that
it is an 8-bit value
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index db6765ba..96176d80 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -2854,7 +2854,9 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
return -1;
break;
}
- case OCON_IBENDPORT:
+ case OCON_IBENDPORT: {
+ uint32_t port;
+
rc = next_entry(buf, fp, sizeof(uint32_t) * 2);
if (rc < 0)
return -1;
@@ -2862,6 +2864,10 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
if (len == 0 || len > IB_DEVICE_NAME_MAX - 1)
return -1;
+ port = le32_to_cpu(buf[1]);
+ if (port > UINT8_MAX || port == 0)
+ return -1;
+
c->u.ibendport.dev_name = malloc(len + 1);
if (!c->u.ibendport.dev_name)
return -1;
@@ -2869,11 +2875,12 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
if (rc < 0)
return -1;
c->u.ibendport.dev_name[len] = 0;
- c->u.ibendport.port = le32_to_cpu(buf[1]);
+ c->u.ibendport.port = port;
if (context_read_and_validate
(&c->context[0], p, fp))
return -1;
break;
+ }
case OCON_PORT:
rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
if (rc < 0)
--
2.17.2
___
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: add missing ibendport port validity check
On Mon, Oct 22, 2018 at 4:49 PM William Roberts
wrote:
> On Mon, Oct 22, 2018 at 1:18 AM Ondrej Mosnacek wrote:
> >
> > The kernel checks if the port is in the range 1-255 when loading an
> > ibenportcon rule. Add the same check to libsepol.
> >
> > Fixes: 118c0cd1038e ("libsepol: Add ibendport ocontext handling")
> > Signed-off-by: Ondrej Mosnacek
> > ---
> > libsepol/src/policydb.c | 11 +--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > index db6765ba..e2808b2d 100644
> > --- a/libsepol/src/policydb.c
> > +++ b/libsepol/src/policydb.c
> > @@ -2854,7 +2854,9 @@ static int ocontext_read_selinux(struct
> > policydb_compat_info *info,
> > return -1;
> > break;
> > }
> > - case OCON_IBENDPORT:
> > + case OCON_IBENDPORT: {
> > + uint32_t port;
> > +
> > rc = next_entry(buf, fp, sizeof(uint32_t) *
> > 2);
> > if (rc < 0)
> > return -1;
> > @@ -2862,6 +2864,10 @@ static int ocontext_read_selinux(struct
> > policydb_compat_info *info,
> > if (len == 0 || len > IB_DEVICE_NAME_MAX -
> > 1)
> > return -1;
> >
> > + port = le32_to_cpu(buf[1]);
> > + if (port > 0xff || port == 0)
> > + return -1;
>
> You switched the other code to using UINT16_MAX, should probably use
> UINT8_MAX here.
Good point. I'll need to update the kernel patch as well.
Thanks,
>
> > +
> > c->u.ibendport.dev_name = malloc(len + 1);
> > if (!c->u.ibendport.dev_name)
> > return -1;
> > @@ -2869,11 +2875,12 @@ static int ocontext_read_selinux(struct
> > policydb_compat_info *info,
> > if (rc < 0)
> > return -1;
> > c->u.ibendport.dev_name[len] = 0;
> > - c->u.ibendport.port = le32_to_cpu(buf[1]);
> > + c->u.ibendport.port = port;
> > if (context_read_and_validate
> > (&c->context[0], p, fp))
> > return -1;
> > break;
> > + }
> > case OCON_PORT:
> > rc = next_entry(buf, fp, sizeof(uint32_t) *
> > 3);
> > if (rc < 0)
> > --
> > 2.17.2
> >
--
Ondrej Mosnacek
Associate Software Engineer, Security Technologies
Red Hat, Inc.
___
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 v5] selinux: policydb - fix byte order and alignment issues
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 <
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 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
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..d7e8126f32dc 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
{
int i, j, rc;
u32 nel, len;
+ __be64 prefixbuf[1];
__le32 buf[3];
struct ocontext *l, *c;
u32 nodebuf[8];
@@ -2217,21 +2218,30 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
goto out;
break;
}
- case OCON_IBPKEY:
- rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
+ case OCON_IBPKEY: {
+ u32 pkey_lo, pkey_hi;
+
+ rc = next_entry(prefixbuf, fp, sizeof(u64));
+ if (rc)
+ goto out;
+
+ /* we need to have subnet_prefix in CPU order */
+ c->u.ibpkey.subnet_prefix =
be64_to_cpu(prefixbuf[0]);
+
+ rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto out;
- c->u.ibpkey.subnet_prefix =
be64_to_cpu(*((__be64 *)nodebuf));
+ pkey_lo = le32_to_cpu(buf[0]);
+ pkey_hi = le32_to_cpu(buf[1]);
- if (nodebuf[2] > 0x ||
- nodebuf[3] > 0x) {
+ if (pkey_lo > U16_MAX || pkey_hi > U16_MAX) {
rc = -EINVAL;
goto out;
}
- c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]);
- c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]);
+ c->u.ibpkey.low_pkey = pkey_lo;
+ c->u.ibpkey.high_pkey = pkey_hi;
rc = context_read_and_validate(&c->context[0],
p,
@@ -2239,7 +2249,10 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
break;
- case OCON_IBENDPORT:
+ }
+ case OCON_IBENDPORT: {
+ u32 port;
+
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto out;
@@ -2249,12 +2262,13 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
- if (buf[1] > 0xff || buf[1] == 0) {
+ port = le32_to_cpu(buf[1]);
+ if (port > 0xff || port == 0) {
rc = -EINVAL;
goto out;
}
- c->u.ibendport.port = le32_to_cpu(buf[1]);
+ c->u.ibendport.port = port;
rc = context_read_and_validate(&c->context[0],
p,
@@ -2262,7 +2276,8 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
[PATCH] libsepol: add missing ibendport port validity check
The kernel checks if the port is in the range 1-255 when loading an
ibenportcon rule. Add the same check to libsepol.
Fixes: 118c0cd1038e ("libsepol: Add ibendport ocontext handling")
Signed-off-by: Ondrej Mosnacek
---
libsepol/src/policydb.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index db6765ba..e2808b2d 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -2854,7 +2854,9 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
return -1;
break;
}
- case OCON_IBENDPORT:
+ case OCON_IBENDPORT: {
+ uint32_t port;
+
rc = next_entry(buf, fp, sizeof(uint32_t) * 2);
if (rc < 0)
return -1;
@@ -2862,6 +2864,10 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
if (len == 0 || len > IB_DEVICE_NAME_MAX - 1)
return -1;
+ port = le32_to_cpu(buf[1]);
+ if (port > 0xff || port == 0)
+ return -1;
+
c->u.ibendport.dev_name = malloc(len + 1);
if (!c->u.ibendport.dev_name)
return -1;
@@ -2869,11 +2875,12 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
if (rc < 0)
return -1;
c->u.ibendport.dev_name[len] = 0;
- c->u.ibendport.port = le32_to_cpu(buf[1]);
+ c->u.ibendport.port = port;
if (context_read_and_validate
(&c->context[0], p, fp))
return -1;
break;
+ }
case OCON_PORT:
rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
if (rc < 0)
--
2.17.2
___
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 v4] selinux: policydb - fix byte order and alignment issues
On Sat, Oct 20, 2018 at 3:05 AM William Roberts
wrote:
> On Fri, Oct 19, 2018 at 7:28 AM Stephen Smalley wrote:
> >
> > On 10/18/2018 03:47 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 | 46 +++---
> > > 1 file changed, 32 insertions(+), 14 deletions(-)
> > >
> > > 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
> > >
> > > diff --git a/security/selinux/ss/policydb.c
> > > b/security/selinux/ss/policydb.c
> > > index f4eadd3f7350..d50668006a52 100644
> > > --- a/security/selinux/ss/policydb.c
> > > +++ b/security/selinux/ss/policydb.c
> > > @@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct
> > > policydb_compat_info *info,
> > > {
> > > int i, j, rc;
> > > u32 nel, len;
> > > + __be64 prefixbuf[1];
> > > __le32 buf[3];
> > > struct ocontext *l, *c;
> > > u32 nodebuf[8];
> > > @@ -2217,21 +2218,30 @@ static int ocontext_read(struct policydb *p,
> > > struct policydb_compat_info *info,
> > > goto out;
> > > break;
> > > }
> > > - case OCON_IBPKEY:
> > > - rc = next_entry(nodebuf, fp, sizeof(u32) *
> > > 4);
> > > + case OCON_IBPKEY: {
> > > + u32 pkey_lo, pkey_hi;
> > > +
> > > + rc = next_entry(prefixbuf, fp, sizeof(u64));
> > > + if (rc)
> > > + goto out;
> > > +
> > > + /* we need to have subnet_prefix in CPU
> > > order */
> > > + c->u.ibpkey.subnet_prefix =
> > > be64_to_cpu(prefixbuf[0]);
> > > +
> > > + rc = next_entry(buf, fp, sizeof(u32) * 2);
> > > if (rc)
> > > goto out;
> > >
> > > - c->u.ibpkey.subnet_prefix =
> > > be64_to_cpu(*((__be64 *)nodebuf));
> > > + pkey_lo = le32_to_cpu(buf[0]);
> > > + pkey_hi = le32_to_cpu(buf[1]);
> > >
> > > - if (nodebuf[2] > 0x ||
> > > - nodebuf[3] > 0x) {
> > > + if (pkey_lo > U16_MAX || pkey_hi > U16_MAX)
> > > {
> > > rc = -EINVAL;
> > > goto out;
> > >
Re: [PATCH v4] selinux: policydb - fix byte order and alignment issues
On Fri, Oct 19, 2018 at 4:26 PM Stephen Smalley wrote:
> On 10/18/2018 03:47 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 | 46 +++---
> > 1 file changed, 32 insertions(+), 14 deletions(-)
> >
> > 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
> >
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index f4eadd3f7350..d50668006a52 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct
> > policydb_compat_info *info,
> > {
> > int i, j, rc;
> > u32 nel, len;
> > + __be64 prefixbuf[1];
> > __le32 buf[3];
> > struct ocontext *l, *c;
> > u32 nodebuf[8];
> > @@ -2217,21 +2218,30 @@ static int ocontext_read(struct policydb *p, struct
> > policydb_compat_info *info,
> > goto out;
> > break;
> > }
> > - case OCON_IBPKEY:
> > - rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
> > + case OCON_IBPKEY: {
> > + u32 pkey_lo, pkey_hi;
> > +
> > + rc = next_entry(prefixbuf, fp, sizeof(u64));
> > + if (rc)
> > + goto out;
> > +
> > + /* we need to have subnet_prefix in CPU order
> > */
> > + c->u.ibpkey.subnet_prefix =
> > be64_to_cpu(prefixbuf[0]);
> > +
> > + rc = next_entry(buf, fp, sizeof(u32) * 2);
> > if (rc)
> > goto out;
> >
> > - c->u.ibpkey.subnet_prefix =
> > be64_to_cpu(*((__be64 *)nodebuf));
> > + pkey_lo = le32_to_cpu(buf[0]);
> > + pkey_hi = le32_to_cpu(buf[1]);
> >
> > - if (nodebuf[2] > 0x ||
> > - nodebuf[3] > 0x) {
> > + if (pkey_lo > U16_MAX || pkey_hi > U16_MAX) {
> > rc = -EINVAL;
> > goto out;
> > }
> >
> > - c->u.ibpkey.low_pkey =
> > le32_to_cpu(nodebuf[2]);
> > - c->u.ibpkey.high_pkey =
> > le32_to_cpu(nodebuf[3]);
> > + c->u.ibpkey.low_pkey = pkey_lo;
> > + c->u.ibpkey.high_pkey = pkey_hi;
> >
> > rc = context_read_and_validate(&c->context[0],
> > p,
> > @@ -2239,6 +2249
[PATCH v4] selinux: policydb - fix byte order and alignment issues
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 <
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 | 46 +++---
1 file changed, 32 insertions(+), 14 deletions(-)
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
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..d50668006a52 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
{
int i, j, rc;
u32 nel, len;
+ __be64 prefixbuf[1];
__le32 buf[3];
struct ocontext *l, *c;
u32 nodebuf[8];
@@ -2217,21 +2218,30 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
goto out;
break;
}
- case OCON_IBPKEY:
- rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
+ case OCON_IBPKEY: {
+ u32 pkey_lo, pkey_hi;
+
+ rc = next_entry(prefixbuf, fp, sizeof(u64));
+ if (rc)
+ goto out;
+
+ /* we need to have subnet_prefix in CPU order */
+ c->u.ibpkey.subnet_prefix =
be64_to_cpu(prefixbuf[0]);
+
+ rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto out;
- c->u.ibpkey.subnet_prefix =
be64_to_cpu(*((__be64 *)nodebuf));
+ pkey_lo = le32_to_cpu(buf[0]);
+ pkey_hi = le32_to_cpu(buf[1]);
- if (nodebuf[2] > 0x ||
- nodebuf[3] > 0x) {
+ if (pkey_lo > U16_MAX || pkey_hi > U16_MAX) {
rc = -EINVAL;
goto out;
}
- c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]);
- c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]);
+ c->u.ibpkey.low_pkey = pkey_lo;
+ c->u.ibpkey.high_pkey = pkey_hi;
rc = context_read_and_validate(&c->context[0],
p,
@@ -2239,6 +2249,7 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
break;
+ }
case OCON_IBENDPORT:
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
@@ -2249,13 +2260,14 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
- if (buf[1] > 0xff || buf[1] == 0) {
+ c->u.ibendport.port = le32_to_cpu(buf[1]);
+
+ if (c->u.ibendport.port > 0xff ||
+ c->u.ibendport.port == 0) {
rc = -EINVAL;
goto out;
}
- c->u.ibendport.port = le32_to_cpu(buf[1]);
-
rc = context_read_and_validate(&c->context[0],
p,
fp);
@@ -3105,6 +3117,7 @@ static int ocontext_write(struct policydb *p, struct
policydb_compat_info *info,
{
unsigned int i, j, rc;
size_t nel, len;
+ __be64 prefixbuf[1];
__le32 buf[3];
u32 nodebuf[8];
struct
[PATCH v2] libsepol: fix endianity in ibpkey range checks
We need to convert from little-endian before dong range checks on the
ibpkey port numbers, otherwise we would be checking a wrong value on
big-endian systems.
Fixes: 9fbb3112769a ("libsepol: Add ibpkey ocontext handling")
Signed-off-by: Ondrej Mosnacek
---
libsepol/src/policydb.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
Changes in v2:
- defer assignment to 16-bit struct fields to after the range check
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index a6d76ca3..db6765ba 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -2828,21 +2828,32 @@ static int ocontext_read_selinux(struct
policydb_compat_info *info,
(&c->context[1], p, fp))
return -1;
break;
- case OCON_IBPKEY:
+ case OCON_IBPKEY: {
+ uint32_t pkey_lo, pkey_hi;
+
rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
- if (rc < 0 || buf[2] > 0x || buf[3] >
0x)
+ if (rc < 0)
+ return -1;
+
+ pkey_lo = le32_to_cpu(buf[2]);
+ pkey_hi = le32_to_cpu(buf[3]);
+
+ if (pkey_lo > UINT16_MAX || pkey_hi >
UINT16_MAX)
return -1;
+ c->u.ibpkey.low_pkey = pkey_lo;
+ c->u.ibpkey.high_pkey = pkey_hi;
+
+ /* we want c->u.ibpkey.subnet_prefix in network
+* (big-endian) order, just memcpy it */
memcpy(&c->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
(&c->context[0], p, fp))
return -1;
break;
+ }
case OCON_IBENDPORT:
rc = next_entry(buf, fp, sizeof(uint32_t) * 2);
if (rc < 0)
--
2.17.2
___
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 6: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.
Oh right, I didn't realize those fields are 16-bit... Let me fix it and re-spin.
>
> > +
> > + 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(&c->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
> > (&c->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)
--
Ondrej Mosnacek
Associate Software Engineer, Security Technologies
Red Hat, Inc.
___
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] libsepol: fix endianity in ibpkey range checks
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]);
+
+ 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(&c->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
(&c->context[0], p, fp))
return -1;
--
2.17.2
___
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 v3] selinux: policydb - fix byte order and alignment issues
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 <
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 | 41 ++
1 file changed, 27 insertions(+), 14 deletions(-)
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
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..b9029491869b 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2108,6 +2108,7 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
{
int i, j, rc;
u32 nel, len;
+ __be64 prefixbuf[1];
__le32 buf[3];
struct ocontext *l, *c;
u32 nodebuf[8];
@@ -2218,21 +2219,26 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
break;
}
case OCON_IBPKEY:
- rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
+ rc = next_entry(prefixbuf, fp, sizeof(u64));
if (rc)
goto out;
- c->u.ibpkey.subnet_prefix =
be64_to_cpu(*((__be64 *)nodebuf));
+ /* we need to have subnet_prefix in CPU order */
+ c->u.ibpkey.subnet_prefix =
be64_to_cpu(prefixbuf[0]);
- if (nodebuf[2] > 0x ||
- nodebuf[3] > 0x) {
+ rc = next_entry(buf, fp, sizeof(u32) * 2);
+ if (rc)
+ goto out;
+
+ c->u.ibpkey.low_pkey = le32_to_cpu(buf[0]);
+ c->u.ibpkey.high_pkey = le32_to_cpu(buf[1]);
+
+ if (c->u.ibpkey.low_pkey > 0x ||
+ c->u.ibpkey.high_pkey > 0x) {
rc = -EINVAL;
goto out;
}
- c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]);
- c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]);
-
rc = context_read_and_validate(&c->context[0],
p,
fp);
@@ -2249,13 +2255,14 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
- if (buf[1] > 0xff || buf[1] == 0) {
+ c->u.ibendport.port = le32_to_cpu(buf[1]);
+
+ if (c->u.ibendport.port > 0xff ||
+ c->u.ibendport.port == 0) {
rc = -EINVAL;
goto out;
}
- c->u.ibendport.port = le32_to_cpu(buf[1]);
-
rc = context_read_and_validate(&c->context[0],
p,
fp);
@@ -3105,6 +3112,7 @@ static int ocontext_write(struct policydb *p, struct
policydb_compat_info *info,
{
unsigned int i, j, rc;
size_t nel, len;
+ __be64 prefixbuf[1];
__le32 buf[3];
u32 nodebuf[8];
struct ocontext *c;
@@ -3192,12 +3200,17 @@ static int ocontext_write(struct policydb *p, struct
policydb_compat_info *info,
return rc;
break;
case OCON_IBPKEY:
- *((__be64 *)nodebuf) =
cpu_to_be64(c->u.ibpkey.subnet_prefix);
+ /* subnet_prefix is in CPU order */
+ prefixbuf[0] =
cpu_to_be64(c->u.ibpkey.subnet_prefix);
- nodebuf[2] = cpu_to_le32(c->u.ibpkey.low_pkey);
-
Re: [PATCH v2] selinux: fix byte order and alignment issues in policydb.c
On Tue, Oct 16, 2018 at 4:19 PM Ondrej Mosnacek wrote:
> On Tue, Oct 16, 2018 at 2:53 PM Stephen Smalley wrote:
> > On 10/16/2018 03:09 AM, Ondrej Mosnacek wrote:
> > > Add missing LE conversions to the Infiniband-related range checks. These
> > > were causing a failure to load any policy with an ibendportcon rule on
> > > BE systems. This can be reproduced by running:
> > >
> > > 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
> > >
> > > (On ppc64 it fails with "/sbin/load_policy: Can't load policy: Invalid
> > > argument")
> > >
> > > Also, the temporary buffers are only guaranteed to be aligned for 32-bit
> > > access so use (get/put)_unaligned_be64() for 64-bit accesses.
> > >
> > > 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 | 28 +++-
> > > 1 file changed, 15 insertions(+), 13 deletions(-)
> > >
> > > Changes in v2:
> > > - add reproducer to commit message
> > > - update e-mail address of James Morris
> > > - better Cc also the old SELinux ML
> > >
> > > diff --git a/security/selinux/ss/policydb.c
> > > b/security/selinux/ss/policydb.c
> > > index f4eadd3f7350..2b310e8f2923 100644
> > > --- a/security/selinux/ss/policydb.c
> > > +++ b/security/selinux/ss/policydb.c
> > > @@ -37,6 +37,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include "security.h"
> > >
> > > #include "policydb.h"
> > > @@ -2108,7 +2109,7 @@ static int ocontext_read(struct policydb *p, struct
> > > policydb_compat_info *info,
> > > {
> > > int i, j, rc;
> > > u32 nel, len;
> > > - __le32 buf[3];
> > > + __le32 buf[4];
> > > struct ocontext *l, *c;
> > > u32 nodebuf[8];
> > >
> > > @@ -2218,20 +2219,20 @@ static int ocontext_read(struct policydb *p,
> > > struct policydb_compat_info *info,
> > > break;
> > > }
> > > case OCON_IBPKEY:
> > > - rc = next_entry(nodebuf, fp, sizeof(u32) *
> > > 4);
> > > + rc = next_entry(buf, fp, sizeof(u32) * 4);
> > > if (rc)
> > > goto out;
> > >
> > > - c->u.ibpkey.subnet_prefix =
> > > be64_to_cpu(*((__be64 *)nodebuf));
> > > + c->u.ibpkey.subnet_prefix =
> > > get_unaligned_be64(buf);
> > >
> > > - if (nodebuf[2] > 0x ||
> > > - nodebuf[3] > 0x) {
> > > + if (le32_to_cpu(buf[2]) > 0x ||
> > > + le32_to_cpu(buf[3]) > 0x) {
> > > rc = -EINVAL;
> > > goto out;
> > > }
> > >
> > > - c->u.ibpkey.low_pkey =
> > > le32_to_cpu(nodebuf[2]);
> > > - c->u.ibpkey.high_pkey =
> > > le32_to_cpu(nodebuf[3]);
> > > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]);
> > > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
> >
> > I'm wondering why the handling here is inconsistent with that of
> > OCON_NODE/OCON_NODE6, which also deals with network byte order / big
> > endian data.
>
> I believe OCON_NODE/OCON_NODE6 doesn't call be32_to
Re: [PATCH v2] selinux: fix byte order and alignment issues in policydb.c
On Tue, Oct 16, 2018 at 2:53 PM Stephen Smalley wrote:
> On 10/16/2018 03:09 AM, Ondrej Mosnacek wrote:
> > Add missing LE conversions to the Infiniband-related range checks. These
> > were causing a failure to load any policy with an ibendportcon rule on
> > BE systems. This can be reproduced by running:
> >
> > 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
> >
> > (On ppc64 it fails with "/sbin/load_policy: Can't load policy: Invalid
> > argument")
> >
> > Also, the temporary buffers are only guaranteed to be aligned for 32-bit
> > access so use (get/put)_unaligned_be64() for 64-bit accesses.
> >
> > 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 | 28 +++-
> > 1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > Changes in v2:
> > - add reproducer to commit message
> > - update e-mail address of James Morris
> > - better Cc also the old SELinux ML
> >
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index f4eadd3f7350..2b310e8f2923 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -37,6 +37,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "security.h"
> >
> > #include "policydb.h"
> > @@ -2108,7 +2109,7 @@ static int ocontext_read(struct policydb *p, struct
> > policydb_compat_info *info,
> > {
> > int i, j, rc;
> > u32 nel, len;
> > - __le32 buf[3];
> > + __le32 buf[4];
> > struct ocontext *l, *c;
> > u32 nodebuf[8];
> >
> > @@ -2218,20 +2219,20 @@ static int ocontext_read(struct policydb *p, struct
> > policydb_compat_info *info,
> > break;
> > }
> > case OCON_IBPKEY:
> > - rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
> > + rc = next_entry(buf, fp, sizeof(u32) * 4);
> > if (rc)
> > goto out;
> >
> > - c->u.ibpkey.subnet_prefix =
> > be64_to_cpu(*((__be64 *)nodebuf));
> > + c->u.ibpkey.subnet_prefix =
> > get_unaligned_be64(buf);
> >
> > - if (nodebuf[2] > 0x ||
> > - nodebuf[3] > 0x) {
> > + if (le32_to_cpu(buf[2]) > 0x ||
> > + le32_to_cpu(buf[3]) > 0x) {
> > rc = -EINVAL;
> > goto out;
> > }
> >
> > - c->u.ibpkey.low_pkey =
> > le32_to_cpu(nodebuf[2]);
> > - c->u.ibpkey.high_pkey =
> > le32_to_cpu(nodebuf[3]);
> > + c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]);
> > + c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
>
> I'm wondering why the handling here is inconsistent with that of
> OCON_NODE/OCON_NODE6, which also deals with network byte order / big
> endian data.
I believe OCON_NODE/OCON_NODE6 doesn't call be32_to_cpu() because the
kernel code probably expects those values to be in the "network
order", in the sense that when you call ntohl() (basically an alias
for be32_to_cpu()) on them, then you get a value where the low bytes
are actually in the low bits of the integer. There are comments that
seem to be intended as a justification doing this. Perhaps the
subnet_prefix has different semantics, perhaps not...
> Also it is inconsistent with the corresponding userspace
> code in libsepol for IBPKEY, which just does a memcpy() for copying
> between the subnet_pr
[PATCH v2] selinux: fix byte order and alignment issues in policydb.c
Add missing LE conversions to the Infiniband-related range checks. These
were causing a failure to load any policy with an ibendportcon rule on
BE systems. This can be reproduced by running:
cat >my_module.cil <
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 | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
Changes in v2:
- add reproducer to commit message
- update e-mail address of James Morris
- better Cc also the old SELinux ML
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..2b310e8f2923 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include "security.h"
#include "policydb.h"
@@ -2108,7 +2109,7 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
{
int i, j, rc;
u32 nel, len;
- __le32 buf[3];
+ __le32 buf[4];
struct ocontext *l, *c;
u32 nodebuf[8];
@@ -2218,20 +2219,20 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
break;
}
case OCON_IBPKEY:
- rc = next_entry(nodebuf, fp, sizeof(u32) * 4);
+ rc = next_entry(buf, fp, sizeof(u32) * 4);
if (rc)
goto out;
- c->u.ibpkey.subnet_prefix =
be64_to_cpu(*((__be64 *)nodebuf));
+ c->u.ibpkey.subnet_prefix =
get_unaligned_be64(buf);
- if (nodebuf[2] > 0x ||
- nodebuf[3] > 0x) {
+ if (le32_to_cpu(buf[2]) > 0x ||
+ le32_to_cpu(buf[3]) > 0x) {
rc = -EINVAL;
goto out;
}
- c->u.ibpkey.low_pkey = le32_to_cpu(nodebuf[2]);
- c->u.ibpkey.high_pkey = le32_to_cpu(nodebuf[3]);
+ c->u.ibpkey.low_pkey = le32_to_cpu(buf[2]);
+ c->u.ibpkey.high_pkey = le32_to_cpu(buf[3]);
rc = context_read_and_validate(&c->context[0],
p,
@@ -2249,7 +2250,8 @@ static int ocontext_read(struct policydb *p, struct
policydb_compat_info *info,
if (rc)
goto out;
- if (buf[1] > 0xff || buf[1] == 0) {
+ if (le32_to_cpu(buf[1]) > 0xff ||
+ le32_to_cpu(buf[1]) == 0) {
rc = -EINVAL;
goto out;
}
@@ -3105,7 +3107,7 @@ static int ocontext_write(struct policydb *p, struct
policydb_compat_info *info,
{
unsigned int i, j, rc;
size_t nel, len;
- __le32 buf[3];
+ __le32 buf[4];
u32 nodebuf[8];
struct ocontext *c;
for (i = 0; i < info->ocon_num; i++) {
@@ -3192,12 +3194,12 @@ static int ocontext_write(struct policydb *p, struct
policydb_compat_info *info,
return rc;
break;
case OCON_IBPKEY:
- *((__be64 *)nodebuf) =
cpu_to_be64(c->u.ibpkey.subnet_prefix);
+ put_unaligned_be64(c->u.ibpkey.subnet_prefix,
buf);
- nodebuf[2] = cpu_to_le32(c->u.ibpkey.low_pkey);
- nodebuf[3] = cpu_to_le32(c->u.ibpkey.high_pkey);
+ buf[2] = cpu_to_le32(c->u.ibpkey.low_pkey);
+ buf[3] = cpu_to_le32(c->u.ibpkey.high_pkey);
- rc = put_entry(nodebuf, sizeof(u32), 4, fp);
+ rc = put_entry(buf, sizeof(u32), 4, fp);
if (rc)
return rc;
rc = context_write(p, &c->context[0], fp);
--
2.17.2
___
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] restorecond: Do not ignore the -f option
Since the default value of watch_file is set unconditionally *after* the
command-line arguments have been parsed, the -f option is (and has
always been) effectively ignored. Fix this by setting it before the
parsing.
Fixes: 48681bb49c03 ("policycoreutils: restorecond: make restorecond
dbuss-able")
Signed-off-by: Ondrej Mosnacek
---
restorecond/restorecond.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/restorecond/restorecond.c b/restorecond/restorecond.c
index e1d26cb9..7b984b29 100644
--- a/restorecond/restorecond.c
+++ b/restorecond/restorecond.c
@@ -148,6 +148,8 @@ int main(int argc, char **argv)
if (is_selinux_enabled() != 1)
return 0;
+ watch_file = server_watch_file;
+
/* Set all options to zero/NULL except for ignore_noent & digest. */
memset(&r_opts, 0, sizeof(r_opts));
r_opts.ignore_noent = SELINUX_RESTORECON_IGNORE_NOENTRY;
@@ -205,7 +207,6 @@ int main(int argc, char **argv)
return 0;
}
- watch_file = server_watch_file;
read_config(master_fd, watch_file);
if (!debug_mode) {
--
2.17.1
___
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 race when removing selinuxfs entries
Hi Al, On Tue, Oct 2, 2018 at 5:58 PM Al Viro wrote: > On Tue, Oct 02, 2018 at 01:18:30PM +0200, Ondrej Mosnacek wrote: > > No. With the side of Hell, No. The bug is real, but this is > not the way to fix it. > > First of all, it's still broken - e.g. mount something on a > subdirectory and watch what that thing will do to it. And > anyone who has permission to reload policy _will_ have one > for mount. I have no doubts there are still tons of bugs left over, but having processes traverse selinuxfs while load_policy is in progress is something that can (and will) easily happen in practice. Mounting over an selinuxfs subdirectory OTOH isn't something that you would normally do. I think it is worth doing a quick but partial fix that fixes a practical problem and then working towards a better long-term solution. I feel like you are assuming that I am trying to fix some security problem here, but that's not true. It *may* be seen as a security issue, but as you point out having permission to load policy will usually imply you can do other kinds of damage anyway. I simply see this as an annoying real-life bug (I know of at least one user that is/was affected) that I want to fix (even if not in a perfect way for now). > > And yes, debugfs_remove() suffers the same problem. Frankly, the > only wish debugfs_remove() implementation inspires is to shoot it > before it breeds. Which is the second problem with that approach - > such stuff really shouldn't be duplicated in individual filesystems. > > Don't copy (let along open-code) d_walk() in there. The guts of > dcache tree handling are subtle enough (and had more than enough > locking bugs over the years) to make spreading the dependencies > on its details all over the tree an invitation for massive PITA > down the road. Right, I am now convinced that it is not a good idea at all. The thought of adding a new function to dcache has crossed my mind, but I dismissed it as I didn't want to needlessly touch other parts of the kernel. Looking back now, it would have been a better choice indeed. > > I have beginnings of patch doing that for debugfs; the same thing > should be usable for selinuxfs as well. However, the main problem > with selinuxfs wrt policy loading is different - what to do if > sel_make_policy_nodes() fails halfway through? And what to do > with accesses to the unholy mix of old and new nodes we currently > have left behind? Again, this a big problem, too, but out of the scope I care about right now. > > Before security_load_policy() we don't know which nodes to create; > after it we have nothing to fall back onto. It looks like we > need to split security_load_policy() into "load", "switch over" and > "free old" parts, so that the whole thing would look like > load > create new directory trees, unconnected to the root so > they couldn't be reached by lookups until we are done > switch over > move the new directory trees in place > kill the old trees (using that invalidate+genocide carefully > combination) > free old data structures > with failures upon the first two steps handled by killing whatever detached > trees we'd created (if any) and freeing the new data structures. > > However, I'd really like to have the folks familiar with selinux guts to > comment upon the feasibility of the above. AFAICS, nobody has ever seriously > looked at that code wrt graceful error handling, etc.[*], so I'm not happy > with making inferences from what the existing code is doing. Yes, that sounds like it could work. I'd be willing to work on that as a longer term solution. Let's hope we get some feedback from them. > > If you are interested in getting selinuxfs into sane shape, that would > be a good place to start. As for the kernel-side rm -rf (which is what > debugfs_remove() et.al. are trying to be)... > * it absolutely needs to be in fs/*.c - either dcache or libfs. > It's too closely tied to dcache guts to do otherwise. > * as the first step it needs to do d_invalidate(), to get rid of > anything that might be mounted on it and to prevent new mounts from appearing. > It's rather tempting to unhash everything in the victim tree at the same > time, but that needs to be done with care - I'm still not entirely happy > with the solution I've got in that area. Alternative is to unhash them > on the way out of subtree. simple_unlink()/simple_rmdir() are wrong > there - we don't want to bother with the parent's timestamps as we go, > for one thing; that should be done only once to parent of the root of > that subtree. For another, we bloody well enforce the emptiness oursel
[PATCH] selinux: fix race when removing selinuxfs entries
Letting the following set of commands run long enough on a multi-core
machine causes soft lockups in the kernel:
(cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
(cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
(cd /sys/fs/selinux/; while true; do find >/dev/null 2>&1; done) &
while true; do load_policy; echo -n .; sleep 0.1; done
The problem is that sel_remove_entries() calls d_genocide() to remove
the whole contents of certain subdirectories in /sys/fs/selinux/. This
function is apparently only intended for removing filesystem entries
that are no longer accessible to userspace, because it doesn't follow
the rule that any code removing entries from a directory must hold the
write lock on the directory's inode RW semaphore (formerly this was a
mutex, see 9902af79c01a ("parallel lookups: actual switch to rwsem")).
In order to fix this, I had to open-code d_genocide() again, adding the
necessary parent inode locking. I based the new implementation on
d_walk() (fs/dcache.c), the original code from before ad52184b705c
("selinuxfs: don't open-code d_genocide()"), and with a slight
inspiration from debugfs_remove() (fs/debugfs/inode.c).
The new code no longer triggers soft lockups with the above reproducer
and it also gives no warnings when run with CONFIG_PROVE_LOCKING=y.
Note that I am adding Fixes: on the commit that replaced the open-coded
version with d_genocide(), but the bug is present also in the original
code that dates back to pre-2.6 times... This patch obviously doesn't
apply to this older code so pre-4.0 kernels will need a dedicated patch
(just adding the parent inode locks should be sufficient there).
Link: https://github.com/SELinuxProject/selinux-kernel/issues/42
Fixes: ad52184b705c ("selinuxfs: don't open-code d_genocide()")
Cc: # 4.0+
Cc: Stephen Smalley
Cc: Al Viro
Signed-off-by: Ondrej Mosnacek
---
security/selinux/selinuxfs.c | 88 ++--
1 file changed, 85 insertions(+), 3 deletions(-)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index f3a5a138a096..4ac9b17e24d1 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1316,10 +1316,92 @@ static const struct file_operations
sel_commit_bools_ops = {
.llseek = generic_file_llseek,
};
-static void sel_remove_entries(struct dentry *de)
+/* removes all children of the given dentry (but not itself) */
+static void sel_remove_entries(struct dentry *root)
{
- d_genocide(de);
- shrink_dcache_parent(de);
+ struct dentry *parent = root;
+ struct dentry *child;
+ struct list_head *next;
+
+ spin_lock(&parent->d_lock);
+
+repeat:
+ next = parent->d_subdirs.next;
+
+ while (next != &parent->d_subdirs) {
+ child = list_entry(next, struct dentry, d_child);
+ next = next->next;
+
+ spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+
+ if (!list_empty(&child->d_subdirs)) {
+ /* Current entry has children, we need to remove those
+* first. We unlock the parent but keep the child locked
+* (it will become the new parent). */
+ spin_unlock(&parent->d_lock);
+
+ /* we need to re-lock the child (new parent) in a
+* special way (see d_walk() in fs/dcache.c) */
+ spin_release(&child->d_lock.dep_map, 1, _RET_IP_);
+ parent = child;
+ spin_acquire(&parent->d_lock.dep_map, 0, 1, _RET_IP_);
+
+ goto repeat;
+ }
+
+resume:
+ if (child->d_inode) {
+ /* acquire a reference to the child */
+ dget_dlock(child);
+
+ /* drop all locks (someof the callees will try to
+* acquire them) */
+ spin_unlock(&child->d_lock);
+ spin_unlock(&parent->d_lock);
+
+ /* IMPORTANT: we need to hold the parent's inode lock
+* because some functions (namely dcache_readdir) assume
+* holding this lock means children won't be removed */
+ inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
+
+ /* now unlink the child */
+ if (d_is_dir(child))
+ simple_rmdir(d_inode(parent), child);
+ else
+ simple_unlink(d_inode(parent), child);
+ d_delete(child);
+
+ inode_unlock(parent->d_inode);
+
+
Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-16 16:11 GMT+02:00 Richard Guy Briggs : > On 2018-04-16 09:26, Ondrej Mosnacek wrote: >> 2018-04-10 1:34 GMT+02:00 Richard Guy Briggs : >> > There were two formats of the audit MAC_STATUS record, one of which was >> > more >> > standard than the other. One listed enforcing status changes and the >> > other listed enabled status changes with a non-standard label. In >> > addition, the record was missing information about which LSM was >> > responsible and the operation's completion status. While this record is >> > only issued on success, the parser expects the res= field to be present. >> > >> > old enforcing/permissive: >> > type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 >> > old_enforcing=1 auid=0 ses=1 >> > old enable/disable: >> > type=MAC_STATUS msg=audit(1523312831.378:24514): selinux=0 auid=0 ses=1 >> > >> > List both sets of status and old values and add the lsm= field and the >> > res= field. >> > >> > Here is the new format: >> > type=MAC_STATUS msg=audit(1523293828.657:891): enforcing=0 old_enforcing=1 >> > auid=0 ses=1 enabled=1 old-enabled=1 lsm=selinux res=1 >> > >> > This record already accompanied a SYSCALL record. >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/46 >> > Signed-off-by: Richard Guy Briggs >> > --- >> > security/selinux/selinuxfs.c | 11 +++ >> > 1 file changed, 7 insertions(+), 4 deletions(-) >> > >> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c >> > index 00eed84..00b21b2 100644 >> > --- a/security/selinux/selinuxfs.c >> > +++ b/security/selinux/selinuxfs.c >> > @@ -145,10 +145,11 @@ static ssize_t sel_write_enforce(struct file *file, >> > const char __user *buf, >> > if (length) >> > goto out; >> > audit_log(current->audit_context, GFP_KERNEL, >> > AUDIT_MAC_STATUS, >> > - "enforcing=%d old_enforcing=%d auid=%u ses=%u", >> > + "enforcing=%d old_enforcing=%d auid=%u ses=%u" >> > + " enabled=%d old-enabled=%d lsm=selinux res=1", >> >> This is just a tiny nit but why does "old_enforcing" use an underscore >> and "old-enabled" a dash? Shouldn't the style be consistent across >> fields? > > Yes, but my understanding is a preference for underscore, and not to > change existing field names. Ah, I just noticed that the field is already used elsewhere in the code, so it makes sense to keep it the same. I thought at first that it is just a typo. > > Steve? > >> Just my two cents... > > These details are worth noticing, thank you. > >> > new_value, selinux_enforcing, >> > from_kuid(&init_user_ns, >> > audit_get_loginuid(current)), >> > - audit_get_sessionid(current)); >> > + audit_get_sessionid(current), selinux_enabled, >> > selinux_enabled); >> > selinux_enforcing = new_value; >> > if (selinux_enforcing) >> > avc_ss_reset(0); >> > @@ -272,9 +273,11 @@ static ssize_t sel_write_disable(struct file *file, >> > const char __user *buf, >> > if (length) >> > goto out; >> > audit_log(current->audit_context, GFP_KERNEL, >> > AUDIT_MAC_STATUS, >> > - "selinux=0 auid=%u ses=%u", >> > + "enforcing=%d old_enforcing=%d auid=%u ses=%u" >> > + " enabled=%d old-enabled=%d lsm=selinux res=1", >> > + selinux_enforcing, selinux_enforcing, >> >> ^ also here >> >> > from_kuid(&init_user_ns, >> > audit_get_loginuid(current)), >> > - audit_get_sessionid(current)); >> > + audit_get_sessionid(current), 0, 1); >> > } >> > >> > length = count; >> >> Ondrej Mosnacek > > - RGB > > -- > Richard Guy Briggs > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 -- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc.
Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-10 1:34 GMT+02:00 Richard Guy Briggs : > There were two formats of the audit MAC_STATUS record, one of which was more > standard than the other. One listed enforcing status changes and the > other listed enabled status changes with a non-standard label. In > addition, the record was missing information about which LSM was > responsible and the operation's completion status. While this record is > only issued on success, the parser expects the res= field to be present. > > old enforcing/permissive: > type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 old_enforcing=1 > auid=0 ses=1 > old enable/disable: > type=MAC_STATUS msg=audit(1523312831.378:24514): selinux=0 auid=0 ses=1 > > List both sets of status and old values and add the lsm= field and the > res= field. > > Here is the new format: > type=MAC_STATUS msg=audit(1523293828.657:891): enforcing=0 old_enforcing=1 > auid=0 ses=1 enabled=1 old-enabled=1 lsm=selinux res=1 > > This record already accompanied a SYSCALL record. > > See: https://github.com/linux-audit/audit-kernel/issues/46 > Signed-off-by: Richard Guy Briggs > --- > security/selinux/selinuxfs.c | 11 +++ > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index 00eed84..00b21b2 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -145,10 +145,11 @@ static ssize_t sel_write_enforce(struct file *file, > const char __user *buf, > if (length) > goto out; > audit_log(current->audit_context, GFP_KERNEL, > AUDIT_MAC_STATUS, > - "enforcing=%d old_enforcing=%d auid=%u ses=%u", > + "enforcing=%d old_enforcing=%d auid=%u ses=%u" > + " enabled=%d old-enabled=%d lsm=selinux res=1", This is just a tiny nit but why does "old_enforcing" use an underscore and "old-enabled" a dash? Shouldn't the style be consistent across fields? Just my two cents... > new_value, selinux_enforcing, > from_kuid(&init_user_ns, audit_get_loginuid(current)), > - audit_get_sessionid(current)); > + audit_get_sessionid(current), selinux_enabled, > selinux_enabled); > selinux_enforcing = new_value; > if (selinux_enforcing) > avc_ss_reset(0); > @@ -272,9 +273,11 @@ static ssize_t sel_write_disable(struct file *file, > const char __user *buf, > if (length) > goto out; > audit_log(current->audit_context, GFP_KERNEL, > AUDIT_MAC_STATUS, > - "selinux=0 auid=%u ses=%u", > + "enforcing=%d old_enforcing=%d auid=%u ses=%u" > + " enabled=%d old-enabled=%d lsm=selinux res=1", > + selinux_enforcing, selinux_enforcing, ^ also here > from_kuid(&init_user_ns, audit_get_loginuid(current)), > - audit_get_sessionid(current)); > + audit_get_sessionid(current), 0, 1); > } > > length = count; > -- > 1.8.3.1 > > -- > Linux-audit mailing list > linux-au...@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit -- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc.
