Message-ID: <aWlLs1o5gk7k5osk@1wt.eu>
Date: Thu, 15 Jan 2026 21:18:59 +0100
From: Willy Tarreau <w@....eu>
To: Paul Moore <paul@...l-moore.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>
Cc: security@...nel.org, selinux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Suspected off-by-one in context_struct_to_string()

Hello,

we've received a suspected vulnerability report on the kernel security
list, that was clearly generated by AI and really not clear at all on
the root causes nor impacts. We first dismissed it and it kept coming
back a few times. I'm not pasting it because it's more confusing than
interesting, though I can pass it to the maintainers if desired. I'm
also purposely *not* CCing the reporter, as the address changed a few
times, and once you respond you receive a new copy of the same report.
Clearly this bot deserves a bit more tuning.

The report claimed that the call to mls_compute_context_len() didn't
properly reflect the size needed by mls_sid_to_context() due to an
off-by-one that would result in the trailing zero being written too far.
Initially we thought that was wrong since there are +1 everywhere in
all lengths calculation in the function. But revisiting it today made
us realize that this indeed seems to be true: the +1 that are everywhere
are in fact due to the surrounding delimiters, and the first one that
appeared to be the one accounting for the trailing zero was in fact
for the starting colon.

In context_struct_to_string(), we have this:

	*scontext_len += strlen(sym_name(p, SYM_USERS, context->user - 1)) + 1;
	*scontext_len += strlen(sym_name(p, SYM_ROLES, context->role - 1)) + 1;
	*scontext_len += strlen(sym_name(p, SYM_TYPES, context->type - 1)) + 1;
	*scontext_len += mls_compute_context_len(p, context);

*scontext_len is initialized to zero, is increased by the length of each
appended string + delimiter, and used as-is in kmalloc() a few lines later:

	scontextp = kmalloc(*scontext_len, GFP_ATOMIC);

then filled by sprintf() then mls_sid_to_context():

        scontextp += sprintf(scontextp, "%s:%s:%s",
                sym_name(p, SYM_USERS, context->user - 1),
                sym_name(p, SYM_ROLES, context->role - 1),
                sym_name(p, SYM_TYPES, context->type - 1));

        mls_sid_to_context(p, context, &scontextp);

And finally the trailing zero is appended:

        *scontextp = 0;

Thus unless I'm missing something, that trailing zero is indeed written
past the end of the allocated area. The impact looks fairly limited
though given that root is required to reach that code.

Given the semantics of *scontext_len that claims to contain the string
length, my feeling is that we should add one to the kmalloc() call:

-	scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
+	scontextp = kmalloc(*scontext_len + 1, GFP_ATOMIC);

I must confess we got confused a bit when trying to follow this code,
because the called functions do not indicate the expected output format
nor whether or not the trailing zero is counted, so it's easy to think
that a +1 stands for the trailing zero instead of an unclear delimiter.
Also, it looks like the sole purpose of mls_compute_context_len() is
to compute the length that will be needed to store the result of
mls_sid_to_context(), and results in an almost copy-paste of one into
the other, making it harder to check if they match (we had to read
them due to the report pointing at that first one for being wrong, which
is not the case depending on what we consider as a string length). I
think that instead a change consisting in calling mls_sid_to_context()
with a NULL destination buffer to avoid emitting bytes, and making it
return the length could make the whole design more robust by doing a
first call to compute the length and a second one to perform the copy.

Let us know if you need more info, if all of this is wrong, if you want
a copy of the original report or even the reporter's address if you want
to attempt to communicate with them (we don't even know if there's a
human or only a bot there).

Thanks,
Willy

Powered by blists - more mailing lists