[PATCH] ima: bound line scan in ima_read_policy() to fix OOB read

Fabrice Derepas <[email protected]>
Newsgroups org.kernel.vger.linux-integrity,org.kernel.vger.linux-kernel,org.kernel.vger.linux-security-module
Message-ID <[email protected]>
ima_read_policy() loads a policy file with kernel_read_file_from_path()
and splits it into lines with

	while (size > 0 && (p = strsep(&datap, "\n")))

kernel_read_file() allocates the destination with vmalloc(i_size) --
exactly i_size bytes, and writes no NUL terminator. strsep()'s scan for
the next '\n' is not bounded by @size, so when the last line has no
trailing newline the scan runs off the end of the buffer (CWE-125). When
i_size is a multiple of PAGE_SIZE the allocation has no slack and the
read walks into the vmalloc guard page and faults.

Reproduced under KASAN in a VM: writing the path of a page-aligned
policy file with no trailing newline to <securityfs>/ima/policy oopses:

  BUG: unable to handle page fault for address: ffffc90000032000
  #PF: supervisor read access in kernel mode
  RIP: 0010:strsep+0x7a/0xd0
  Call Trace:
   ima_write_policy+0x1f4/0x260
   vfs_write+0x16a/0x6f0
   ksys_write+0xcb/0x160
   do_syscall_64+0xe0/0x5a0

This requires CAP_MAC_ADMIN (the policy file is mode 0200), but a policy
file that does not end in a newline is an ordinary, non-malicious
condition, so a legitimate policy load can crash the kernel.

Walk the buffer with memchr() bounded by the remaining size instead of
strsep(): terminate each line in place at its newline, and parse a
NUL-terminated copy of a final line that has none. The explicit per-line
accounting replaces the old "size -= rc" step, whose off-by-one
(ima_parse_add_rule() returns strlen() + 1) made a trailing line without a
newline fail with -EINVAL; such a policy now loads. The loop now consumes
the buffer exactly, so the trailing "if (size) return -EINVAL" is dropped.

Fixes: 7429b092811f ("ima: load policy using path")
Assisted-by: copilot-cli:claude-opus-4-6 frama-c
Signed-off-by: Fabrice Derepas <[email protected]>
---
Tested under KASAN (CONFIG_KASAN_GENERIC + CONFIG_KASAN_VMALLOC) in QEMU,
loading a policy via "echo /path > <securityfs>/ima/policy":

  - page-aligned file, no trailing newline: unpatched -> guard-page oops
    in strsep()/ima_read_policy() (trace above); patched -> no fault, the
    load fails cleanly with -EINVAL on the (garbage) content.
  - valid policy with a trailing newline: loads before and after.
  - valid rule with no trailing newline: unpatched -> -EINVAL (the size
    underflow); patched -> loads.

lib/string.o is not KASAN-instrumented, so the over-read is caught by the
vmalloc guard page rather than a shadow report; the confirmation is the
page-fault oops with strsep()/ima_write_policy() in the trace.

 security/integrity/ima/ima_fs.c | 46 ++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 174a94740..7b530b130 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -526,12 +526,10 @@ static const struct file_operations ima_ascii_measurements_staged_ops = {
 static ssize_t ima_read_policy(char *path)
 {
 	void *data = NULL;
-	char *datap;
-	size_t size;
+	char *datap, *eol, *p;
+	size_t size, linelen;
 	int rc, pathlen = strlen(path);
 
-	char *p;
-
 	/* remove \n */
 	datap = path;
 	strsep(&datap, "\n");
@@ -546,21 +544,49 @@ static ssize_t ima_read_policy(char *path)
 	rc = 0;
 
 	datap = data;
-	while (size > 0 && (p = strsep(&datap, "\n"))) {
+	while (size > 0) {
+		eol = memchr(datap, '\n', size);
+		linelen = eol ? (size_t)(eol - datap) : size;
+
+		if (eol) {
+			/* NUL-terminate the line in place, within bounds. */
+			*eol = '\0';
+			p = datap;
+		} else {
+			/*
+			 * kernel_read_file_from_path() does not NUL-terminate
+			 * the buffer, and it may be exactly i_size bytes long,
+			 * so a string walk off the end is possible.  The final
+			 * line without a trailing newline has no room for a
+			 * terminator; parse a terminated copy instead.
+			 */
+			p = kmemdup_nul(datap, linelen, GFP_KERNEL);
+			if (!p) {
+				rc = -ENOMEM;
+				break;
+			}
+		}
+
 		pr_debug("rule: %s\n", p);
 		rc = ima_parse_add_rule(p);
+		if (!eol)
+			kfree(p);
 		if (rc < 0)
 			break;
-		size -= rc;
+		rc = 0;
+
+		datap += linelen;
+		size -= linelen;
+		if (eol) {
+			datap++;	/* skip the newline */
+			size--;
+		}
 	}
 
 	vfree(data);
 	if (rc < 0)
 		return rc;
-	else if (size)
-		return -EINVAL;
-	else
-		return pathlen;
+	return pathlen;
 }
 
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,

base-commit: d58772d8520c7ef247c4b95c9bd76d3a25da9ff5
-- 
2.53.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.