Suspected off-by-one in context_struct_to_string()

Suspected off-by-one in context_struct_to_string()

[Willy Tarreau]

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

Re: Suspected off-by-one in context_struct_to_string()

[Paul Moore]

On Thu, Jan 15, 2026 at 3:19 PM Willy Tarreau <w@1wt.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.

It's funny, I had to rescue this email from my spam folder just now
too.  Unfortunately I have to step away for the evening right now so I
can't look at this, but if you don't hear from Stephen or anyone else
by tomorrow I'll take a closer look then.  I'm intentionally trimming
the rest of the email to potentially work around the spammy bits, but
who knows.

If you happen to be seeing this for the first time, Willy's original
message was captured on lore and can be found at the link below:

https://lore.kernel.org/selinux/aWlLs1o5gk7k5osk@1wt.eu

-- 
paul-moore.com

Re: Suspected off-by-one in context_struct_to_string()

[Christian Göttsche]

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

Re: Suspected off-by-one in context_struct_to_string()

[Willy Tarreau]

On Fri, Jan 16, 2026 at 09:16:10AM +0100, Christian Göttsche wrote:
> On Thu, 15 Jan 2026 at 21:20, Willy Tarreau <w@1wt.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.

Sorry if I'm not clear, but my point is that above each strlen()+1
seems to serve as the length of the text + its colon delimiter, so
it covers useful chars and excludes the trailing zero, which is fine.

> >         *scontext_len += mls_compute_context_len(p, context);

Here it does exactly the same.

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

So now we're allocating an area of the number of useful chars, not
counting the trailing zero.

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

Yet we're emitting it.

At least that's how I read it.

Willy

Re: Suspected off-by-one in context_struct_to_string()

[Stephen Smalley]

On Fri, Jan 16, 2026 at 3:16 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Thu, 15 Jan 2026 at 21:20, Willy Tarreau <w@1wt.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.

That is correct. The MLS portion is conditional on whether MLS is
enabled; hence, the +1 for the type length computation includes the
terminating NUL,
and then the mls_compute_context_len() starts at 1 for the leading ":"
if MLS is enabled. The code could admittedly be clearer but I don't
believe this is a bug.

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

Re: Suspected off-by-one in context_struct_to_string()

[Willy Tarreau]

Hi Stephen,

On Fri, Jan 16, 2026 at 10:12:15AM -0500, Stephen Smalley wrote:
> On Fri, Jan 16, 2026 at 3:16 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > On Thu, 15 Jan 2026 at 21:20, Willy Tarreau <w@1wt.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.
> 
> That is correct. The MLS portion is conditional on whether MLS is
> enabled; hence, the +1 for the type length computation includes the
> terminating NUL,
> and then the mls_compute_context_len() starts at 1 for the leading ":"
> if MLS is enabled. The code could admittedly be clearer but I don't
> believe this is a bug.

OK and I, too, think that mls_compute_context_len() stands by what
its name implies. But then *who* is responsible in all this chain
for allocating the room for the trailing zero that is being appended
at the end ?

Or is this the last +1 of this block maybe ?

     *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; // \0 ?

I'm asking because nothing is really clear, and if it happens to work as
intended, it's not super clear why.

thanks,
Willy

Re: Suspected off-by-one in context_struct_to_string()

[Stephen Smalley]

On Fri, Jan 16, 2026 at 10:30 AM Willy Tarreau <w@1wt.eu> wrote:
>
> Hi Stephen,
>
> On Fri, Jan 16, 2026 at 10:12:15AM -0500, Stephen Smalley wrote:
> > On Fri, Jan 16, 2026 at 3:16 AM Christian Göttsche
> > <cgzones@googlemail.com> wrote:
> > >
> > > On Thu, 15 Jan 2026 at 21:20, Willy Tarreau <w@1wt.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.
> >
> > That is correct. The MLS portion is conditional on whether MLS is
> > enabled; hence, the +1 for the type length computation includes the
> > terminating NUL,
> > and then the mls_compute_context_len() starts at 1 for the leading ":"
> > if MLS is enabled. The code could admittedly be clearer but I don't
> > believe this is a bug.
>
> OK and I, too, think that mls_compute_context_len() stands by what
> its name implies. But then *who* is responsible in all this chain
> for allocating the room for the trailing zero that is being appended
> at the end ?
>
> Or is this the last +1 of this block maybe ?
>
>      *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; // \0 ?
>
> I'm asking because nothing is really clear, and if it happens to work as
> intended, it's not super clear why.

Yes, it is that last +1. Historically, originally the MLS support was
a Kconfig option and the entire
mls_*() parts were compiled out altogether if MLS was disabled. In
that situation, the context
ended with the type name and thus counting the NUL aka '\0' byte there
made sense. Later the MLS
support was changed to be dynamically determined at policy load time,
but that function still returns 0
if MLS is disabled so the NUL byte is still counted in the type name
length computation above.
Happy to split it out into its own line and move after the mls_*()
funciton if it would be easier to read.

Re: Suspected off-by-one in context_struct_to_string()

[Willy Tarreau]

On Fri, Jan 16, 2026 at 11:58:30AM -0500, Stephen Smalley wrote:
> On Fri, Jan 16, 2026 at 10:30 AM Willy Tarreau <w@1wt.eu> wrote:
> > But then *who* is responsible in all this chain
> > for allocating the room for the trailing zero that is being appended
> > at the end ?
> >
> > Or is this the last +1 of this block maybe ?
> >
> >      *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; // \0 ?
> >
> > I'm asking because nothing is really clear, and if it happens to work as
> > intended, it's not super clear why.
> 
> Yes, it is that last +1.

OK so that was the trickiest to spot (comments are mine above).

> Historically, originally the MLS support was
> a Kconfig option and the entire
> mls_*() parts were compiled out altogether if MLS was disabled. In
> that situation, the context
> ended with the type name and thus counting the NUL aka '\0' byte there
> made sense. Later the MLS
> support was changed to be dynamically determined at policy load time,
> but that function still returns 0
> if MLS is disabled so the NUL byte is still counted in the type name
> length computation above.

I'm fine with this.

> Happy to split it out into its own line and move after the mls_*()
> funciton if it would be easier to read.

I think that what's mostly missing is a comment before the construction
explaining the expected output format, and another comment saying that
the +1 cover the following delimiter for the first fields, and the
trailing zero for the last one, and that mls_*() include the needeed
delimiter. That way the calculation would look like it obviously
matches expectations.

Thanks for taking the time to explain, now we're certain there's no
issue and next time we get this report again we won't doubt anymore.

Willy