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

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

[Ian Bridges]

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 <icb@fastmail.org>
---
 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);
 
-	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);
 
 	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;
 }
 
-- 
2.47.3

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

[David Laight]

On Mon, 22 Jun 2026 10:00:07 -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.
> 
> 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 <icb@fastmail.org>
> ---
>  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.

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

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

[Casey Schaufler]

On 6/22/2026 9:53 AM, David Laight wrote:
> On Mon, 22 Jun 2026 10:00:07 -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.
>>
>> 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 <icb@fastmail.org>
>> ---
>>  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.
>
>>  
>> -	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'.
> Why does 'security' code so often look like c**p.

Sturgeon's Law.

Also, it's pretty rare that developers outside the security community
look at the security code. It usually only happens when there's a global
change, like this one. And to be clear, reviews like this *are* appreciated.

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

[Ian Bridges]

On Mon, Jun 22, 2026 at 05:53:01PM +0100, David Laight wrote:
> On Mon, 22 Jun 2026 10:00:07 -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.
> > 
> > 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 <icb@fastmail.org>
> > ---
> >  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;
> >  }
> >  
>