On Sat, 2023-12-30 at 16:44 +0100, Salvatore Bonaccorso wrote:
> John, did you had a chance to work on this backport for 6.1.y stable
> upstream so we could pick it downstream in Debian in one of the next
> stable imports? Cherry-picking 1cf26c3d2c4c ("apparmor: fix apparmor
> mediating locking non-fs unix sockets") does not work, if not
> havinging the work around e2967ede2297 ("apparmor: compute policydb
> permission on profile load") AFAICS, so that needs a 6.1.y specific
> backport submitted to stable@vger.kernel.org ?
>
> I think we could have people from this bug as well providing a
> Tested-by when necessary. I'm not feeling confident enough to be able
> to provide myself such a patch to sent to stable (and you only giving
> an Acked-by/Reviewed-by), so if you can help out here with your
> upstream hat on that would be more than appreciated and welcome :)
>
> Thanks a lot for your work!
I played around with this a bit the past week as well, and came to
the same conclusion as Salvatore did that commits e2967ede2297 and
1cf26c3d2c4c need to be cherry-picked back to the 6.1 stable tree.
I've attached the two commits rebased onto 6.1.y as patches to this
message. Commit e2967ede2297 needed a little bit of touchup to apply
cleanly, and 1cf26c3d2c4c just needed adjustments for line number
changes. I included some comments at the top of each patch.
With these two commits cherry-picked on top of the 6.1.69 kernel, I
can boot a bookworm system and successfully start a service within a
container that utilizes `PrivateNetwork=yes`. Rebooting back into an
unpatched vanilla 6.1.69 kernel continues to show the problem.
While I didn't see any immediate issues (ie, `aa-status` and log
files looked OK), I don't understand the changes in the first commit
well enough to be confident in sending these patches for inclusion in
the upstream stable tree on my own.
Mathias
This is a rebase of commit e2967ede2297 ("apparmor: compute policydb permission
on profile load") onto the 6.1.69 kernel release.
The original commit had a line that changed `kvzalloc()` -> `kvcalloc()` in
security/apparmor/policy_unpack.c, but that code doesn't appear in the 6.1 tree,
so I dropped it.
Also included is the one-line declaration of `struct aa_perms default_perms` in
security/apparmor/file.c which was introduced in commit 408d53e923bd ("apparmor:
compute file permissions on profile load").
Tested-by: Mathias Gibbens <gibmat@debian.org>
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 7160e7aa5..aaba936e1 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -633,7 +633,7 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms,
state = aa_dfa_match_len(dfa, profile->policy.start[0],
match_str, match_len);
if (state)
- aa_compute_perms(dfa, state, &tmp);
+ tmp = *aa_lookup_perms(profile->policy.perms, state);
}
aa_apply_modes_to_perms(profile, &tmp);
aa_perms_accum_raw(perms, &tmp);
diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index e1b7e9360..8cf610a22 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -212,6 +212,7 @@ static u32 map_old_perms(u32 old)
*
* Returns: computed permission set
*/
+struct aa_perms default_perms = {};
struct aa_perms aa_compute_fperms(struct aa_dfa *dfa, unsigned int state,
struct path_cond *cond)
{
diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h
index 13f20c598..de9631edb 100644
--- a/security/apparmor/include/perms.h
+++ b/security/apparmor/include/perms.h
@@ -133,6 +133,17 @@ extern struct aa_perms allperms;
xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2)))
+extern struct aa_perms default_perms;
+
+static inline struct aa_perms *aa_lookup_perms(struct aa_perms *perms,
+ unsigned int state)
+{
+ if (!(perms))
+ return &default_perms;
+
+ return &(perms[state]);
+}
+
void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs,
u32 mask);
void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names,
@@ -141,8 +152,6 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs,
u32 chrsmask, const char * const *names, u32 namesmask);
void aa_apply_modes_to_perms(struct aa_profile *profile,
struct aa_perms *perms);
-void aa_compute_perms(struct aa_dfa *dfa, unsigned int state,
- struct aa_perms *perms);
void aa_perms_accum(struct aa_perms *accum, struct aa_perms *addend);
void aa_perms_accum_raw(struct aa_perms *accum, struct aa_perms *addend);
void aa_profile_match_label(struct aa_profile *profile, struct aa_label *label,
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 639b5b248..230ef5007 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -77,6 +77,7 @@ enum profile_mode {
struct aa_policydb {
/* Generic policy DFA specific rule types will be subsections of it */
struct aa_dfa *dfa;
+ struct aa_perms *perms;
unsigned int start[AA_CLASS_LAST + 1];
};
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index a67c5897e..2fc58ba15 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1330,7 +1330,7 @@ static int label_compound_match(struct aa_profile *profile,
if (!state)
goto fail;
}
- aa_compute_perms(profile->policy.dfa, state, perms);
+ *perms = *aa_lookup_perms(profile->policy.perms, state);
aa_apply_modes_to_perms(profile, perms);
if ((perms->allow & request) != request)
return -EACCES;
@@ -1381,7 +1381,7 @@ static int label_components_match(struct aa_profile *profile,
return 0;
next:
- aa_compute_perms(profile->policy.dfa, state, &tmp);
+ tmp = *aa_lookup_perms(profile->policy.perms, state);
aa_apply_modes_to_perms(profile, &tmp);
aa_perms_accum(perms, &tmp);
label_for_each_cont(i, label, tp) {
@@ -1390,7 +1390,7 @@ static int label_components_match(struct aa_profile *profile,
state = match_component(profile, tp, start);
if (!state)
goto fail;
- aa_compute_perms(profile->policy.dfa, state, &tmp);
+ tmp = *aa_lookup_perms(profile->policy.perms, state);
aa_apply_modes_to_perms(profile, &tmp);
aa_perms_accum(perms, &tmp);
}
diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
index 1c72a6110..505ef5848 100644
--- a/security/apparmor/lib.c
+++ b/security/apparmor/lib.c
@@ -315,48 +315,6 @@ void aa_apply_modes_to_perms(struct aa_profile *profile, struct aa_perms *perms)
*/
}
-static u32 map_other(u32 x)
-{
- return ((x & 0x3) << 8) | /* SETATTR/GETATTR */
- ((x & 0x1c) << 18) | /* ACCEPT/BIND/LISTEN */
- ((x & 0x60) << 19); /* SETOPT/GETOPT */
-}
-
-static u32 map_xbits(u32 x)
-{
- return ((x & 0x1) << 7) |
- ((x & 0x7e) << 9);
-}
-
-void aa_compute_perms(struct aa_dfa *dfa, unsigned int state,
- struct aa_perms *perms)
-{
- /* This mapping is convulated due to history.
- * v1-v4: only file perms
- * v5: added policydb which dropped in perm user conditional to
- * gain new perm bits, but had to map around the xbits because
- * the userspace compiler was still munging them.
- * v9: adds using the xbits in policydb because the compiler now
- * supports treating policydb permission bits different.
- * Unfortunately there is not way to force auditing on the
- * perms represented by the xbits
- */
- *perms = (struct aa_perms) {
- .allow = dfa_user_allow(dfa, state) |
- map_xbits(dfa_user_xbits(dfa, state)),
- .audit = dfa_user_audit(dfa, state),
- .quiet = dfa_user_quiet(dfa, state) |
- map_xbits(dfa_other_xbits(dfa, state)),
- };
-
- /* for v5-v9 perm mapping in the policydb, the other set is used
- * to extend the general perm set
- */
- perms->allow |= map_other(dfa_other_allow(dfa, state));
- perms->audit |= map_other(dfa_other_audit(dfa, state));
- perms->quiet |= map_other(dfa_other_quiet(dfa, state));
-}
-
/**
* aa_perms_accum_raw - accumulate perms with out masking off overlapping perms
* @accum - perms struct to accumulate into
diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index f61247241..1e978c2b1 100644
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -203,25 +203,6 @@ static unsigned int match_mnt_flags(struct aa_dfa *dfa, unsigned int state,
return state;
}
-/**
- * compute_mnt_perms - compute mount permission associated with @state
- * @dfa: dfa to match against (NOT NULL)
- * @state: state match finished in
- *
- * Returns: mount permissions
- */
-static struct aa_perms compute_mnt_perms(struct aa_dfa *dfa,
- unsigned int state)
-{
- struct aa_perms perms = {
- .allow = dfa_user_allow(dfa, state),
- .audit = dfa_user_audit(dfa, state),
- .quiet = dfa_user_quiet(dfa, state),
- };
-
- return perms;
-}
-
static const char * const mnt_info_table[] = {
"match succeeded",
"failed mntpnt match",
@@ -236,50 +217,52 @@ static const char * const mnt_info_table[] = {
* Returns 0 on success else element that match failed in, this is the
* index into the mnt_info_table above
*/
-static int do_match_mnt(struct aa_dfa *dfa, unsigned int start,
+static int do_match_mnt(struct aa_policydb *policy, unsigned int start,
const char *mntpnt, const char *devname,
const char *type, unsigned long flags,
void *data, bool binary, struct aa_perms *perms)
{
unsigned int state;
- AA_BUG(!dfa);
+ AA_BUG(!policy);
+ AA_BUG(!policy->dfa);
+ AA_BUG(!policy->perms);
AA_BUG(!perms);
- state = aa_dfa_match(dfa, start, mntpnt);
- state = aa_dfa_null_transition(dfa, state);
+ state = aa_dfa_match(policy->dfa, start, mntpnt);
+ state = aa_dfa_null_transition(policy->dfa, state);
if (!state)
return 1;
if (devname)
- state = aa_dfa_match(dfa, state, devname);
- state = aa_dfa_null_transition(dfa, state);
+ state = aa_dfa_match(policy->dfa, state, devname);
+ state = aa_dfa_null_transition(policy->dfa, state);
if (!state)
return 2;
if (type)
- state = aa_dfa_match(dfa, state, type);
- state = aa_dfa_null_transition(dfa, state);
+ state = aa_dfa_match(policy->dfa, state, type);
+ state = aa_dfa_null_transition(policy->dfa, state);
if (!state)
return 3;
- state = match_mnt_flags(dfa, state, flags);
+ state = match_mnt_flags(policy->dfa, state, flags);
if (!state)
return 4;
- *perms = compute_mnt_perms(dfa, state);
+ *perms = *aa_lookup_perms(policy->perms, state);
if (perms->allow & AA_MAY_MOUNT)
return 0;
/* only match data if not binary and the DFA flags data is expected */
if (data && !binary && (perms->allow & AA_MNT_CONT_MATCH)) {
- state = aa_dfa_null_transition(dfa, state);
+ state = aa_dfa_null_transition(policy->dfa, state);
if (!state)
return 4;
- state = aa_dfa_match(dfa, state, data);
+ state = aa_dfa_match(policy->dfa, state, data);
if (!state)
return 5;
- *perms = compute_mnt_perms(dfa, state);
+ *perms = *aa_lookup_perms(policy->perms, state);
if (perms->allow & AA_MAY_MOUNT)
return 0;
}
@@ -341,7 +324,7 @@ static int match_mnt_path_str(struct aa_profile *profile,
}
error = -EACCES;
- pos = do_match_mnt(profile->policy.dfa,
+ pos = do_match_mnt(&profile->policy,
profile->policy.start[AA_CLASS_MOUNT],
mntpnt, devname, type, flags, data, binary, &perms);
if (pos) {
@@ -601,7 +584,7 @@ static int profile_umount(struct aa_profile *profile, const struct path *path,
state = aa_dfa_match(profile->policy.dfa,
profile->policy.start[AA_CLASS_MOUNT],
name);
- perms = compute_mnt_perms(profile->policy.dfa, state);
+ perms = *aa_lookup_perms(profile->policy.perms, state);
if (AA_MAY_UMOUNT & ~perms.allow)
error = -EACCES;
@@ -672,7 +655,7 @@ static struct aa_label *build_pivotroot(struct aa_profile *profile,
new_name);
state = aa_dfa_null_transition(profile->policy.dfa, state);
state = aa_dfa_match(profile->policy.dfa, state, old_name);
- perms = compute_mnt_perms(profile->policy.dfa, state);
+ perms = *aa_lookup_perms(profile->policy.perms, state);
if (AA_MAY_PIVOTROOT & perms.allow)
error = 0;
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index 7efe4d172..88e8a7ea5 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -125,7 +125,7 @@ int aa_profile_af_perm(struct aa_profile *profile, struct common_audit_data *sa,
buffer[1] = cpu_to_be16((u16) type);
state = aa_dfa_match_len(profile->policy.dfa, state, (char *) &buffer,
4);
- aa_compute_perms(profile->policy.dfa, state, &perms);
+ perms = *aa_lookup_perms(profile->policy.perms, state);
aa_apply_modes_to_perms(profile, &perms);
return aa_check_perms(profile, &perms, request, sa, audit_net_cb);
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index c7b84fb56..f4d507d81 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -233,7 +233,7 @@ void aa_free_profile(struct aa_profile *profile)
kfree_sensitive(profile->dirname);
aa_put_dfa(profile->xmatch);
aa_put_dfa(profile->policy.dfa);
-
+ kvfree(profile->policy.perms);
if (profile->data) {
rht = profile->data;
profile->data = NULL;
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 7012fd82f..dfb1b616a 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -642,6 +642,54 @@ static int datacmp(struct rhashtable_compare_arg *arg, const void *obj)
return strcmp(data->key, *key);
}
+static u32 map_other(u32 x)
+{
+ return ((x & 0x3) << 8) | /* SETATTR/GETATTR */
+ ((x & 0x1c) << 18) | /* ACCEPT/BIND/LISTEN */
+ ((x & 0x60) << 19); /* SETOPT/GETOPT */
+}
+
+static struct aa_perms compute_perms_entry(struct aa_dfa *dfa,
+ unsigned int state)
+{
+ struct aa_perms perms = { };
+
+ perms.allow = dfa_user_allow(dfa, state);
+ perms.audit = dfa_user_audit(dfa, state);
+ perms.quiet = dfa_user_quiet(dfa, state);
+
+ /* for v5 perm mapping in the policydb, the other set is used
+ * to extend the general perm set
+ */
+
+ perms.allow |= map_other(dfa_other_allow(dfa, state));
+ perms.audit |= map_other(dfa_other_audit(dfa, state));
+ perms.quiet |= map_other(dfa_other_quiet(dfa, state));
+
+ return perms;
+}
+
+static struct aa_perms *compute_perms(struct aa_dfa *dfa)
+{
+ int state;
+ int state_count;
+ struct aa_perms *table;
+
+ AA_BUG(!dfa);
+
+ state_count = dfa->tables[YYTD_ID_BASE]->td_lolen;
+ /* DFAs are restricted from having a state_count of less than 2 */
+ table = kvcalloc(state_count, sizeof(struct aa_perms), GFP_KERNEL);
+ if (!table)
+ return NULL;
+
+ /* zero init so skip the trap state (state == 0) */
+ for (state = 1; state < state_count; state++)
+ table[state] = compute_perms_entry(dfa, state);
+
+ return table;
+}
+
/**
* unpack_profile - unpack a serialized profile
* @e: serialized data extent information (NOT NULL)
@@ -834,6 +882,11 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
goto fail;
} else
profile->policy.dfa = aa_get_dfa(nulldfa);
+ profile->policy.perms = compute_perms(profile->policy.dfa);
+ if (!profile->policy.perms) {
+ info = "failed to remap policydb permission table";
+ goto fail;
+ }
/* get file rules */
profile->file.dfa = unpack_dfa(e);
This is a rebase of commit 1cf26c3d2c4c ("apparmor: fix apparmor mediating
locking non-fs unix sockets") onto the 6.1.69 kernel release.
Tested-by: Mathias Gibbens <gibmat@debian.org>
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index dfb1b616a..e050843ff 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -31,6 +31,7 @@
#define K_ABI_MASK 0x3ff
#define FORCE_COMPLAIN_FLAG 0x800
#define VERSION_LT(X, Y) (((X) & K_ABI_MASK) < ((Y) & K_ABI_MASK))
+#define VERSION_LE(X, Y) (((X) & K_ABI_MASK) <= ((Y) & K_ABI_MASK))
#define VERSION_GT(X, Y) (((X) & K_ABI_MASK) > ((Y) & K_ABI_MASK))
#define v5 5 /* base version */
@@ -650,7 +651,8 @@ static u32 map_other(u32 x)
}
static struct aa_perms compute_perms_entry(struct aa_dfa *dfa,
- unsigned int state)
+ unsigned int state,
+ u32 version)
{
struct aa_perms perms = { };
@@ -663,13 +665,15 @@ static struct aa_perms compute_perms_entry(struct aa_dfa *dfa,
*/
perms.allow |= map_other(dfa_other_allow(dfa, state));
+ if (VERSION_LE(version, v8))
+ perms.allow |= AA_MAY_LOCK;
perms.audit |= map_other(dfa_other_audit(dfa, state));
perms.quiet |= map_other(dfa_other_quiet(dfa, state));
return perms;
}
-static struct aa_perms *compute_perms(struct aa_dfa *dfa)
+static struct aa_perms *compute_perms(struct aa_dfa *dfa, u32 version)
{
int state;
int state_count;
@@ -685,7 +689,7 @@ static struct aa_perms *compute_perms(struct aa_dfa *dfa)
/* zero init so skip the trap state (state == 0) */
for (state = 1; state < state_count; state++)
- table[state] = compute_perms_entry(dfa, state);
+ table[state] = compute_perms_entry(dfa, state, version);
return table;
}
@@ -882,7 +886,8 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
goto fail;
} else
profile->policy.dfa = aa_get_dfa(nulldfa);
- profile->policy.perms = compute_perms(profile->policy.dfa);
+ profile->policy.perms = compute_perms(profile->policy.dfa,
+ e->version);
if (!profile->policy.perms) {
info = "failed to remap policydb permission table";
goto fail;
Attachment:
signature.asc
Description: This is a digitally signed message part