[apparmor] [PATCH AUTOSEL 6.16-6.12] apparmor: fix x_table_lookup when stacking is not the first entry
Sasha Levin
sashal at kernel.org
Fri Aug 8 15:30:52 UTC 2025
From: John Johansen <john.johansen at canonical.com>
[ Upstream commit a9eb185be84e998aa9a99c7760534ccc06216705 ]
x_table_lookup currently does stacking during label_parse() if the
target specifies a stack but its only caller ensures that it will
never be used with stacking.
Refactor to slightly simplify the code in x_to_label(), this
also fixes a long standing problem where x_to_labels check on stacking
is only on the first element to the table option list, instead of
the element that is found and used.
Signed-off-by: John Johansen <john.johansen at canonical.com>
Signed-off-by: Sasha Levin <sashal at kernel.org>
---
LLM Generated explanations, may be completely bogus:
**Backport Status: YES**
This commit should be backported to stable kernel trees for the
following reasons:
## Bug Fix Analysis
### 1. **Critical Bug Fix**
This fixes a **long-standing bug** in AppArmor's domain transition
handling where stacking directives would fail when they weren't the
first entry in the transition table. The commit message explicitly
states this fixes a "long standing problem" where the stacking check was
only performed on the first element rather than the element actually
found and used.
### 2. **Security Impact**
- **Policy Bypass Risk**: The bug could cause incorrect security
profiles to be applied during domain transitions, potentially allowing
unauthorized access
- **Inconsistent Enforcement**: Applications might not receive the
correct stacked security context, leading to security policy
violations
- **Profile Selection Errors**: Wrong profiles could be selected when
stacking is involved, compromising the intended security boundaries
### 3. **Small and Contained Fix**
The changes are:
- Limited to two functions in a single file
(`security/apparmor/domain.c`)
- Focused on fixing the logic flow without introducing new features
- Clear refactoring that separates concerns properly
### 4. **Low Regression Risk**
- The fix simplifies the code by properly separating lookup from
stacking logic
- Changes from `aa_label_parse()` to `aa_label_merge()` for stacking is
the correct semantic operation
- Proper resource management with `aa_put_label(stack)` prevents memory
leaks
- The refactored loop in `x_table_lookup()` is cleaner and easier to
understand
### 5. **Real-World Impact**
This bug would manifest in production systems where:
- AppArmor profiles use stacking features
- The stacking directive appears after other entries in the transition
table
- Domain transitions are critical for security isolation
### 6. **Clear Problem and Solution**
The commit clearly identifies:
- **Problem**: Stacking check only on first element, not the found
element
- **Solution**: Proper separation of lookup and stacking logic, checking
the actual found entry
### 7. **Maintainer Authorship**
The commit is authored by John Johansen, the AppArmor maintainer,
indicating authoritative understanding of the subsystem and the fix's
importance.
This is exactly the type of commit that stable trees should include: a
clear bug fix for a security-relevant subsystem that could cause policy
enforcement failures in production systems, with minimal code changes
and low regression risk.
security/apparmor/domain.c | 52 +++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 23 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 5939bd9a9b9b..08ca9057f82b 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -508,6 +508,7 @@ static const char *next_name(int xtype, const char *name)
* @name: returns: name tested to find label (NOT NULL)
*
* Returns: refcounted label, or NULL on failure (MAYBE NULL)
+ * @name will always be set with the last name tried
*/
struct aa_label *x_table_lookup(struct aa_profile *profile, u32 xindex,
const char **name)
@@ -517,6 +518,7 @@ struct aa_label *x_table_lookup(struct aa_profile *profile, u32 xindex,
struct aa_label *label = NULL;
u32 xtype = xindex & AA_X_TYPE_MASK;
int index = xindex & AA_X_INDEX_MASK;
+ const char *next;
AA_BUG(!name);
@@ -524,25 +526,27 @@ struct aa_label *x_table_lookup(struct aa_profile *profile, u32 xindex,
/* TODO: move lookup parsing to unpack time so this is a straight
* index into the resultant label
*/
- for (*name = rules->file->trans.table[index]; !label && *name;
- *name = next_name(xtype, *name)) {
+ for (next = rules->file->trans.table[index]; next;
+ next = next_name(xtype, next)) {
+ const char *lookup = (*next == '&') ? next + 1 : next;
+ *name = next;
if (xindex & AA_X_CHILD) {
- struct aa_profile *new_profile;
- /* release by caller */
- new_profile = aa_find_child(profile, *name);
- if (new_profile)
- label = &new_profile->label;
+ /* TODO: switich to parse to get stack of child */
+ struct aa_profile *new = aa_find_child(profile, lookup);
+
+ if (new)
+ /* release by caller */
+ return &new->label;
continue;
}
- label = aa_label_parse(&profile->label, *name, GFP_KERNEL,
+ label = aa_label_parse(&profile->label, lookup, GFP_KERNEL,
true, false);
- if (IS_ERR(label))
- label = NULL;
+ if (!IS_ERR_OR_NULL(label))
+ /* release by caller */
+ return label;
}
- /* released by caller */
-
- return label;
+ return NULL;
}
/**
@@ -567,9 +571,9 @@ static struct aa_label *x_to_label(struct aa_profile *profile,
struct aa_ruleset *rules = list_first_entry(&profile->rules,
typeof(*rules), list);
struct aa_label *new = NULL;
+ struct aa_label *stack = NULL;
struct aa_ns *ns = profile->ns;
u32 xtype = xindex & AA_X_TYPE_MASK;
- const char *stack = NULL;
switch (xtype) {
case AA_X_NONE:
@@ -578,13 +582,14 @@ static struct aa_label *x_to_label(struct aa_profile *profile,
break;
case AA_X_TABLE:
/* TODO: fix when perm mapping done at unload */
- stack = rules->file->trans.table[xindex & AA_X_INDEX_MASK];
- if (*stack != '&') {
- /* released by caller */
- new = x_table_lookup(profile, xindex, lookupname);
- stack = NULL;
+ /* released by caller
+ * if null for both stack and direct want to try fallback
+ */
+ new = x_table_lookup(profile, xindex, lookupname);
+ if (!new || **lookupname != '&')
break;
- }
+ stack = new;
+ new = NULL;
fallthrough; /* to X_NAME */
case AA_X_NAME:
if (xindex & AA_X_CHILD)
@@ -599,6 +604,7 @@ static struct aa_label *x_to_label(struct aa_profile *profile,
break;
}
+ /* fallback transition check */
if (!new) {
if (xindex & AA_X_INHERIT) {
/* (p|c|n)ix - don't change profile but do
@@ -617,12 +623,12 @@ static struct aa_label *x_to_label(struct aa_profile *profile,
/* base the stack on post domain transition */
struct aa_label *base = new;
- new = aa_label_parse(base, stack, GFP_KERNEL, true, false);
- if (IS_ERR(new))
- new = NULL;
+ new = aa_label_merge(base, stack, GFP_KERNEL);
+ /* null on error */
aa_put_label(base);
}
+ aa_put_label(stack);
/* released by caller */
return new;
}
--
2.39.5
More information about the AppArmor
mailing list