Message-ID: <CAEjxPJ4ct1tWUs14C7Hdj=xZBK08qJ4XMfqZ_SAAq2=icMXm5w@mail.gmail.com>
Date: Fri, 16 Jan 2026 10:12:15 -0500
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Christian Göttsche <cgzones@...glemail.com>
Cc: Willy Tarreau <w@....eu>, Paul Moore <paul@...l-moore.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 Fri, Jan 16, 2026 at 3:16 AM Christian Göttsche
<cgzones@...glemail.com> wrote:
>
> 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.

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

Powered by blists - more mailing lists