| Message ID | 20230314125734.19896-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC,v2] selinux: cache access vector decisions in the inode security blob | expand |
On Tue, Mar 14, 2023 at 9:03 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > I think Linus suggested this a long long time ago but I never got around > to trying it. Better late than never. Compute the access vector decision > when the inode security blob is initialized and cache it in the blob. > Update it on file opens. In selinux_inode_permission and inode_has_perm, > use this cached decision unless invalidated. > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > This is relative to "selinux: stop passing selinux_state pointers > and their offspring" which is not yet merged. It is now :) > There is an obvious race here; we need some form of synchronization > to ensure consistency of isec->task_sid, isec->avdsid, and > isec->avd since otherwise we could end up using the wrong access > vector decision if e.g. two tasks with different SIDs are accessing > the same file. Doing so safely without ending up with worse > overhead for selinux_inode_permission and inode_has_perm requires > care; open to suggestions here. Unfortunately, I think we're stuck having to take the inode_security_struct's spinlock to synchronize updates; or rather, nothing clever is coming to mind that doesn't introduce a bunch of unwanted complexity/overhead. With that in mind, I'm thinking more about where we might do an update when we already have the spinlock held. The obvious first candidate is inode_doinit_with_dentry(), but I believe you previously mentioned that inode_doinit_with_dentry() alone didn't have much of an impact. The other place that comes to mind is in __inode_security_revalidate() as this is already called from a fair number of places that I think we would care about, and it has the advantage of a parameter which signals if it is safe to sleep. We already call inode_doinit_with_dentry() there when it is safe and the inode needs to be revalidated, why not update the cached AVC related fields if it is safe and the inode is already valid? I'm not sure if this will result in too much cache thrashing in the inode_security_struct, or if the spinlock overhead will be unwanted in all the __inode_security_revalidate() callers, but I figured it was worth mentioning in case you hadn't already played with it. > security/selinux/avc.c | 45 +++++++++++++++--- > security/selinux/hooks.c | 79 ++++++++++++++++++++++++++----- > security/selinux/include/avc.h | 7 +++ > security/selinux/include/objsec.h | 2 + > security/selinux/ss/services.c | 3 +- > 5 files changed, 118 insertions(+), 18 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index c162e51fb43c..c74bdd76b38a 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -357,6 +357,8 @@ static int avc_xperms_populate(struct avc_node *node, > struct avc_xperms_decision_node *dest_xpd; > struct avc_xperms_decision_node *src_xpd; > > + if (!src) > + return 0; This feels like something that we might want to do anyway, although I would probably combine it with the check immediately below: if (!src || src->xp.len == 0) return 0; > if (src->xp.len == 0) > return 0; > dest = avc_xperms_alloc(); ... > @@ -1121,6 +1125,35 @@ static noinline int avc_perm_nonode(u32 ssid, u32 tsid, u16 tclass, > return 0; > } > > +/** > + * avc_get_avd - Get access vector decisions > + * @ssid: source security identifier > + * @tsid: target security identifier > + * @tclass: target security class > + * @avd: access vector decisions > + * > + * Get access vector decisions for the specified (@ssid, @tsid, @tclass) > + * triple, fetching them from the access vector cache if present or > + * calling the security server to compute them on a miss. Unlike > + * avc_has_perm_noaudit(), this function does not check any > + * requested permission; it just returns the entire decision vector. > + */ > +void avc_get_avd(u32 ssid, u32 tsid, u16 tclass, struct av_decision *avd) > +{ > + struct avc_node *node; > + > + rcu_read_lock(); > + node = avc_lookup(ssid, tsid, tclass); > + if (unlikely(!node)) { > + rcu_read_unlock(); > + avc_compute_av(ssid, tsid, tclass, avd, NULL); > + return; Out of curiosity, did you do any measurements without the avc_compute_av() call, relying solely on a previously computed entry in the AVC? > + } > + memcpy(avd, &node->ae.avd, sizeof(*avd)); > + rcu_read_unlock(); > +} > + > + > /** > * avc_has_perm_noaudit - Check permissions but perform no auditing. > * @ssid: source security identifier ... > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index f14d1ffe54c5..7353c027c389 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -1107,7 +1107,8 @@ void security_compute_av(u32 ssid, > rcu_read_lock(); > policy = rcu_dereference(selinux_state.policy); > avd_init(policy, avd); > - xperms->len = 0; > + if (xperms) > + xperms->len = 0; Similar to the comment for avc_xperms_populate(), I wonder if this is something we want to check regardless. > if (!selinux_initialized()) > goto allow; > > -- > 2.39.2
On Tue, Mar 14, 2023 at 4:43 PM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Mar 14, 2023 at 9:03 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > I think Linus suggested this a long long time ago but I never got around > > to trying it. Better late than never. Compute the access vector decision > > when the inode security blob is initialized and cache it in the blob. > > Update it on file opens. In selinux_inode_permission and inode_has_perm, > > use this cached decision unless invalidated. > > > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > --- > > There is an obvious race here; we need some form of synchronization > > to ensure consistency of isec->task_sid, isec->avdsid, and > > isec->avd since otherwise we could end up using the wrong access > > vector decision if e.g. two tasks with different SIDs are accessing > > the same file. Doing so safely without ending up with worse > > overhead for selinux_inode_permission and inode_has_perm requires > > care; open to suggestions here. > > Unfortunately, I think we're stuck having to take the > inode_security_struct's spinlock to synchronize updates; or rather, > nothing clever is coming to mind that doesn't introduce a bunch of > unwanted complexity/overhead. > > With that in mind, I'm thinking more about where we might do an update > when we already have the spinlock held. The obvious first candidate > is inode_doinit_with_dentry(), but I believe you previously mentioned > that inode_doinit_with_dentry() alone didn't have much of an impact. > The other place that comes to mind is in __inode_security_revalidate() > as this is already called from a fair number of places that I think we > would care about, and it has the advantage of a parameter which > signals if it is safe to sleep. We already call > inode_doinit_with_dentry() there when it is safe and the inode needs > to be revalidated, why not update the cached AVC related fields if it > is safe and the inode is already valid? I'm not sure if this will > result in too much cache thrashing in the inode_security_struct, or if > the spinlock overhead will be unwanted in all the > __inode_security_revalidate() callers, but I figured it was worth > mentioning in case you hadn't already played with it. We are already taking the isec->lock (or otherwise have exclusive access to a newly initialized isec) in all the places where we are updating the isec->avd and isec->avdsid. The issue is not the updates but rather the reads in inode_has_perm() and selinux_inode_permission(). Would prefer some lockless synchronization there as otherwise I fear we are making things worse rather than better. Need to ensure that the (isec->task_sid, isec->avdsid, isec->avd) triple is consistent (i.e. that the isec->avd represents the access vector decisions computed based on the isec->task_sid and isec->avdsid) and that the (sid, isec->sid, avc_policy_seqno()) triple matches the (isec->task_sid, isec->avdsid, isec->avd.seqno) triple in order to use that avd. > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > > index c162e51fb43c..c74bdd76b38a 100644 > > --- a/security/selinux/avc.c > > +++ b/security/selinux/avc.c > > @@ -357,6 +357,8 @@ static int avc_xperms_populate(struct avc_node *node, > > struct avc_xperms_decision_node *dest_xpd; > > struct avc_xperms_decision_node *src_xpd; > > > > + if (!src) > > + return 0; > > This feels like something that we might want to do anyway, although I > would probably combine it with the check immediately below: > > if (!src || src->xp.len == 0) > return 0; Yes, can do that in the next revision. > > @@ -1121,6 +1125,35 @@ static noinline int avc_perm_nonode(u32 ssid, u32 tsid, u16 tclass, > > return 0; > > } > > > > +/** > > + * avc_get_avd - Get access vector decisions > > + * @ssid: source security identifier > > + * @tsid: target security identifier > > + * @tclass: target security class > > + * @avd: access vector decisions > > + * > > + * Get access vector decisions for the specified (@ssid, @tsid, @tclass) > > + * triple, fetching them from the access vector cache if present or > > + * calling the security server to compute them on a miss. Unlike > > + * avc_has_perm_noaudit(), this function does not check any > > + * requested permission; it just returns the entire decision vector. > > + */ > > +void avc_get_avd(u32 ssid, u32 tsid, u16 tclass, struct av_decision *avd) > > +{ > > + struct avc_node *node; > > + > > + rcu_read_lock(); > > + node = avc_lookup(ssid, tsid, tclass); > > + if (unlikely(!node)) { > > + rcu_read_unlock(); > > + avc_compute_av(ssid, tsid, tclass, avd, NULL); > > + return; > > Out of curiosity, did you do any measurements without the > avc_compute_av() call, relying solely on a previously computed entry > in the AVC? The approach here is to call avc_get_avd() on isec initialization/update and upon file open to cache the access vector decisions at that time for later use upon inode_has_perm and selinux_inode_permission. So avc_get_avd() is just avc_has_perm_noaudit() w/o actually checking any specific requested permissions. Like avc_has_perm_noaudit(), it first tries to look up the entry in the AVC and only falls back to avc_compute_av() on a cache miss. avc_get_avd() is not getting called during selinux_inode_permission itself. If we don't call avc_compute_av() in the cache miss case, then we'll have to do so at selinux_inode_permission and inode_has_perm() time, in which case we'll pay a higher cost there and that's the more performance-critical path. > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index f14d1ffe54c5..7353c027c389 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -1107,7 +1107,8 @@ void security_compute_av(u32 ssid, > > rcu_read_lock(); > > policy = rcu_dereference(selinux_state.policy); > > avd_init(policy, avd); > > - xperms->len = 0; > > + if (xperms) > > + xperms->len = 0; > > Similar to the comment for avc_xperms_populate(), I wonder if this is > something we want to check regardless. Agree, just wasn't needed until this change.
On Wed, Mar 15, 2023 at 5:33 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > We are already taking the isec->lock (or otherwise have exclusive > access to a newly initialized isec) in all the places where we are > updating the isec->avd and isec->avdsid. The issue is not the updates > but rather the reads in inode_has_perm() and > selinux_inode_permission(). Right. And that is always going to be unordered, in the sense that you will get "one or the other" value whether you have strict locking or not. So even with a spinlock around the actual low-level selinux data reads and writes, there is only "data consistency", not any actual *ordering*. The pathname lookup itself is simply not ordered (and cannot in any sane model be ordered) wrt somebody else coming in and changing any selinux rules. So I don't think this is even worth worrying about. There is no ordering, because no ordering can possibly exist. The only thing that can matter is consistency: any *individual* security decision should either get the old rules or the new rules (never a mix of the two), but either of those is fine - and as you traverse a whole pathname and do multiple different security decisions for each path component (and for the final open), you *will* get a mixture of old and new if the rules are updated concurrently. I don't think this is a problem, and I don't even think it's fixable (sure, in theory, we could do some big sequence number lock or similar, but no way do we actually want to do that for path lookup in reality). Linus
On Wed, Mar 15, 2023 at 1:37 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Mar 15, 2023 at 5:33 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > We are already taking the isec->lock (or otherwise have exclusive > > access to a newly initialized isec) in all the places where we are > > updating the isec->avd and isec->avdsid. The issue is not the updates > > but rather the reads in inode_has_perm() and > > selinux_inode_permission(). > > Right. > > And that is always going to be unordered, in the sense that you will > get "one or the other" value whether you have strict locking or not. > > So even with a spinlock around the actual low-level selinux data reads > and writes, there is only "data consistency", not any actual > *ordering*. The pathname lookup itself is simply not ordered (and > cannot in any sane model be ordered) wrt somebody else coming in and > changing any selinux rules. > > So I don't think this is even worth worrying about. There is no > ordering, because no ordering can possibly exist. > > The only thing that can matter is consistency: any *individual* > security decision should either get the old rules or the new rules > (never a mix of the two), but either of those is fine - and as you > traverse a whole pathname and do multiple different security decisions > for each path component (and for the final open), you *will* get a > mixture of old and new if the rules are updated concurrently. > > I don't think this is a problem, and I don't even think it's fixable > (sure, in theory, we could do some big sequence number lock or > similar, but no way do we actually want to do that for path lookup in > reality). So my primary concern is wrt consistency of the (isec->task_sid, isec->avdsid, isec->avd) triple. In particular, if we have two or more tasks with different SIDs accessing the same inode, then selinux_file_open() is going to update those values on each open(), and inode_has_perm() and selinux_inode_permission() may see inconsistent states with a mixture of the old and new SID pair and access vectors that will result in permission being incorrectly allowed or denied. I'm not sure how to safely prevent this in a manner that doesn't impose too much overhead on inode_has_perm() and selinux_inode_permission().
On Wed, Mar 15, 2023 at 2:05 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Wed, Mar 15, 2023 at 1:37 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > On Wed, Mar 15, 2023 at 5:33 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > > > > We are already taking the isec->lock (or otherwise have exclusive > > > access to a newly initialized isec) in all the places where we are > > > updating the isec->avd and isec->avdsid. The issue is not the updates > > > but rather the reads in inode_has_perm() and > > > selinux_inode_permission(). > > > > Right. > > > > And that is always going to be unordered, in the sense that you will > > get "one or the other" value whether you have strict locking or not. > > > > So even with a spinlock around the actual low-level selinux data reads > > and writes, there is only "data consistency", not any actual > > *ordering*. The pathname lookup itself is simply not ordered (and > > cannot in any sane model be ordered) wrt somebody else coming in and > > changing any selinux rules. > > > > So I don't think this is even worth worrying about. There is no > > ordering, because no ordering can possibly exist. > > > > The only thing that can matter is consistency: any *individual* > > security decision should either get the old rules or the new rules > > (never a mix of the two), but either of those is fine - and as you > > traverse a whole pathname and do multiple different security decisions > > for each path component (and for the final open), you *will* get a > > mixture of old and new if the rules are updated concurrently. > > > > I don't think this is a problem, and I don't even think it's fixable > > (sure, in theory, we could do some big sequence number lock or > > similar, but no way do we actually want to do that for path lookup in > > reality). > > So my primary concern is wrt consistency of the (isec->task_sid, > isec->avdsid, isec->avd) triple. Agreed. FWIW, I thought you were looking for a different approach for the writers in your original mail. Considering the multiple cache fields, and the consistency requirements, I think the only realistic solution would be to wrap them in their own struct/pointer referenced by the inode_security_struct and use RCU on the read side. Different readers would obviously have the potential for different views, but the cached values would at least be consistent for any given reader. The downside would be the allocations necessary when updating the cache, which I'm guessing would wipe out any gains from the cache ... but that's just a guess. > In particular, if we have two or more > tasks with different SIDs accessing the same inode, then > selinux_file_open() is going to update those values on each open(), > and inode_has_perm() and selinux_inode_permission() may see > inconsistent states with a mixture of the old and new SID pair and > access vectors that will result in permission being incorrectly > allowed or denied. I'm not sure how to safely prevent this in a manner > that doesn't impose too much overhead on inode_has_perm() and > selinux_inode_permission().
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index c162e51fb43c..c74bdd76b38a 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -357,6 +357,8 @@ static int avc_xperms_populate(struct avc_node *node, struct avc_xperms_decision_node *dest_xpd; struct avc_xperms_decision_node *src_xpd; + if (!src) + return 0; if (src->xp.len == 0) return 0; dest = avc_xperms_alloc(); @@ -988,15 +990,17 @@ static noinline struct avc_node *avc_compute_av(u32 ssid, u32 tsid, u16 tclass, struct av_decision *avd, struct avc_xperms_node *xp_node) { - INIT_LIST_HEAD(&xp_node->xpd_head); - security_compute_av(ssid, tsid, tclass, avd, &xp_node->xp); + if (xp_node) + INIT_LIST_HEAD(&xp_node->xpd_head); + security_compute_av(ssid, tsid, tclass, avd, + xp_node ? &xp_node->xp : NULL); return avc_insert(ssid, tsid, tclass, avd, xp_node); } -static noinline int avc_denied(u32 ssid, u32 tsid, - u16 tclass, u32 requested, - u8 driver, u8 xperm, unsigned int flags, - struct av_decision *avd) +noinline int avc_denied(u32 ssid, u32 tsid, + u16 tclass, u32 requested, + u8 driver, u8 xperm, unsigned int flags, + struct av_decision *avd) { if (flags & AVC_STRICT) return -EACCES; @@ -1121,6 +1125,35 @@ static noinline int avc_perm_nonode(u32 ssid, u32 tsid, u16 tclass, return 0; } +/** + * avc_get_avd - Get access vector decisions + * @ssid: source security identifier + * @tsid: target security identifier + * @tclass: target security class + * @avd: access vector decisions + * + * Get access vector decisions for the specified (@ssid, @tsid, @tclass) + * triple, fetching them from the access vector cache if present or + * calling the security server to compute them on a miss. Unlike + * avc_has_perm_noaudit(), this function does not check any + * requested permission; it just returns the entire decision vector. + */ +void avc_get_avd(u32 ssid, u32 tsid, u16 tclass, struct av_decision *avd) +{ + struct avc_node *node; + + rcu_read_lock(); + node = avc_lookup(ssid, tsid, tclass); + if (unlikely(!node)) { + rcu_read_unlock(); + avc_compute_av(ssid, tsid, tclass, avd, NULL); + return; + } + memcpy(avd, &node->ae.avd, sizeof(*avd)); + rcu_read_unlock(); +} + + /** * avc_has_perm_noaudit - Check permissions but perform no auditing. * @ssid: source security identifier diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index db6d8b68b543..f7341f87ea99 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -793,6 +793,9 @@ static int selinux_set_mnt_opts(struct super_block *sb, goto out; root_isec->sid = rootcontext_sid; + avc_get_avd(root_isec->task_sid, root_isec->sid, + root_isec->sclass, &root_isec->avd); + root_isec->avdsid = root_isec->sid; root_isec->initialized = LABEL_INITIALIZED; } @@ -1517,8 +1520,10 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent isec->initialized = LABEL_INVALID; goto out_unlock; } - isec->initialized = LABEL_INITIALIZED; isec->sid = sid; + avc_get_avd(task_sid, sid, sclass, &isec->avd); + isec->avdsid = sid; + isec->initialized = LABEL_INITIALIZED; } out_unlock: @@ -1611,7 +1616,9 @@ static int inode_has_perm(const struct cred *cred, struct common_audit_data *adp) { struct inode_security_struct *isec; - u32 sid; + u32 sid, denied; + struct av_decision avd; + int rc, rc2; validate_creds(cred); @@ -1621,6 +1628,21 @@ static int inode_has_perm(const struct cred *cred, sid = cred_sid(cred); isec = selinux_inode(inode); + if (sid == isec->task_sid && isec->sid == isec->avdsid && + isec->avd.seqno == avc_policy_seqno()) { + memcpy(&avd, &isec->avd, sizeof(avd)); + denied = perms & ~avd.allowed; + if (unlikely(denied)) + rc = avc_denied(sid, isec->sid, isec->sclass, + perms, 0, 0, 0, &avd); + else + rc = 0; + rc2 = avc_audit(sid, isec->sid, isec->sclass, perms, &avd, rc, adp); + if (rc2) + return rc2; + return rc; + } + return avc_has_perm(sid, isec->sid, isec->sclass, perms, adp); } @@ -2851,6 +2873,8 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, struct inode_security_struct *isec = selinux_inode(inode); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = newsid; + avc_get_avd(tsec->sid, newsid, isec->sclass, &isec->avd); + isec->avdsid = newsid; isec->initialized = LABEL_INITIALIZED; } @@ -2912,20 +2936,20 @@ static int selinux_inode_init_security_anon(struct inode *inode, return rc; } - isec->initialized = LABEL_INITIALIZED; /* * Now that we've initialized security, check whether we're * allowed to actually create this type of anonymous inode. */ + rc = avc_has_perm_noaudit(tsec->sid, isec->sid, isec->sclass, + FILE__CREATE, 0, &isec->avd); + + isec->initialized = LABEL_INITIALIZED; ad.type = LSM_AUDIT_DATA_ANONINODE; ad.u.anonclass = name ? (const char *)name->name : "?"; - - return avc_has_perm(tsec->sid, - isec->sid, - isec->sclass, - FILE__CREATE, - &ad); + avc_audit(tsec->sid, isec->sid, isec->sclass, FILE__CREATE, &isec->avd, + rc, &ad); + return rc; } static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode) @@ -3041,8 +3065,19 @@ static int selinux_inode_permission(struct inode *inode, int mask) if (IS_ERR(isec)) return PTR_ERR(isec); - rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, - &avd); + if (sid == isec->task_sid && isec->sid == isec->avdsid && + isec->avd.seqno == avc_policy_seqno()) { + memcpy(&avd, &isec->avd, sizeof(avd)); + denied = perms & ~avd.allowed; + if (unlikely(denied)) + rc = avc_denied(sid, isec->sid, isec->sclass, + perms, 0, 0, 0, &avd); + else + rc = 0; + } else { + rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, + 0, &avd); + } audited = avc_audit_required(perms, &avd, rc, from_access ? FILE__AUDIT_ACCESS : 0, &denied); @@ -3247,6 +3282,8 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, spin_lock(&isec->lock); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = newsid; + avc_get_avd(isec->task_sid, newsid, isec->sclass, &isec->avd); + isec->avdsid = newsid; isec->initialized = LABEL_INITIALIZED; spin_unlock(&isec->lock); } @@ -3406,6 +3443,8 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name, spin_lock(&isec->lock); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = newsid; + avc_get_avd(isec->task_sid, newsid, isec->sclass, &isec->avd); + isec->avdsid = newsid; isec->initialized = LABEL_INITIALIZED; spin_unlock(&isec->lock); return 0; @@ -3874,6 +3913,18 @@ static int selinux_file_open(struct file *file) */ fsec->isid = isec->sid; fsec->pseqno = avc_policy_seqno(); + + /* + * Update inode task SID and avd to reflect opener for later + * use in selinux_inode_permission and inode_has_perm. + */ + spin_lock(&isec->lock); + isec->task_sid = fsec->sid; + isec->avdsid = isec->sid; + avc_get_avd(isec->task_sid, isec->avdsid, + isec->sclass, &isec->avd); + spin_unlock(&isec->lock); + /* * Since the inode label or policy seqno may have changed * between the selinux_inode_permission check and the saving @@ -4162,6 +4213,8 @@ static void selinux_task_to_inode(struct task_struct *p, spin_lock(&isec->lock); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = sid; + avc_get_avd(isec->task_sid, sid, isec->sclass, &isec->avd); + isec->avdsid = sid; isec->initialized = LABEL_INITIALIZED; spin_unlock(&isec->lock); } @@ -4534,6 +4587,8 @@ static int selinux_socket_post_create(struct socket *sock, int family, isec->sclass = sclass; isec->sid = sid; + avc_get_avd(isec->task_sid, sid, sclass, &isec->avd); + isec->avdsid = sid; isec->initialized = LABEL_INITIALIZED; if (sock->sk) { @@ -4827,6 +4882,8 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock) newisec = inode_security_novalidate(SOCK_INODE(newsock)); newisec->sclass = sclass; newisec->sid = sid; + avc_get_avd(newisec->task_sid, sid, sclass, &newisec->avd); + newisec->avdsid = sid; newisec->initialized = LABEL_INITIALIZED; return 0; diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h index 9301222c8e55..14fc5ed1a156 100644 --- a/security/selinux/include/avc.h +++ b/security/selinux/include/avc.h @@ -141,6 +141,13 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid, unsigned flags, struct av_decision *avd); +void avc_get_avd(u32 ssid, u32 tsid, u16 tclass, struct av_decision *avd); + +int avc_denied(u32 ssid, u32 tsid, + u16 tclass, u32 requested, + u8 driver, u8 xperm, unsigned int flags, + struct av_decision *avd); + int avc_has_perm(u32 ssid, u32 tsid, u16 tclass, u32 requested, struct common_audit_data *auditdata); diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h index 2953132408bf..da95a75e6c77 100644 --- a/security/selinux/include/objsec.h +++ b/security/selinux/include/objsec.h @@ -50,6 +50,8 @@ struct inode_security_struct { u32 sid; /* SID of this object */ u16 sclass; /* security class of this object */ unsigned char initialized; /* initialization flag */ + u32 avdsid; /* SID when avd was computed */ + struct av_decision avd; /* access vector decisions */ spinlock_t lock; }; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index f14d1ffe54c5..7353c027c389 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -1107,7 +1107,8 @@ void security_compute_av(u32 ssid, rcu_read_lock(); policy = rcu_dereference(selinux_state.policy); avd_init(policy, avd); - xperms->len = 0; + if (xperms) + xperms->len = 0; if (!selinux_initialized()) goto allow;
I think Linus suggested this a long long time ago but I never got around to trying it. Better late than never. Compute the access vector decision when the inode security blob is initialized and cache it in the blob. Update it on file opens. In selinux_inode_permission and inode_has_perm, use this cached decision unless invalidated. Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- This is relative to "selinux: stop passing selinux_state pointers and their offspring" which is not yet merged. There is an obvious race here; we need some form of synchronization to ensure consistency of isec->task_sid, isec->avdsid, and isec->avd since otherwise we could end up using the wrong access vector decision if e.g. two tasks with different SIDs are accessing the same file. Doing so safely without ending up with worse overhead for selinux_inode_permission and inode_has_perm requires care; open to suggestions here. security/selinux/avc.c | 45 +++++++++++++++--- security/selinux/hooks.c | 79 ++++++++++++++++++++++++++----- security/selinux/include/avc.h | 7 +++ security/selinux/include/objsec.h | 2 + security/selinux/ss/services.c | 3 +- 5 files changed, 118 insertions(+), 18 deletions(-)