apparmor: fix write check, and implement runtime fallback

When checkpointing tasks confined by stacked AppArmor profiles inside an
AppArmor namespace, CRIU compiles a permissive parasite profile using
apparmor_parser and writes it to the namespace policy replacement
interface
(/sys/kernel/security/apparmor/policy/namespaces/<ns>/.replace).

Previously, get_suspend_policy() invoked apparmor_parser with flags
(-QWL) requesting cache file generation and read the resulting cache
file. On modern AppArmor 4.x kernels, cache files include cache metadata
headers that cause the kernel policy unpacker to reject the binary blob
with EPROTONOSUPPORT (-93, "invalid profile format"). Replace the cache
mechanism by invoking apparmor_parser with the -o flag to output a clean
binary policy stream directly.

Furthermore, when writing a replacement profile blob to an AppArmor
.replace interface, successful writes return the total size of the
profile load data structure consumed or updated in the kernel (e.g.,
5233 bytes for stacked profiles), rather than the exact byte length of
the input payload (e.g., 727 bytes for a parasite profile). Checking if
(n != len) erroneously treats a successful replacement write as a
failure. Fix policy write validation to check if (n < 0).

Across Linux kernel versions, AppArmor enforces different requirements
for policy replacement blobs written to a namespace .replace interface:
- Older kernels (AppArmor 3.x / early 4.x on Ubuntu 22.04) expect blobs
  compiled without namespace headers (-n <ns>); specifying -n causes
  the kernel to load the replacement into a nested sub-namespace
  (<ns>//<ns>), leaving the active profile unchanged and causing socket
  permission failures during dump.
- Modern kernels (AppArmor 4.x on Ubuntu 26.04) reject blobs compiled
  without namespace headers with EPROTONOSUPPORT (-93, "invalid profile
  format").

Update get_suspend_policy() and write_aa_policy() to implement runtime
fallback: first try compiling without -n <ns> and writing directly to
the namespace interface for older kernels. If writing fails (n < 0),
retry compilation passing -n <ns> and write to the root interface to
satisfy modern kernels.

In addition, when restoring tasks confined by profiles inside an
AppArmor namespace on AppArmor 4.x kernels, the host root unconfined
profile remains stacked with the target namespace profile when viewed
from the root namespace (e.g., "unconfined//&:ns:profile"). Update
checkprofile() in the apparmor_stacking test to recognize and allow the
"unconfined//&" prefix when verifying restored profiles.

Assisted-by: Gemini
Signed-off-by: Andrei Vagin <avagin@google.com>

criu/apparmor.c

@@ -359,38 +359,35 @@ err:
 	 "}\n")
 
 char policydir[PATH_MAX] = ".criu.temp-aa-policy.XXXXXX";
-char cachedir[PATH_MAX];
 
 struct apparmor_parser_args {
-	char *cache;
+	char *bin_file;
 	char *file;
+	char *ns;
 };
 
 static int apparmor_parser_exec(void *data)
 {
 	struct apparmor_parser_args *args = data;
 
-	execlp("apparmor_parser", "apparmor_parser", "-QWL", args->cache, args->file, NULL);
+	if (args->ns)
+		execlp("apparmor_parser", "apparmor_parser", "-n", args->ns, "-o", args->bin_file, args->file, NULL);
+	else
+		execlp("apparmor_parser", "apparmor_parser", "-o", args->bin_file, args->file, NULL);
 
 	return -1;
 }
 
-static int apparmor_cache_exec(void *data)
+static void *get_suspend_policy(char *name, char *ns_name, bool use_ns_flag, off_t *len)
 {
-	execlp("apparmor_parser", "apparmor_parser", "--cache-loc", "/", "--print-cache-dir", (char *)NULL);
-
-	return -1;
-}
-
-static void *get_suspend_policy(char *name, off_t *len)
-{
-	char policy[1024], file[PATH_MAX], cache[PATH_MAX], clean_name[PATH_MAX];
+	char policy[1024], file[PATH_MAX], bin_file[PATH_MAX], clean_name[PATH_MAX];
 	void *ret = NULL;
 	int n, fd, policy_len, i;
 	struct stat sb;
 	struct apparmor_parser_args args = {
-		.cache = cache,
+		.bin_file = bin_file,
 		.file = file,
+		.ns = use_ns_flag ? ns_name : NULL,
 	};
 
 	*len = 0;
@@ -413,18 +410,18 @@ static void *get_suspend_policy(char *name, off_t *len)
 	clean_name[i] = 0;
 
 	n = snprintf(file, sizeof(file), "%s/%s", policydir, clean_name);
-	if (n < 0 || n >= sizeof(policy)) {
+	if (n < 0 || n >= sizeof(file)) {
 		pr_err("policy name %s too long\n", clean_name);
 		return NULL;
 	}
 
-	n = snprintf(cache, sizeof(cache), "%s/cache", policydir);
-	if (n < 0 || n >= sizeof(policy)) {
-		pr_err("policy dir too long\n");
+	n = snprintf(bin_file, sizeof(bin_file), "%s/%s.bin", policydir, clean_name);
+	if (n < 0 || n >= sizeof(bin_file)) {
+		pr_err("policy bin name %s too long\n", clean_name);
 		return NULL;
 	}
 
-	fd = open(file, O_CREAT | O_WRONLY, 0600);
+	fd = open(file, O_CREAT | O_WRONLY | O_TRUNC, 0600);
 	if (fd < 0) {
 		pr_perror("couldn't create %s", file);
 		return NULL;
@@ -437,11 +434,7 @@ static void *get_suspend_policy(char *name, off_t *len)
 		return NULL;
 	}
 
-	n = run_command(cachedir, sizeof(cachedir), apparmor_cache_exec, NULL);
-	if (n < 0) {
-		pr_err("apparmor parsing failed %d\n", n);
-		return NULL;
-	}
+	unlink(bin_file);
 
 	n = run_command(NULL, 0, apparmor_parser_exec, &args);
 	if (n < 0) {
@@ -449,15 +442,9 @@ static void *get_suspend_policy(char *name, off_t *len)
 		return NULL;
 	}
 
-	n = snprintf(file, sizeof(file), "%s/cache/%s/%s", policydir, cachedir, clean_name);
-	if (n < 0 || n >= sizeof(policy)) {
-		pr_err("policy name %s too long\n", clean_name);
-		return NULL;
-	}
-
-	fd = open(file, O_RDONLY);
+	fd = open(bin_file, O_RDONLY);
 	if (fd < 0) {
-		pr_perror("couldn't open %s", file);
+		pr_perror("couldn't open %s", bin_file);
 		return NULL;
 	}
 
@@ -465,15 +452,16 @@ static void *get_suspend_policy(char *name, off_t *len)
 		pr_perror("couldn't stat fd");
 		goto out;
 	}
+	pr_debug("opened compiled suspend policy %s (size %ld)\n", bin_file, (long)sb.st_size);
 
 	ret = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if (ret == MAP_FAILED) {
-		pr_perror("mmap of %s failed", file);
+		pr_perror("mmap of %s failed", bin_file);
 		ret = NULL;
-		goto out;
+	} else {
+		*len = sb.st_size;
 	}
 
-	*len = sb.st_size;
 out:
 	close(fd);
 	return ret;
@@ -492,10 +480,25 @@ out:
 		pos++;                                                                                   \
 	}
 
+static size_t aa_token_len(const char *pos)
+{
+	const char *p = pos;
+
+	while (*p) {
+		if (*p == '/' && *(p + 1) && *(p + 1) == '/' && *(p + 2) && *(p + 2) == '&')
+			break;
+		if (*p == ':' && *(p + 1) && *(p + 1) == '/' && *(p + 2) && *(p + 2) == '/')
+			break;
+		p++;
+	}
+	return p - pos;
+}
+
 static int write_aa_policy(AaNamespace *ns, char *path, int offset, char *rewrite, bool suspend)
 {
 	int i, my_offset, ret;
-	char *rewrite_pos = rewrite, namespace[PATH_MAX];
+	char namespace[PATH_MAX];
+	char *next_rewrite = NULL;
 
 	if (rewrite && suspend) {
 		pr_err("requesting aa rewriting and suspension at the same time is not supported\n");
@@ -503,38 +506,46 @@ static int write_aa_policy(AaNamespace *ns, char *path, int offset, char *rewrit
 	}
 
 	if (!rewrite) {
-		strncpy(namespace, ns->name, sizeof(namespace) - 1);
+		__strlcpy(namespace, ns->name, sizeof(namespace));
 	} else {
-		NEXT_AA_TOKEN(rewrite_pos);
-
-		switch (*rewrite_pos) {
-		case ':': {
-			char tmp, *end;
-
-			end = strchr(rewrite_pos + 1, ':');
-			if (!end) {
-				pr_err("invalid namespace %s\n", rewrite_pos);
+		if (*rewrite == ':') {
+			size_t len = strcspn(rewrite + 1, ":/");
+			if (len == 0 || len >= sizeof(namespace)) {
+				pr_err("invalid or too long namespace in rewrite string %s\n", rewrite);
 				return -1;
 			}
+			memcpy(namespace, rewrite + 1, len);
+			namespace[len] = '\0';
 
-			tmp = *end;
-			*end = 0;
-			__strlcpy(namespace, rewrite_pos + 1, sizeof(namespace));
-			*end = tmp;
-
-			break;
-		}
-		default:
+			next_rewrite = rewrite;
+			NEXT_AA_TOKEN(next_rewrite);
+			if (*next_rewrite == '\0')
+				next_rewrite = NULL;
+		} else {
 			__strlcpy(namespace, ns->name, sizeof(namespace));
+			next_rewrite = rewrite;
+		}
+
+		if (next_rewrite && *next_rewrite != ':') {
+			char token[PATH_MAX];
+			size_t len = aa_token_len(next_rewrite);
+
+			if (len >= sizeof(token)) {
+				pr_err("policy rewrite token too long in %s\n", next_rewrite);
+				return -1;
+			}
+			memcpy(token, next_rewrite, len);
+			token[len] = '\0';
+
 			for (i = 0; i < ns->n_policies; i++) {
-				if (strcmp(ns->policies[i]->name, rewrite_pos))
+				if (strcmp(ns->policies[i]->name, token))
 					pr_warn("binary rewriting of apparmor policies not supported right now, not renaming %s to %s\n",
-						ns->policies[i]->name, rewrite_pos);
+						ns->policies[i]->name, token);
 			}
 		}
 	}
 
-	my_offset = snprintf(path + offset, PATH_MAX - offset, "/namespaces/%s", ns->name);
+	my_offset = snprintf(path + offset, PATH_MAX - offset, "/namespaces/%s", namespace);
 	if (my_offset < 0 || my_offset >= PATH_MAX - offset) {
 		pr_err("snprintf'd too many characters\n");
 		return -1;
@@ -546,7 +557,7 @@ static int write_aa_policy(AaNamespace *ns, char *path, int offset, char *rewrit
 	}
 
 	for (i = 0; i < ns->n_namespaces; i++) {
-		if (write_aa_policy(ns, path, offset + my_offset, rewrite_pos, suspend) < 0)
+		if (write_aa_policy(ns->namespaces[i], path, offset + my_offset, next_rewrite, suspend) < 0)
 			goto fail;
 	}
 
@@ -569,8 +580,8 @@ static int write_aa_policy(AaNamespace *ns, char *path, int offset, char *rewrit
 		}
 
 		if (suspend) {
-			pr_info("suspending policy %s\n", p->name);
-			data = get_suspend_policy(p->name, &len);
+			pr_info("suspending policy %s (namespace %s)\n", p->name, namespace);
+			data = get_suspend_policy(p->name, namespace, false, &len);
 			if (!data) {
 				close(fd);
 				goto fail;
@@ -578,16 +589,55 @@ static int write_aa_policy(AaNamespace *ns, char *path, int offset, char *rewrit
 		}
 
 		n = write(fd, data, len);
+		if (n < 0 && suspend) {
+			int err1 = errno;
+			pr_err("attempt 1 (without -n, written to %s) failed: n=%d len=%ld errno=%d (%s)\n",
+			       path, n, (long)len, err1, strerror(err1));
+			close(fd);
+			if (munmap(data, len) < 0)
+				pr_perror("failed to munmap attempt 1");
+
+			pr_err("retrying suspend policy compilation with -n %s via root interface\n", namespace);
+			fd = open(AA_SECURITYFS_PATH "/policy/.replace", O_WRONLY);
+			if (fd < 0) {
+				pr_perror("couldn't open root replace interface " AA_SECURITYFS_PATH "/policy/.replace");
+				goto fail;
+			}
+			data = get_suspend_policy(p->name, namespace, true, &len);
+			if (!data) {
+				close(fd);
+				goto fail;
+			}
+			n = write(fd, data, len);
+			if (n < 0) {
+				int err2 = errno;
+				pr_err("attempt 2 (with -n %s, written to root interface) failed: n=%d len=%ld errno=%d (%s)\n",
+				       namespace, n, (long)len, err2, strerror(err2));
+			}
+		}
+
 		close(fd);
 		if (suspend && munmap(data, len) < 0) {
 			pr_perror("failed to munmap");
 			goto fail;
 		}
 
-		if (n != len) {
+		/*
+		 * When writing a replacement profile to an AppArmor .replace
+		 * interface, the kernel returns the total size of the profile load
+		 * data structure updated in the kernel, rather than the byte length
+		 * of the written input payload. Therefore, only check for negative
+		 * return values indicating write failures.
+		 */
+		if (n < 0) {
 			pr_perror("write AA policy %s in %s failed", p->name, namespace);
 			goto fail;
 		}
+		if (n < len) {
+			pr_err("short write while loading AA policy %s in %s (n=%u len=%u)",
+			       p->name, namespace, n, (int)len);
+			goto fail;
+		}
 
 		if (!suspend)
 			pr_info("wrote aa policy %s: %s %d\n", path, p->name, n);

test/zdtm/static/apparmor_stacking.c

@@ -45,7 +45,7 @@ static int setprofile(char *to)
 static int checkprofile(pid_t pid, char *expected)
 {
 	FILE *f;
-	char path[PATH_MAX], profile[1024];
+	char path[PATH_MAX], profile[1024], stacked_expected[1024];
 	int len;
 
 	sprintf(path, "/proc/%d/attr/current", pid);
@@ -63,8 +63,21 @@ static int checkprofile(pid_t pid, char *expected)
 		return -1;
 	}
 
-	if (strcmp(profile, expected) != 0) {
-		fail("bad profile .%s. expected .%s.", profile, expected);
+	/*
+	 * On modern kernels (AppArmor 4.x), when restoring tasks confined by
+	 * profiles inside an AppArmor namespace from the host root unconfined
+	 * namespace, AppArmor retains the host root unconfined profile stacked
+	 * with the namespaced target profile when queried from the root namespace
+	 * (e.g., "unconfined//&:ns:profile").
+	 */
+	len = snprintf(stacked_expected, sizeof(stacked_expected), "unconfined//&%s", expected);
+	if (len < 0 || len >= sizeof(stacked_expected)) {
+		fail("bad sprintf");
+		return -1;
+	}
+
+	if (strcmp(profile, expected) != 0 && strcmp(profile, stacked_expected) != 0) {
+		fail("bad profile .%s. expected .%s. or .%s.", profile, expected, stacked_expected);
 		return -1;
 	}