On Wed, 1 Jul 2026 00:54:11 -0500 Ian Bridges <[email protected]> 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 <[email protected]> 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; > > > } > >
