Re: [PATCH v2] selinux: replace strlcat() with seq_buf in selinux_ima_collect_state()

[David Laight <david.laight.linux@gmail.com>]

On Wed, 1 Jul 2026 00:54:11 -0500
Ian Bridges <icb@fastmail.org> wrote:

> On Sat, Jun 27, 2026 at 10:49:14PM +0100, David Laight wrote:
> > On Fri, 26 Jun 2026 10:57:10 -0500
> > Ian Bridges <icb@fastmail.org> wrote:
...
> > > 
> > > v1: https://lore.kernel.org/all/ajlN94VO7BYNUTAy@dev/
> > > 
> > > I didn't change the precomputation of the string size. An alternative,
> > > which is used by other seq_buf callers (e.g. kernel/rcu/refscale.c,
> > > mm/memcontrol.c), is to drop the precomputation and allocate an oversized
> > > fixed buffer, relying on the seq_buf overflow check as a backstop. I'm
> > > happy to rework the patch to adopt that alternative.  
> > 
> > That would be reasonable, the output is always exactly the same length and
> > the buffer is freed just after being allocated.
> > A comment that there are 3 + 15 strings and all are under 32 bytes so 1k
> > is plenty would suffice.
> > 
> > 	David
> >  
> 
> I didn't originally take this route because a fixed buffer bakes in an
> invariant (the whole string must stay under 1K) with nothing to enforce
> it at compile time. If more capabilities are added later, or a name grows
> longer, the string could overflow. That shows up only at runtime, as the
> seq_buf_has_overflowed() WARN, and the measured selinux-state string is
> silently truncated.

You could use '32 * (3 + NUM_CAPAPILITIES)'.
IIRC the longest is 24 characters (plus 2 for the =[01]) so that will be plenty.

	David


> 
> Thanks for the review.
> 
> Ian
> 
> > > 
> > >  security/selinux/ima.c | 40 +++++++++++++---------------------------
> > >  1 file changed, 13 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > > index aa34da9b0aeb..cb0efa2fc1ad 100644
> > > --- a/security/selinux/ima.c
> > > +++ b/security/selinux/ima.c
> > > @@ -9,6 +9,7 @@
> > >   */
> > >  #include <linux/vmalloc.h>
> > >  #include <linux/ima.h>
> > > +#include <linux/seq_buf.h>
> > >  #include "security.h"
> > >  #include "ima.h"
> > >  
> > > @@ -20,46 +21,31 @@
> > >   */
> > >  static char *selinux_ima_collect_state(void)
> > >  {
> > > -	const char *on = "=1;", *off = "=0;";
> > > +	struct seq_buf s;
> > >  	char *buf;
> > > -	int buf_len, len, i, rc;
> > > +	int buf_len, suffix_len, i;
> > >  
> > >  	buf_len = strlen("initialized=0;enforcing=0;checkreqprot=0;") + 1;
> > > +	suffix_len = strlen("=0;");
> > >  
> > > -	len = strlen(on);
> > >  	for (i = 0; i < __POLICYDB_CAP_MAX; i++)
> > > -		buf_len += strlen(selinux_policycap_names[i]) + len;
> > > +		buf_len += strlen(selinux_policycap_names[i]) + suffix_len;
> > >  
> > >  	buf = kzalloc(buf_len, GFP_KERNEL);
> > >  	if (!buf)
> > >  		return NULL;
> > >  
> > > -	rc = strscpy(buf, "initialized", buf_len);
> > > -	WARN_ON(rc < 0);
> > > +	seq_buf_init(&s, buf, buf_len);
> > >  
> > > -	rc = strlcat(buf, selinux_initialized() ? on : off, buf_len);
> > > -	WARN_ON(rc >= buf_len);
> > > +	seq_buf_printf(&s, "initialized=%d;enforcing=%d;checkreqprot=%d;",
> > > +		       selinux_initialized(), enforcing_enabled(),
> > > +		       checkreqprot_get());
> > >  
> > > -	rc = strlcat(buf, "enforcing", buf_len);
> > > -	WARN_ON(rc >= buf_len);
> > > -
> > > -	rc = strlcat(buf, enforcing_enabled() ? on : off, buf_len);
> > > -	WARN_ON(rc >= buf_len);
> > > -
> > > -	rc = strlcat(buf, "checkreqprot", buf_len);
> > > -	WARN_ON(rc >= buf_len);
> > > -
> > > -	rc = strlcat(buf, checkreqprot_get() ? on : off, buf_len);
> > > -	WARN_ON(rc >= buf_len);
> > > -
> > > -	for (i = 0; i < __POLICYDB_CAP_MAX; i++) {
> > > -		rc = strlcat(buf, selinux_policycap_names[i], buf_len);
> > > -		WARN_ON(rc >= buf_len);
> > > +	for (i = 0; i < __POLICYDB_CAP_MAX; i++)
> > > +		seq_buf_printf(&s, "%s=%d;", selinux_policycap_names[i],
> > > +			       selinux_state.policycap[i]);
> > >  
> > > -		rc = strlcat(buf, selinux_state.policycap[i] ? on : off,
> > > -			buf_len);
> > > -		WARN_ON(rc >= buf_len);
> > > -	}
> > > +	WARN_ON(seq_buf_has_overflowed(&s));
> > >  
> > >  	return buf;
> > >  }  
> >