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

[Ian Bridges <icb@fastmail.org>]

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:
> 
> > In preparation for removing the deprecated strlcat() API[1], replace the
> > strscpy()/strlcat() chain in selinux_ima_collect_state() with a struct
> > seq_buf, which tracks the write position and remaining space internally.
> > 
> > Each field is written with seq_buf_printf() using a "=%d;" format, which
> > removes the open-coded "=1;"/"=0;" constants. The seven per-append
> > WARN_ON(rc >= buf_len) truncation checks are replaced by a single
> > seq_buf_has_overflowed() check after the string is built.
> > 
> > Link: https://github.com/KSPP/linux/issues/370 [1]
> > Signed-off-by: Ian Bridges <icb@fastmail.org>
> > ---
> > Changed in v2: replace the v1 seq_buf_puts() pairs with seq_buf_printf()
> > using a "=%d;" format, which drops the open-coded "=1;"/"=0;" constants.
> 
> Did you verify that all the values are 0/1 otherwise a !! prefix might
> be needed.

Yes. selinux_initialized(), enforcing_enabled() and checkreqprot_get()
all return bool, and selinux_state.policycap[] is a bool array.
checkreqprot_get() always returns 0. The others read bool fields that
are written only by normal assignment (WRITE_ONCE() or
smp_store_release()), which normalizes the stored value to 0 or 1. So
"%d" prints "0" or "1", the same as the old ? "=1;" : "=0;" mapping, and
no !! is needed.

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

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