Message-ID: <CAJ2a_DeFC5Z2VKXoDDkKmhcB8cft_ZtU1UtriPX292q4GRyh-A@mail.gmail.com>
Date: Fri, 16 Jan 2026 09:16:10 +0100
From: Christian Göttsche <cgzones@...glemail.com>
To: Willy Tarreau <w@....eu>
Cc: Paul Moore <paul@...l-moore.com>, Stephen Smalley <stephen.smalley.work@...il.com>, 
	security@...nel.org, selinux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Suspected off-by-one in context_struct_to_string()

On Thu, 15 Jan 2026 at 21:20, Willy Tarreau <w@....eu> wrote:
>
> 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;

I think this +1 from the type name length covers the trailing NUL
byte, since mls_compute_context_len() and mls_sid_to_context() cover
the one byte space for the separating colon between type and optional
MLS component.

>         *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