On Wed, 2026-08-26 at 11:51 +0200, Roberto Sassu wrote:
> On Fri, 2026-08-14 at 10:54 +0200, Fabrice Derepas wrote:
> > 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.
>
> Looks unnecessarily complicated. I would replicate instead the same
> behavior of ima_write_policy() to allocate a buffer with an additional
> byte for the terminator.
>
> Read the inode size, vmalloc() size + 1, set the terminator, and pass
> the buffer to kernel_read_file_from_path().
>
> I would still pass NULL for file_size to save ourselves from rechecking
> if it changed after the kernel_read_file_from_path() call.
Could be simpler...
Something like this? (not tested)
diff --git a/security/integrity/ima/ima_fs.c
b/security/integrity/ima/ima_fs.c
index 3b8174a91a34..6151f440cd2f 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -528,7 +528,7 @@ static const struct file_operations
ima_ascii_measurements_staged_ops = {
static ssize_t ima_read_policy(char *path)
{
void *data = NULL;
- char *datap;
+ char *datap, *buf = NULL;
size_t size;
int rc, pathlen = strlen(path);
@@ -548,6 +548,17 @@ static ssize_t ima_read_policy(char *path)
rc = 0;
datap = data;
+ if (size && datap[size - 1] != '\n') {
+ buf = kmemdup_nul(data, size, GFP_KERNEL);
+ if (!buf) {
+ vfree(data);
+ return -ENOMEM;
+ }
+ datap = buf;
+ /* Count the NUL terminator. */
+ size++;
+ }
+
while (size > 0 && (p = strsep(&datap, "\n"))) {
pr_debug("rule: %s\n", p);
rc = ima_parse_add_rule(p);
@@ -557,6 +568,7 @@ static ssize_t ima_read_policy(char *path)
}
vfree(data);
+ kfree(buf);
if (rc < 0)
return rc;
else if (size)
Roberto
>
> Thanks
>
> Roberto
>
> > 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
>
