On Tue, Mar 14, 2023 at 9:03 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> ---
> 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
--
paul-moore.com
