On Sat, Jun 27, 2026 at 10:49:14PM +0100, David Laight wrote: > On Fri, 26 Jun 2026 10:57:10 -0500 > Ian Bridges <[email protected]> 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 <[email protected]> > > --- > > 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; > > } >
