On Mon, Jun 22, 2026 at 05:53:01PM +0100, David Laight wrote:
> On Mon, 22 Jun 2026 10:00:07 -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.
> >
> > The seven open-coded WARN_ON(rc >= buf_len) truncation checks become a
> > single seq_buf_has_overflowed() check after the string is built. The
> > kzalloc() and its exact-size computation are unchanged, so the
> > measurement string passed to IMA is unchanged.
> >
> > Link: https://github.com/KSPP/linux/issues/370 [1]
> > Signed-off-by: Ian Bridges <[email protected]>
> > ---
> > security/selinux/ima.c | 35 ++++++++++++++---------------------
> > 1 file changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > index aa34da9b0aeb..3d81093d16aa 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"
> >
> > @@ -21,8 +22,9 @@
> > 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, len, i;
> >
> > buf_len = strlen("initialized=0;enforcing=0;checkreqprot=0;") + 1;
> >
> > @@ -34,33 +36,24 @@ static char *selinux_ima_collect_state(void)
> > if (!buf)
> > return NULL;
> >
> > - rc = strscpy(buf, "initialized", buf_len);
> > - WARN_ON(rc < 0);
> > + seq_buf_init(&s, buf, buf_len);
>
> That is silly, you need the length of the buffer not the length of a string
> that is the expected length of the output.
>
Is buf_len not the correct value to use here? buf_len is passed as the size
argument to the earlier kzalloc() call (not shown in the patch diff) that
allocates buf.
> >
> > - rc = strlcat(buf, selinux_initialized() ? on : off, buf_len);
> > - WARN_ON(rc >= buf_len);
> > + seq_buf_puts(&s, "initialized");
> > + seq_buf_puts(&s, selinux_initialized() ? on : off);
> >
> > - rc = strlcat(buf, "enforcing", buf_len);
> > - WARN_ON(rc >= buf_len);
> > + seq_buf_puts(&s, "enforcing");
> > + seq_buf_puts(&s, enforcing_enabled() ? on : off);
> >
> > - 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);
> > + seq_buf_puts(&s, "checkreqprot");
> > + seq_buf_puts(&s, checkreqprot_get() ? on : off);
>
> That lot would be easier to read as a seq_printf() - with %d and
> kill 'on' and 'off'.
This is a good suggestion, I'll use this approach in the v2.
> Why does 'security' code so often look like c**p.
>
> David
>
> >
> > for (i = 0; i < __POLICYDB_CAP_MAX; i++) {
> > - rc = strlcat(buf, selinux_policycap_names[i], buf_len);
> > - WARN_ON(rc >= buf_len);
> > -
> > - rc = strlcat(buf, selinux_state.policycap[i] ? on : off,
> > - buf_len);
> > - WARN_ON(rc >= buf_len);
> > + seq_buf_puts(&s, selinux_policycap_names[i]);
> > + seq_buf_puts(&s, selinux_state.policycap[i] ? on : off);
> > }
> >
> > + WARN_ON(seq_buf_has_overflowed(&s));
> > +
> > return buf;
> > }
> >
>
