Noting for additional clarity that I have withdrawn this RFC patchset
in favor of a later PATCH v2 with the same subject line.
On Fri, Sep 13, 2024 at 4:21 PM Ryan Lee <ryan....@canonical.com> wrote:
>
> When auditing capabilities, AppArmor uses a per-CPU, per-profile cache
> such that the same capability for the same profile doesn't get repeatedly
> audited, with the original goal of reducing audit logspam. However, this
> cache does not have an expiration time, resulting in confusion when a
> profile is shared across binaries (for example) and an expected DENIED
> audit entry doesn't appear, despite the cache entry having been populated
> much longer ago. This confusion was exacerbated by the per-CPU nature of
> the cache resulting in the expected entries sporadically appearing when
> the later denial+audit occurred on a different CPU.
>
> To resolve this, record the last time a capability was audited for a
> profile and add a timestamp expiration check before doing the audit. This
> first patch hardcodes a small duration for the timeout period.
>
> Signed-off-by: Ryan Lee <ryan....@canonical.com>
> ---
> security/apparmor/capability.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> index 7c0f66f1b297..64005b3d0fcc 100644
> --- a/security/apparmor/capability.c
> +++ b/security/apparmor/capability.c
> @@ -12,6 +12,7 @@
> #include <linux/errno.h>
> #include <linux/gfp.h>
> #include <linux/security.h>
> +#include <linux/timekeeping.h>
>
> #include "include/apparmor.h"
> #include "include/capability.h"
> @@ -33,6 +34,8 @@ struct aa_sfs_entry aa_sfs_entry_caps[] = {
> struct audit_cache {
> struct aa_profile *profile;
> kernel_cap_t caps;
> + /* Capabilities go from 0 to CAP_LAST_CAP */
> + u64 ktime_ns_last_audited[CAP_LAST_CAP+1];
> };
>
> static DEFINE_PER_CPU(struct audit_cache, audit_cache);
> @@ -65,6 +68,8 @@ static void audit_cb(struct audit_buffer *ab, void *va)
> static int audit_caps(struct apparmor_audit_data *ad, struct aa_profile
> *profile,
> int cap, int error)
> {
> + const u64 AUDIT_CACHE_TIMEOUT_NS = 100*1000; /* 100 us */
> + u64 audit_cache_expiration;
> struct aa_ruleset *rules = list_first_entry(&profile->rules,
> typeof(*rules), list);
> struct audit_cache *ent;
> @@ -90,7 +95,9 @@ static int audit_caps(struct apparmor_audit_data *ad,
> struct aa_profile *profile
>
> /* Do simple duplicate message elimination */
> ent = &get_cpu_var(audit_cache);
> - if (profile == ent->profile && cap_raised(ent->caps, cap)) {
> + audit_cache_expiration = ent->ktime_ns_last_audited[cap] +
> AUDIT_CACHE_TIMEOUT_NS;
> + if (profile == ent->profile && cap_raised(ent->caps, cap)
> + && ktime_get_ns() <= audit_cache_expiration) {
> put_cpu_var(audit_cache);
> if (COMPLAIN_MODE(profile))
> return complain_error(error);
> @@ -99,6 +106,7 @@ static int audit_caps(struct apparmor_audit_data *ad,
> struct aa_profile *profile
> aa_put_profile(ent->profile);
> ent->profile = aa_get_profile(profile);
> cap_raise(ent->caps, cap);
> + ent->ktime_ns_last_audited[cap] = ktime_get_ns();
> }
> put_cpu_var(audit_cache);
>
> --
> Major items I'm seeking input on (reason for RFC designation):
> - Whether storing a timestamp per capability is the best approach or whether
> we should do something else
> - Whether to hardcode the expiration offset or whether to expose it as a
> sysctl (see PATCH 3/3 of this series)
>
