apparmor: fix use-after-free in rawdata dedup loop
Subsystems (3)
PATCH 1/1 apparmor: fix use-after-free in rawdata dedup loop
aa_replace_profiles() walks ns->rawdata_list to dedup the incoming policy blob against entries already attached to existing profiles. Per the kernel-doc on struct aa_loaddata, list membership does not hold a reference: profiles hold pcount, and when the last pcount drops, do_ploaddata_rmfs() is queued on a workqueue that takes ns->lock and removes the entry. Between dropping the last pcount and the workqueue running, an entry remains on the list with pcount == 0.
aa_get_profile_loaddata() is an unconditional kref_get() on pcount, so when the dedup loop hits such an entry, refcount hardening reports
refcount_t: addition on 0; use-after-free.
inside aa_replace_profiles(), and the poisoned counter then trips "saturated" and "underflow" warnings on the subsequent uses of the same loaddata.
Before commit a0b7091c4de4 ("apparmor: fix race on rawdata dereference") the dedup path used a get_unless_zero-style helper on a single counter, so the existing "if (tmp)" guard was meaningful. The split-refcount refactor introduced aa_get_profile_loaddata(), which has plain kref_get() semantics, and the guard quietly became a no-op.
Introduce aa_get_profile_loaddata_not0(), matching the existing _not0 convention used by aa_get_profile_not0(), and use it for the rawdata_list dedup lookup so dying entries are skipped.
Reproduced on x86_64 with v7.1-rc5 in QEMU+KVM running Ubuntu 24.04 + stress-ng 0.17.06:
stress-ng --apparmor 1 --klog-check --timeout 60s
Without this patch the three refcount_t warnings fire within a few seconds. With it the same 60 s run is clean. Coverage is a smoke-test only; a longer soak with CONFIG_KASAN, CONFIG_KCSAN and CONFIG_PROVE_LOCKING would be welcome from anyone with the cycles.
--- security/apparmor/include/policy_unpack.h | 19 +++++++++++++++++++ security/apparmor/policy.c | 8 ++++++-- 2 files changed, 25 insertions(+), 2 deletions(-)
security/apparmor/include/policy_unpack.h +19 -0
@@ -163,6 +163,25 @@ aa_get_profile_loaddata(struct aa_loaddata *data)return data; } +/** +* aa_get_profile_loaddata_not0 - get a profile reference count if not zero +* @data: reference to get a count on +* +* Like aa_get_profile_loaddata(), but safe to call on an entry that may +* be on a list (e.g. ns->rawdata_list) where the last pcount has already +* dropped and the deferred cleanup has not yet run. +* +* Returns: pointer to reference, or %NULL if @data is NULL or its +* profile refcount has already reached zero. +*/ +static inline struct aa_loaddata * +aa_get_profile_loaddata_not0 (struct aa_loaddata *data) +{ +if (data &&kref_get_unless_zero (&data->pcount)) +return data; +return NULL ; +} +void __aa_loaddata_update (struct aa_loaddata *data,long revision);bool aa_rawdata_eq (struct aa_loaddata *l,struct aa_loaddata *r);void aa_loaddata_kref (struct kref *kref);
security/apparmor/policy.c +6 -2
@@ -1223,8 +1223,12 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,if (aa_rawdata_eq (rawdata_ent, udata)) {struct aa_loaddata *tmp; - tmp =aa_get_profile_loaddata (rawdata_ent); -/* check we didn't fail the race */ +/* +* Entries remain on rawdata_list with +* pcount == 0 until do_ploaddata_rmfs() +* runs; only take a live profile ref. +*/ + tmp =aa_get_profile_loaddata_not0 (rawdata_ent);if (tmp) {aa_put_profile_loaddata (udata); udata = tmp;
base-commit: e7ae89a0c97ce2b68b0983cd01eda67cf373517d
Signature · 1 line
2.43.0