[PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Currently, parsing of the cmdline has two issues:
- By using atoi, no error checking is done. What happens if an argument
  that isn't an integer is provided, e.g. enforcing=foo? And as there
  is also no validation that the number provided is actually valid, 1
  or 0, what happens if enforcing=2?

- After the first strstr, no arguments that follow are searched for; if
  I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
  into account. This is made even worse due to halting searching after
  finding the first "enforcing=" token, meaning that if the cmdline was
  as follows:

  fooenforcing=0 enforcing=0

  the enforcing parameter is entirely ignored.

This patch fixes this by:

  - Using strtol to actually validate that we got passed a number, and
    then validating that that number is either 0 or 1. If instead we
    get passed an invalid value, we skip over the argument entirely.

  - Looping until the last "enforcing=" in the cmdline. Latter (valid)
    arguments take precedence over previous arguments.

Although this patch (intentionally) breaks the case where "enforcing="
is provided with a positive argument that isn't 1, enforcing=2 doesn't
really make much sense, and being strict with the arguments we parse is
a good thing given that SELinux's mode of operation is controlled by
that option.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libselinux/src/load_policy.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index dc1e4b6e..9d411b95 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -244,17 +244,26 @@ int selinux_init_load_policy(int *enforce)
 	rc = mount("proc", "/proc", "proc", 0, 0);
 	cfg = fopen("/proc/cmdline", "re");
 	if (cfg) {
-		char *tmp;
 		buf = malloc(selinux_page_size);
 		if (!buf) {
 			fclose(cfg);
 			return -1;
 		}
-		if (fgets(buf, selinux_page_size, cfg) &&
-		    (tmp = strstr(buf, "enforcing="))) {
-			if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
-				secmdline =
-				    atoi(tmp + sizeof("enforcing=") - 1);
+		if (fgets(buf, selinux_page_size, cfg)) {
+			char *search = buf;
+			char *tmp;
+			while ((tmp = strstr(search, "enforcing="))) {
+				if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
+					char *valstr = tmp + sizeof("enforcing=") - 1;
+					char *endptr;
+					errno = 0;
+					long val = strtol(valstr, &endptr, 10);
+					if (endptr != valstr && errno == 0 && (val == 0 || val == 1)) {
+						secmdline = (int)val;
+					}
+				}
+				/* advance past the current substring, latter arguments take precedence */
+				search = tmp + 1;
 			}
 		}
 		fclose(cfg);
-- 
2.50.1

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[robinshao007]

Hi Rahul，

I got what you want to do.
But I'm wondering whether the file "libselinux/src/load_policy.c" belongs to the SELinux kernel code - perhaps it's not part of the SELinux kernel components. 
It is a part of libselinux module.

robinshao007@163.com


///////////////////////////////////////////////////////////

 



From: Rahul Sandhu



Date: 2025-07-20 20:52



To: selinux



CC: Rahul Sandhu



Subject: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter



Currently, parsing of the cmdline has two issues:



- By using atoi, no error checking is done. What happens if an argument



  that isn't an integer is provided, e.g. enforcing=foo? And as there



  is also no validation that the number provided is actually valid, 1



  or 0, what happens if enforcing=2?



 



- After the first strstr, no arguments that follow are searched for; if



  I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken



  into account. This is made even worse due to halting searching after



  finding the first "enforcing=" token, meaning that if the cmdline was



  as follows:



 



  fooenforcing=0 enforcing=0



 



  the enforcing parameter is entirely ignored.



 



This patch fixes this by:



 



  - Using strtol to actually validate that we got passed a number, and



    then validating that that number is either 0 or 1. If instead we



    get passed an invalid value, we skip over the argument entirely.



 



  - Looping until the last "enforcing=" in the cmdline. Latter (valid)



    arguments take precedence over previous arguments.



 



Although this patch (intentionally) breaks the case where "enforcing="



is provided with a positive argument that isn't 1, enforcing=2 doesn't



really make much sense, and being strict with the arguments we parse is



a good thing given that SELinux's mode of operation is controlled by



that option.



 



Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>



---



libselinux/src/load_policy.c | 21 +++++++++++++++------



1 file changed, 15 insertions(+), 6 deletions(-)



 



diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c



index dc1e4b6e..9d411b95 100644



--- a/libselinux/src/load_policy.c



+++ b/libselinux/src/load_policy.c



@@ -244,17 +244,26 @@ int selinux_init_load_policy(int *enforce)



rc = mount("proc", "/proc", "proc", 0, 0);



cfg = fopen("/proc/cmdline", "re");



if (cfg) {



- char *tmp;



buf = malloc(selinux_page_size);



if (!buf) {



fclose(cfg);



return -1;



}



- if (fgets(buf, selinux_page_size, cfg) &&



-     (tmp = strstr(buf, "enforcing="))) {



- if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {



- secmdline =



-     atoi(tmp + sizeof("enforcing=") - 1);



+ if (fgets(buf, selinux_page_size, cfg)) {



+ char *search = buf;



+ char *tmp;



+ while ((tmp = strstr(search, "enforcing="))) {



+ if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {



+ char *valstr = tmp + sizeof("enforcing=") - 1;



+ char *endptr;



+ errno = 0;



+ long val = strtol(valstr, &endptr, 10);



+ if (endptr != valstr && errno == 0 && (val == 0 || val == 1)) {



+ secmdline = (int)val;



+ }



+ }



+ /* advance past the current substring, latter arguments take precedence */



+ search = tmp + 1;



}



}



fclose(cfg);



--



2.50.1



 

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Hi Robin,

Yes, this patch is for libselinux (so part of SELinux userspace), and
not the kernel side of things. As per the contributing document in the
userspace repository[1], patches for SELinux userspace are also to be
sent to the ml.

[1] https://github.com/SELinuxProject/selinux/blob/main/CONTRIBUTING.md#contributing-code

Regards,
Rahul

Re: Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[robinshao007]

Hi Rahul，


You are right. Thanks for your information.


Regards,
Robin





 



From: Rahul Sandhu



Date: 2025-07-21 17:47



To: robinshao007@163.com



CC: nvraxn@gmail.com; selinux@vger.kernel.org



Subject: Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter



Hi Robin,



 



Yes, this patch is for libselinux (so part of SELinux userspace), and



not the kernel side of things. As per the contributing document in the



userspace repository[1], patches for SELinux userspace are also to be



sent to the ml.



 



[1] https://github.com/SELinuxProject/selinux/blob/main/CONTRIBUTING.md#contributing-code



 



Regards,



Rahul

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Sun, Jul 20, 2025 at 8:52 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Currently, parsing of the cmdline has two issues:
> - By using atoi, no error checking is done. What happens if an argument
>   that isn't an integer is provided, e.g. enforcing=foo? And as there
>   is also no validation that the number provided is actually valid, 1
>   or 0, what happens if enforcing=2?
>
> - After the first strstr, no arguments that follow are searched for; if
>   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
>   into account. This is made even worse due to halting searching after
>   finding the first "enforcing=" token, meaning that if the cmdline was
>   as follows:
>
>   fooenforcing=0 enforcing=0
>
>   the enforcing parameter is entirely ignored.
>
> This patch fixes this by:
>
>   - Using strtol to actually validate that we got passed a number, and
>     then validating that that number is either 0 or 1. If instead we
>     get passed an invalid value, we skip over the argument entirely.
>
>   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
>     arguments take precedence over previous arguments.
>
> Although this patch (intentionally) breaks the case where "enforcing="
> is provided with a positive argument that isn't 1, enforcing=2 doesn't
> really make much sense, and being strict with the arguments we parse is
> a good thing given that SELinux's mode of operation is controlled by
> that option.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>

We should make it match the kernel's logic for parsing and handling
enforcing= on the cmdline. For reference, the kernel does this:

static int __init enforcing_setup(char *str)
{
        unsigned long enforcing;
        if (!kstrtoul(str, 0, &enforcing))
                selinux_enforcing_boot = enforcing ? 1 : 0;
        return 1;
}
__setup("enforcing=", enforcing_setup);

And the kernel's parser ignores anything after a "--", passing
anything after that to the init process.

I am not sure off the top of my head what the kernel does if the same
parameter is passed multiple times.

> ---
>  libselinux/src/load_policy.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index dc1e4b6e..9d411b95 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -244,17 +244,26 @@ int selinux_init_load_policy(int *enforce)
>         rc = mount("proc", "/proc", "proc", 0, 0);
>         cfg = fopen("/proc/cmdline", "re");
>         if (cfg) {
> -               char *tmp;
>                 buf = malloc(selinux_page_size);
>                 if (!buf) {
>                         fclose(cfg);
>                         return -1;
>                 }
> -               if (fgets(buf, selinux_page_size, cfg) &&
> -                   (tmp = strstr(buf, "enforcing="))) {
> -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> -                               secmdline =
> -                                   atoi(tmp + sizeof("enforcing=") - 1);
> +               if (fgets(buf, selinux_page_size, cfg)) {
> +                       char *search = buf;
> +                       char *tmp;
> +                       while ((tmp = strstr(search, "enforcing="))) {
> +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> +                                       char *endptr;
> +                                       errno = 0;
> +                                       long val = strtol(valstr, &endptr, 10);
> +                                       if (endptr != valstr && errno == 0 && (val == 0 || val == 1)) {
> +                                               secmdline = (int)val;
> +                                       }
> +                               }
> +                               /* advance past the current substring, latter arguments take precedence */
> +                               search = tmp + 1;
>                         }
>                 }
>                 fclose(cfg);
> --
> 2.50.1
>
>

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Mon, Jul 21, 2025 at 8:56 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Sun, Jul 20, 2025 at 8:52 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Currently, parsing of the cmdline has two issues:
> > - By using atoi, no error checking is done. What happens if an argument
> >   that isn't an integer is provided, e.g. enforcing=foo? And as there
> >   is also no validation that the number provided is actually valid, 1
> >   or 0, what happens if enforcing=2?
> >
> > - After the first strstr, no arguments that follow are searched for; if
> >   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
> >   into account. This is made even worse due to halting searching after
> >   finding the first "enforcing=" token, meaning that if the cmdline was
> >   as follows:
> >
> >   fooenforcing=0 enforcing=0
> >
> >   the enforcing parameter is entirely ignored.
> >
> > This patch fixes this by:
> >
> >   - Using strtol to actually validate that we got passed a number, and
> >     then validating that that number is either 0 or 1. If instead we
> >     get passed an invalid value, we skip over the argument entirely.
> >
> >   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
> >     arguments take precedence over previous arguments.
> >
> > Although this patch (intentionally) breaks the case where "enforcing="
> > is provided with a positive argument that isn't 1, enforcing=2 doesn't
> > really make much sense, and being strict with the arguments we parse is
> > a good thing given that SELinux's mode of operation is controlled by
> > that option.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>
> We should make it match the kernel's logic for parsing and handling
> enforcing= on the cmdline. For reference, the kernel does this:
>
> static int __init enforcing_setup(char *str)
> {
>         unsigned long enforcing;
>         if (!kstrtoul(str, 0, &enforcing))
>                 selinux_enforcing_boot = enforcing ? 1 : 0;
>         return 1;
> }
> __setup("enforcing=", enforcing_setup);
>
> And the kernel's parser ignores anything after a "--", passing
> anything after that to the init process.
>
> I am not sure off the top of my head what the kernel does if the same
> parameter is passed multiple times.

Ok, looks like the kernel allows the same parameter multiple times and
the last one wins, so the behavior you implemented is correct in that
regard but not the way you handle enforcing=2.


>
> > ---
> >  libselinux/src/load_policy.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> > index dc1e4b6e..9d411b95 100644
> > --- a/libselinux/src/load_policy.c
> > +++ b/libselinux/src/load_policy.c
> > @@ -244,17 +244,26 @@ int selinux_init_load_policy(int *enforce)
> >         rc = mount("proc", "/proc", "proc", 0, 0);
> >         cfg = fopen("/proc/cmdline", "re");
> >         if (cfg) {
> > -               char *tmp;
> >                 buf = malloc(selinux_page_size);
> >                 if (!buf) {
> >                         fclose(cfg);
> >                         return -1;
> >                 }
> > -               if (fgets(buf, selinux_page_size, cfg) &&
> > -                   (tmp = strstr(buf, "enforcing="))) {
> > -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> > -                               secmdline =
> > -                                   atoi(tmp + sizeof("enforcing=") - 1);
> > +               if (fgets(buf, selinux_page_size, cfg)) {
> > +                       char *search = buf;
> > +                       char *tmp;
> > +                       while ((tmp = strstr(search, "enforcing="))) {
> > +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> > +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> > +                                       char *endptr;
> > +                                       errno = 0;
> > +                                       long val = strtol(valstr, &endptr, 10);
> > +                                       if (endptr != valstr && errno == 0 && (val == 0 || val == 1)) {
> > +                                               secmdline = (int)val;
> > +                                       }
> > +                               }
> > +                               /* advance past the current substring, latter arguments take precedence */
> > +                               search = tmp + 1;
> >                         }
> >                 }
> >                 fclose(cfg);
> > --
> > 2.50.1
> >
> >

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Hi Stephen,

> We should make it match the kernel's logic for parsing and handling
> enforcing= on the cmdline. For reference, the kernel does this:
>
> static int __init enforcing_setup(char *str)
> {
>        unsigned long enforcing;
>        if (!kstrtoul(str, 0, &enforcing))
>                selinux_enforcing_boot = enforcing ? 1 : 0;
>        return 1;
> }
> __setup("enforcing=", enforcing_setup);

Okay, seems reasonable, I'll send a v2 to follow that logic shortly.

> And the kernel's parser ignores anything after a "--", passing
> anything after that to the init process.

Just to clarify, unless I'm missing anything I don't see any need for
us to worry about that as:

1. Based on the logic above it would seem 'enforcing=' is recognised by
   the kernel?
2. We're reading /proc/cmdline anyway, so I don't see a reason for that
   to be a concern - we're going to see all arguments as far as I can
   tell.

Although, I'm a bit confused about CONFIG_SECURITY_SELINUX_DEVELOP, how
are we handling that in libselinux? I don't think that stops userspace
from loading in permissive mode, and even with:

#define selinux_enforcing_boot 1

I don't see how that would stop libselinux from loading in permissive.

Regards,
Rahul

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Tue, Jul 22, 2025 at 1:42 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Hi Stephen,
>
> > We should make it match the kernel's logic for parsing and handling
> > enforcing= on the cmdline. For reference, the kernel does this:
> >
> > static int __init enforcing_setup(char *str)
> > {
> >        unsigned long enforcing;
> >        if (!kstrtoul(str, 0, &enforcing))
> >                selinux_enforcing_boot = enforcing ? 1 : 0;
> >        return 1;
> > }
> > __setup("enforcing=", enforcing_setup);
>
> Okay, seems reasonable, I'll send a v2 to follow that logic shortly.
>
> > And the kernel's parser ignores anything after a "--", passing
> > anything after that to the init process.
>
> Just to clarify, unless I'm missing anything I don't see any need for
> us to worry about that as:
>
> 1. Based on the logic above it would seem 'enforcing=' is recognised by
>    the kernel?
> 2. We're reading /proc/cmdline anyway, so I don't see a reason for that
>    to be a concern - we're going to see all arguments as far as I can
>    tell.

Yes, I believe you are correct.

> Although, I'm a bit confused about CONFIG_SECURITY_SELINUX_DEVELOP, how
> are we handling that in libselinux? I don't think that stops userspace
> from loading in permissive mode, and even with:
>
> #define selinux_enforcing_boot 1
>
> I don't see how that would stop libselinux from loading in permissive.

I'm not aware of any Linux or Android distribution that disables
CONFIG_SECURITY_SELINUX_DEVELOP itself, although Samsung disables
permissive mode another way. That said, I agree that libselinux should
handle it correctly. If I am reading it correctly, if one boots with
enforcing=0 on the kernel cmdline and
CONFIG_SECURITY_SELINUX_DEVELOP=n, then selinux_init_load_policy()
will set *enforce to 0, get the current enforcing status from the
kernel via security_getenforce(), see that it differs from *enforce,
call security_setenforce(*enforce), which will fail, and then fall
through to loading the policy. So the system will come up enforcing
with a policy loaded, which is correct. The only seeming bug here is
that *enforce will be left as 0, which could confuse the caller, but
it appears to be only used to determine how to handle errors from the
policy load, so a failed policy load might not halt the system. So
that should be fixed too to set *enforce to the value returned by
security_getenforce() in that situation.


>
> Regards,
> Rahul

Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Tue, Jul 22, 2025 at 9:05 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Jul 22, 2025 at 1:42 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Hi Stephen,
> >
> > > We should make it match the kernel's logic for parsing and handling
> > > enforcing= on the cmdline. For reference, the kernel does this:
> > >
> > > static int __init enforcing_setup(char *str)
> > > {
> > >        unsigned long enforcing;
> > >        if (!kstrtoul(str, 0, &enforcing))
> > >                selinux_enforcing_boot = enforcing ? 1 : 0;
> > >        return 1;
> > > }
> > > __setup("enforcing=", enforcing_setup);
> >
> > Okay, seems reasonable, I'll send a v2 to follow that logic shortly.
> >
> > > And the kernel's parser ignores anything after a "--", passing
> > > anything after that to the init process.
> >
> > Just to clarify, unless I'm missing anything I don't see any need for
> > us to worry about that as:
> >
> > 1. Based on the logic above it would seem 'enforcing=' is recognised by
> >    the kernel?
> > 2. We're reading /proc/cmdline anyway, so I don't see a reason for that
> >    to be a concern - we're going to see all arguments as far as I can
> >    tell.
>
> Yes, I believe you are correct.
>
> > Although, I'm a bit confused about CONFIG_SECURITY_SELINUX_DEVELOP, how
> > are we handling that in libselinux? I don't think that stops userspace
> > from loading in permissive mode, and even with:
> >
> > #define selinux_enforcing_boot 1
> >
> > I don't see how that would stop libselinux from loading in permissive.
>
> I'm not aware of any Linux or Android distribution that disables
> CONFIG_SECURITY_SELINUX_DEVELOP itself, although Samsung disables
> permissive mode another way. That said, I agree that libselinux should
> handle it correctly. If I am reading it correctly, if one boots with
> enforcing=0 on the kernel cmdline and
> CONFIG_SECURITY_SELINUX_DEVELOP=n, then selinux_init_load_policy()
> will set *enforce to 0, get the current enforcing status from the
> kernel via security_getenforce(), see that it differs from *enforce,
> call security_setenforce(*enforce), which will fail, and then fall
> through to loading the policy. So the system will come up enforcing
> with a policy loaded, which is correct. The only seeming bug here is
> that *enforce will be left as 0, which could confuse the caller, but
> it appears to be only used to determine how to handle errors from the
> policy load, so a failed policy load might not halt the system. So
> that should be fixed too to set *enforce to the value returned by
> security_getenforce() in that situation.

Android doesn't use selinux_init_load_policy() and directly handles
checking and setting enforcing mode itself so this is not a concern
for Android, just Linux distributions.

>
>
> >
> > Regards,
> > Rahul

[PATCH v2] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Currently, parsing of the cmdline has two issues:
- By using atoi, no error checking is done. What happens if an argument
  that isn't an integer is provided, e.g. enforcing=foo? And as there
  is also no validation that the number provided is actually valid, 1
  or 0, what happens if enforcing=2?

- After the first strstr, no arguments that follow are searched for; if
  I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
  into account. This is made even worse due to halting searching after
  finding the first "enforcing=" token, meaning that if the cmdline was
  as follows:

  fooenforcing=0 enforcing=0

  the enforcing parameter is entirely ignored.

This patch fixes this by:

  - Using strtol to actually validate that we got passed a number, and
    then validating that that number is either 0 or 1. If instead we
    get passed an invalid value, we skip over the argument entirely.

  - Looping until the last "enforcing=" in the cmdline. Latter (valid)
    arguments take precedence over previous arguments.

In the case where "enforcing=" is passed an invalid argument (i.e. not
0 or 1), default to enforcing mode (so enforcing=1) as the kernel also
does.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libselinux/src/load_policy.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

v2: Follow the same argument parsing behaviour as the kernel does.

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index dc1e4b6e..0d2a16d2 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
 	rc = mount("proc", "/proc", "proc", 0, 0);
 	cfg = fopen("/proc/cmdline", "re");
 	if (cfg) {
-		char *tmp;
 		buf = malloc(selinux_page_size);
 		if (!buf) {
 			fclose(cfg);
 			return -1;
 		}
-		if (fgets(buf, selinux_page_size, cfg) &&
-		    (tmp = strstr(buf, "enforcing="))) {
-			if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
-				secmdline =
-				    atoi(tmp + sizeof("enforcing=") - 1);
+		if (fgets(buf, selinux_page_size, cfg)) {
+			char *search = buf;
+			char *tmp;
+			while ((tmp = strstr(search, "enforcing="))) {
+				if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
+					char *valstr = tmp + sizeof("enforcing=") - 1;
+					char *endptr;
+					errno = 0;
+					const long val = strtol(valstr, &endptr, 0);
+					if (endptr != valstr && errno == 0) {
+						secmdline = val ? 1 : 0;
+					} else {
+						secmdline = 1;
+					}
+				}
+				/* advance past the current substring, latter arguments take precedence */
+				search = tmp + 1;
 			}
 		}
 		fclose(cfg);
-- 
2.50.1

Re: [PATCH v2] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Thu, Jul 24, 2025 at 5:13 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Currently, parsing of the cmdline has two issues:
> - By using atoi, no error checking is done. What happens if an argument
>   that isn't an integer is provided, e.g. enforcing=foo? And as there
>   is also no validation that the number provided is actually valid, 1
>   or 0, what happens if enforcing=2?
>
> - After the first strstr, no arguments that follow are searched for; if
>   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
>   into account. This is made even worse due to halting searching after
>   finding the first "enforcing=" token, meaning that if the cmdline was
>   as follows:
>
>   fooenforcing=0 enforcing=0
>
>   the enforcing parameter is entirely ignored.
>
> This patch fixes this by:
>
>   - Using strtol to actually validate that we got passed a number, and
>     then validating that that number is either 0 or 1. If instead we
>     get passed an invalid value, we skip over the argument entirely.
>
>   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
>     arguments take precedence over previous arguments.
>
> In the case where "enforcing=" is passed an invalid argument (i.e. not
> 0 or 1), default to enforcing mode (so enforcing=1) as the kernel also
> does.

Sorry if I was unclear, but the kernel leaves selinux_enforcing_boot
initialized as zero if kstrtoul() returns an error. So it accepts
enforcing=2 as equivalent to enforcing=1 but it does not accept
enforcing=on or similar.

>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
>  libselinux/src/load_policy.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> v2: Follow the same argument parsing behaviour as the kernel does.
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index dc1e4b6e..0d2a16d2 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
>         rc = mount("proc", "/proc", "proc", 0, 0);
>         cfg = fopen("/proc/cmdline", "re");
>         if (cfg) {
> -               char *tmp;
>                 buf = malloc(selinux_page_size);
>                 if (!buf) {
>                         fclose(cfg);
>                         return -1;
>                 }
> -               if (fgets(buf, selinux_page_size, cfg) &&
> -                   (tmp = strstr(buf, "enforcing="))) {
> -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> -                               secmdline =
> -                                   atoi(tmp + sizeof("enforcing=") - 1);
> +               if (fgets(buf, selinux_page_size, cfg)) {
> +                       char *search = buf;
> +                       char *tmp;
> +                       while ((tmp = strstr(search, "enforcing="))) {
> +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> +                                       char *endptr;
> +                                       errno = 0;
> +                                       const long val = strtol(valstr, &endptr, 0);
> +                                       if (endptr != valstr && errno == 0) {
> +                                               secmdline = val ? 1 : 0;
> +                                       } else {
> +                                               secmdline = 1;

Unless I misunderstand, the kernel would default to 0 in this case.

> +                                       }
> +                               }
> +                               /* advance past the current substring, latter arguments take precedence */
> +                               search = tmp + 1;
>                         }
>                 }
>                 fclose(cfg);
> --
> 2.50.1
>

Re: [PATCH v2] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

> Sorry if I was unclear, but the kernel leaves selinux_enforcing_boot
> initialized as zero if kstrtoul() returns an error. So it accepts
> enforcing=2 as equivalent to enforcing=1 but it does not accept
> enforcing=on or similar.

Oops that's on me, I misread the manual page on kstrtoul before my
coffee this morning, apologies! I'll send a v3 shortly.

Regards,
Rahul

[PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Currently, parsing of the cmdline has two issues:
- By using atoi, no error checking is done. What happens if an argument
  that isn't an integer is provided, e.g. enforcing=foo? And as there
  is also no validation that the number provided is actually valid, 1
  or 0, what happens if enforcing=2?

- After the first strstr, no arguments that follow are searched for; if
  I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
  into account. This is made even worse due to halting searching after
  finding the first "enforcing=" token, meaning that if the cmdline was
  as follows:

  fooenforcing=0 enforcing=0

  the enforcing parameter is entirely ignored.

This patch fixes this by:

  - Using strtol to actually validate that we got passed a number, and
    then validating that that number is either 0 or 1. If instead we
    get passed an invalid value, we skip over the argument entirely.

  - Looping until the last "enforcing=" in the cmdline. Latter (valid)
    arguments take precedence over previous arguments.

For the case where "enforcing=" is provided with a valid integer, 0 is
treated as permissive mode, and anything else (such as 1 or 2, etc) is
treated as enforcing mode. When "enforcing=" is passed an argument that
is not a valid integer (such as "on"), default to enforcing=0, i.e.
permissive mode. This is in line with how the kernel parses the
enforcing parameter.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libselinux/src/load_policy.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

v2: Follow the same argument parsing behaviour as the kernel does.
v3: Actually follow the kernel's behaviour where "enforcing=" is not
    provided with a valid integer...

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index dc1e4b6e..ec2d5614 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
 	rc = mount("proc", "/proc", "proc", 0, 0);
 	cfg = fopen("/proc/cmdline", "re");
 	if (cfg) {
-		char *tmp;
 		buf = malloc(selinux_page_size);
 		if (!buf) {
 			fclose(cfg);
 			return -1;
 		}
-		if (fgets(buf, selinux_page_size, cfg) &&
-		    (tmp = strstr(buf, "enforcing="))) {
-			if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
-				secmdline =
-				    atoi(tmp + sizeof("enforcing=") - 1);
+		if (fgets(buf, selinux_page_size, cfg)) {
+			char *search = buf;
+			char *tmp;
+			while ((tmp = strstr(search, "enforcing="))) {
+				if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
+					char *valstr = tmp + sizeof("enforcing=") - 1;
+					char *endptr;
+					errno = 0;
+					const long val = strtol(valstr, &endptr, 0);
+					if (endptr != valstr && errno == 0) {
+						secmdline = val ? 1 : 0;
+					} else {
+						secmdline = 0;
+					}
+				}
+				/* advance past the current substring, latter arguments take precedence */
+				search = tmp + 1;
 			}
 		}
 		fclose(cfg);
-- 
2.50.1

Re: [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Thu, Jul 24, 2025 at 9:05 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Currently, parsing of the cmdline has two issues:
> - By using atoi, no error checking is done. What happens if an argument
>   that isn't an integer is provided, e.g. enforcing=foo? And as there
>   is also no validation that the number provided is actually valid, 1
>   or 0, what happens if enforcing=2?
>
> - After the first strstr, no arguments that follow are searched for; if
>   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
>   into account. This is made even worse due to halting searching after
>   finding the first "enforcing=" token, meaning that if the cmdline was
>   as follows:
>
>   fooenforcing=0 enforcing=0
>
>   the enforcing parameter is entirely ignored.
>
> This patch fixes this by:
>
>   - Using strtol to actually validate that we got passed a number, and
>     then validating that that number is either 0 or 1. If instead we
>     get passed an invalid value, we skip over the argument entirely.

This is not quite correct but not a big deal to me since you clarify below.

>
>   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
>     arguments take precedence over previous arguments.
>
> For the case where "enforcing=" is provided with a valid integer, 0 is
> treated as permissive mode, and anything else (such as 1 or 2, etc) is
> treated as enforcing mode. When "enforcing=" is passed an argument that
> is not a valid integer (such as "on"), default to enforcing=0, i.e.
> permissive mode. This is in line with how the kernel parses the
> enforcing parameter.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>

This looks good to me wrt consistency with the kernel but will defer
to one of the other selinux userspace maintainers to merge.

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
>  libselinux/src/load_policy.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> v2: Follow the same argument parsing behaviour as the kernel does.
> v3: Actually follow the kernel's behaviour where "enforcing=" is not
>     provided with a valid integer...
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index dc1e4b6e..ec2d5614 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
>         rc = mount("proc", "/proc", "proc", 0, 0);
>         cfg = fopen("/proc/cmdline", "re");
>         if (cfg) {
> -               char *tmp;
>                 buf = malloc(selinux_page_size);
>                 if (!buf) {
>                         fclose(cfg);
>                         return -1;
>                 }
> -               if (fgets(buf, selinux_page_size, cfg) &&
> -                   (tmp = strstr(buf, "enforcing="))) {
> -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> -                               secmdline =
> -                                   atoi(tmp + sizeof("enforcing=") - 1);
> +               if (fgets(buf, selinux_page_size, cfg)) {
> +                       char *search = buf;
> +                       char *tmp;
> +                       while ((tmp = strstr(search, "enforcing="))) {
> +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> +                                       char *endptr;
> +                                       errno = 0;
> +                                       const long val = strtol(valstr, &endptr, 0);
> +                                       if (endptr != valstr && errno == 0) {
> +                                               secmdline = val ? 1 : 0;
> +                                       } else {
> +                                               secmdline = 0;
> +                                       }
> +                               }
> +                               /* advance past the current substring, latter arguments take precedence */
> +                               search = tmp + 1;
>                         }
>                 }
>                 fclose(cfg);
> --
> 2.50.1
>

Re: [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Thu, Jul 24, 2025 at 9:27 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Jul 24, 2025 at 9:05 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Currently, parsing of the cmdline has two issues:
> > - By using atoi, no error checking is done. What happens if an argument
> >   that isn't an integer is provided, e.g. enforcing=foo? And as there
> >   is also no validation that the number provided is actually valid, 1
> >   or 0, what happens if enforcing=2?
> >
> > - After the first strstr, no arguments that follow are searched for; if
> >   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
> >   into account. This is made even worse due to halting searching after
> >   finding the first "enforcing=" token, meaning that if the cmdline was
> >   as follows:
> >
> >   fooenforcing=0 enforcing=0
> >
> >   the enforcing parameter is entirely ignored.
> >
> > This patch fixes this by:
> >
> >   - Using strtol to actually validate that we got passed a number, and
> >     then validating that that number is either 0 or 1. If instead we
> >     get passed an invalid value, we skip over the argument entirely.
>
> This is not quite correct but not a big deal to me since you clarify below.
>
> >
> >   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
> >     arguments take precedence over previous arguments.
> >
> > For the case where "enforcing=" is provided with a valid integer, 0 is
> > treated as permissive mode, and anything else (such as 1 or 2, etc) is
> > treated as enforcing mode. When "enforcing=" is passed an argument that
> > is not a valid integer (such as "on"), default to enforcing=0, i.e.
> > permissive mode. This is in line with how the kernel parses the
> > enforcing parameter.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>
> This looks good to me wrt consistency with the kernel but will defer
> to one of the other selinux userspace maintainers to merge.
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Also, not sure if this matters but b4 warns about the following:

Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
    + Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> (✓
DKIM/gmail.com)
  ---
  ✓ Signed: DKIM/gmail.com
---
Total patches: 1
---
NOTE: some trailers ignored due to from/email mismatches:
    ! Trailer: Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
     Msg From: robinshao007@163.com <robinshao007@163.com>
NOTE: Rerun with -S to apply them anyway

>
> > ---
> >  libselinux/src/load_policy.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > v2: Follow the same argument parsing behaviour as the kernel does.
> > v3: Actually follow the kernel's behaviour where "enforcing=" is not
> >     provided with a valid integer...
> >
> > diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> > index dc1e4b6e..ec2d5614 100644
> > --- a/libselinux/src/load_policy.c
> > +++ b/libselinux/src/load_policy.c
> > @@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
> >         rc = mount("proc", "/proc", "proc", 0, 0);
> >         cfg = fopen("/proc/cmdline", "re");
> >         if (cfg) {
> > -               char *tmp;
> >                 buf = malloc(selinux_page_size);
> >                 if (!buf) {
> >                         fclose(cfg);
> >                         return -1;
> >                 }
> > -               if (fgets(buf, selinux_page_size, cfg) &&
> > -                   (tmp = strstr(buf, "enforcing="))) {
> > -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> > -                               secmdline =
> > -                                   atoi(tmp + sizeof("enforcing=") - 1);
> > +               if (fgets(buf, selinux_page_size, cfg)) {
> > +                       char *search = buf;
> > +                       char *tmp;
> > +                       while ((tmp = strstr(search, "enforcing="))) {
> > +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> > +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> > +                                       char *endptr;
> > +                                       errno = 0;
> > +                                       const long val = strtol(valstr, &endptr, 0);
> > +                                       if (endptr != valstr && errno == 0) {
> > +                                               secmdline = val ? 1 : 0;
> > +                                       } else {
> > +                                               secmdline = 0;
> > +                                       }
> > +                               }
> > +                               /* advance past the current substring, latter arguments take precedence */
> > +                               search = tmp + 1;
> >                         }
> >                 }
> >                 fclose(cfg);
> > --
> > 2.50.1
> >

Re: [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

> Also, not sure if this matters but b4 warns about the following:
> 
> Checking attestation on all messages, may take a moment...
> ---
>   ✓ [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
>     + Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> (✓
> DKIM/gmail.com)
>   ---
>   ✓ Signed: DKIM/gmail.com
> ---
> Total patches: 1
> ---
> NOTE: some trailers ignored due to from/email mismatches:
>     ! Trailer: Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>      Msg From: robinshao007@163.com <robinshao007@163.com>
> NOTE: Rerun with -S to apply them anyway

Huh, not sure what's up with that, they did reply to the thread earlier
and it did mangle some of the spacing, might be that? Regardless, I'll
update the commit message too, forgot about the earlier paragraph...

[PATCH v4] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Currently, parsing of the cmdline has two issues:
- By using atoi, no error checking is done. What happens if an argument
  that isn't an integer is provided, e.g. enforcing=foo? And as there
  is also no validation that the number provided is actually valid, 1
  or 0, what happens if enforcing=2?

- After the first strstr, no arguments that follow are searched for; if
  I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
  into account. This is made even worse due to halting searching after
  finding the first "enforcing=" token, meaning that if the cmdline was
  as follows:

  fooenforcing=0 enforcing=0

  the enforcing parameter is entirely ignored.

This patch fixes this by:

  - Using strtol to actually validate that we got passed a number, and
    if not default to 0 (i.e. permissive mode) as per kernel behaviour.
    If given a positive integer that isn't 1, also treat that as 1 (so
    enforcing mode).

  - Looping until the last "enforcing=" in the cmdline. Latter (valid)
    arguments take precedence over previous arguments.

For the case where "enforcing=" is provided with a valid integer, 0 is
treated as permissive mode, and anything else (such as 1 or 2, etc) is
treated as enforcing mode. When "enforcing=" is passed an argument that
is not a valid integer (such as "on"), default to enforcing=0, i.e.
permissive mode. This is in line with how the kernel parses the
enforcing parameter.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libselinux/src/load_policy.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

v2: Follow the same argument parsing behaviour as the kernel does.
v3: Actually follow the kernel's behaviour where "enforcing=" is not
    provided with a valid integer...
v3: Update the commit message to also reflect the behaviour above.

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index dc1e4b6e..ec2d5614 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
 	rc = mount("proc", "/proc", "proc", 0, 0);
 	cfg = fopen("/proc/cmdline", "re");
 	if (cfg) {
-		char *tmp;
 		buf = malloc(selinux_page_size);
 		if (!buf) {
 			fclose(cfg);
 			return -1;
 		}
-		if (fgets(buf, selinux_page_size, cfg) &&
-		    (tmp = strstr(buf, "enforcing="))) {
-			if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
-				secmdline =
-				    atoi(tmp + sizeof("enforcing=") - 1);
+		if (fgets(buf, selinux_page_size, cfg)) {
+			char *search = buf;
+			char *tmp;
+			while ((tmp = strstr(search, "enforcing="))) {
+				if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
+					char *valstr = tmp + sizeof("enforcing=") - 1;
+					char *endptr;
+					errno = 0;
+					const long val = strtol(valstr, &endptr, 0);
+					if (endptr != valstr && errno == 0) {
+						secmdline = val ? 1 : 0;
+					} else {
+						secmdline = 0;
+					}
+				}
+				/* advance past the current substring, latter arguments take precedence */
+				search = tmp + 1;
 			}
 		}
 		fclose(cfg);
-- 
2.50.1

Re: [PATCH v4] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Thu, Jul 24, 2025 at 9:51 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Currently, parsing of the cmdline has two issues:
> - By using atoi, no error checking is done. What happens if an argument
>   that isn't an integer is provided, e.g. enforcing=foo? And as there
>   is also no validation that the number provided is actually valid, 1
>   or 0, what happens if enforcing=2?
>
> - After the first strstr, no arguments that follow are searched for; if
>   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
>   into account. This is made even worse due to halting searching after
>   finding the first "enforcing=" token, meaning that if the cmdline was
>   as follows:
>
>   fooenforcing=0 enforcing=0
>
>   the enforcing parameter is entirely ignored.
>
> This patch fixes this by:
>
>   - Using strtol to actually validate that we got passed a number, and
>     if not default to 0 (i.e. permissive mode) as per kernel behaviour.
>     If given a positive integer that isn't 1, also treat that as 1 (so
>     enforcing mode).
>
>   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
>     arguments take precedence over previous arguments.
>
> For the case where "enforcing=" is provided with a valid integer, 0 is
> treated as permissive mode, and anything else (such as 1 or 2, etc) is
> treated as enforcing mode. When "enforcing=" is passed an argument that
> is not a valid integer (such as "on"), default to enforcing=0, i.e.
> permissive mode. This is in line with how the kernel parses the
> enforcing parameter.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
>  libselinux/src/load_policy.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> v2: Follow the same argument parsing behaviour as the kernel does.
> v3: Actually follow the kernel's behaviour where "enforcing=" is not
>     provided with a valid integer...
> v3: Update the commit message to also reflect the behaviour above.
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index dc1e4b6e..ec2d5614 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
>         rc = mount("proc", "/proc", "proc", 0, 0);
>         cfg = fopen("/proc/cmdline", "re");
>         if (cfg) {
> -               char *tmp;
>                 buf = malloc(selinux_page_size);
>                 if (!buf) {
>                         fclose(cfg);
>                         return -1;
>                 }
> -               if (fgets(buf, selinux_page_size, cfg) &&
> -                   (tmp = strstr(buf, "enforcing="))) {
> -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> -                               secmdline =
> -                                   atoi(tmp + sizeof("enforcing=") - 1);
> +               if (fgets(buf, selinux_page_size, cfg)) {
> +                       char *search = buf;
> +                       char *tmp;
> +                       while ((tmp = strstr(search, "enforcing="))) {
> +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> +                                       char *endptr;
> +                                       errno = 0;
> +                                       const long val = strtol(valstr, &endptr, 0);
> +                                       if (endptr != valstr && errno == 0) {
> +                                               secmdline = val ? 1 : 0;
> +                                       } else {
> +                                               secmdline = 0;
> +                                       }
> +                               }
> +                               /* advance past the current substring, latter arguments take precedence */
> +                               search = tmp + 1;

Apologies for not asking this earlier, but is there a reason you only
increment tmp by 1 versus by sizeof("enforcing=")-1?

>                         }
>                 }
>                 fclose(cfg);
> --
> 2.50.1
>

Re: [PATCH v4] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

> Apologies for not asking this earlier, but is there a reason you only
> increment tmp by 1 versus by sizeof("enforcing=")-1?

No specific reason, I guess that's also more efficient so v5 incoming.

[PATCH v5] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Rahul Sandhu]

Currently, parsing of the cmdline has two issues:
- By using atoi, no error checking is done. What happens if an argument
  that isn't an integer is provided, e.g. enforcing=foo? And as there
  is also no validation that the number provided is actually valid, 1
  or 0, what happens if enforcing=2?

- After the first strstr, no arguments that follow are searched for; if
  I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
  into account. This is made even worse due to halting searching after
  finding the first "enforcing=" token, meaning that if the cmdline was
  as follows:

  fooenforcing=0 enforcing=0

  the enforcing parameter is entirely ignored.

This patch fixes this by:

  - Using strtol to actually validate that we got passed a number, and
    if not default to 0 (i.e. permissive mode) as per kernel behaviour.
    If given a positive integer that isn't 1, also treat that as 1 (so
    enforcing mode).

  - Looping until the last "enforcing=" in the cmdline. Latter (valid)
    arguments take precedence over previous arguments.

For the case where "enforcing=" is provided with a valid integer, 0 is
treated as permissive mode, and anything else (such as 1 or 2, etc) is
treated as enforcing mode. When "enforcing=" is passed an argument that
is not a valid integer (such as "on"), default to enforcing=0, i.e.
permissive mode. This is in line with how the kernel parses the
enforcing parameter.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libselinux/src/load_policy.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

v2: Follow the same argument parsing behaviour as the kernel does.
v3: Actually follow the kernel's behaviour where "enforcing=" is not
    provided with a valid integer...
v4: Update the commit message to also reflect the behaviour above.
v5: Advance past sizeof("enforcing=") - 1 instead of just 1

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index dc1e4b6e..f67e5538 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
 	rc = mount("proc", "/proc", "proc", 0, 0);
 	cfg = fopen("/proc/cmdline", "re");
 	if (cfg) {
-		char *tmp;
 		buf = malloc(selinux_page_size);
 		if (!buf) {
 			fclose(cfg);
 			return -1;
 		}
-		if (fgets(buf, selinux_page_size, cfg) &&
-		    (tmp = strstr(buf, "enforcing="))) {
-			if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
-				secmdline =
-				    atoi(tmp + sizeof("enforcing=") - 1);
+		if (fgets(buf, selinux_page_size, cfg)) {
+			char *search = buf;
+			char *tmp;
+			while ((tmp = strstr(search, "enforcing="))) {
+				if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
+					char *valstr = tmp + sizeof("enforcing=") - 1;
+					char *endptr;
+					errno = 0;
+					const long val = strtol(valstr, &endptr, 0);
+					if (endptr != valstr && errno == 0) {
+						secmdline = val ? 1 : 0;
+					} else {
+						secmdline = 0;
+					}
+				}
+				/* advance past the current substring, latter arguments take precedence */
+				search = tmp + sizeof("enforcing=") - 1;
 			}
 		}
 		fclose(cfg);
-- 
2.50.1

Re: [PATCH v5] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Fri, Jul 25, 2025 at 6:16 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Currently, parsing of the cmdline has two issues:
> - By using atoi, no error checking is done. What happens if an argument
>   that isn't an integer is provided, e.g. enforcing=foo? And as there
>   is also no validation that the number provided is actually valid, 1
>   or 0, what happens if enforcing=2?
>
> - After the first strstr, no arguments that follow are searched for; if
>   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
>   into account. This is made even worse due to halting searching after
>   finding the first "enforcing=" token, meaning that if the cmdline was
>   as follows:
>
>   fooenforcing=0 enforcing=0
>
>   the enforcing parameter is entirely ignored.
>
> This patch fixes this by:
>
>   - Using strtol to actually validate that we got passed a number, and
>     if not default to 0 (i.e. permissive mode) as per kernel behaviour.
>     If given a positive integer that isn't 1, also treat that as 1 (so
>     enforcing mode).
>
>   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
>     arguments take precedence over previous arguments.
>
> For the case where "enforcing=" is provided with a valid integer, 0 is
> treated as permissive mode, and anything else (such as 1 or 2, etc) is
> treated as enforcing mode. When "enforcing=" is passed an argument that
> is not a valid integer (such as "on"), default to enforcing=0, i.e.
> permissive mode. This is in line with how the kernel parses the
> enforcing parameter.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>

Reviewed-by: Stephen Smalley <stephen.smalley.work@gmall.com>
Tested-by:  Stephen Smalley <stephen.smalley.work@gmall.com>
> ---
>  libselinux/src/load_policy.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> v2: Follow the same argument parsing behaviour as the kernel does.
> v3: Actually follow the kernel's behaviour where "enforcing=" is not
>     provided with a valid integer...
> v4: Update the commit message to also reflect the behaviour above.
> v5: Advance past sizeof("enforcing=") - 1 instead of just 1
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index dc1e4b6e..f67e5538 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
>         rc = mount("proc", "/proc", "proc", 0, 0);
>         cfg = fopen("/proc/cmdline", "re");
>         if (cfg) {
> -               char *tmp;
>                 buf = malloc(selinux_page_size);
>                 if (!buf) {
>                         fclose(cfg);
>                         return -1;
>                 }
> -               if (fgets(buf, selinux_page_size, cfg) &&
> -                   (tmp = strstr(buf, "enforcing="))) {
> -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> -                               secmdline =
> -                                   atoi(tmp + sizeof("enforcing=") - 1);
> +               if (fgets(buf, selinux_page_size, cfg)) {
> +                       char *search = buf;
> +                       char *tmp;
> +                       while ((tmp = strstr(search, "enforcing="))) {
> +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> +                                       char *endptr;
> +                                       errno = 0;
> +                                       const long val = strtol(valstr, &endptr, 0);
> +                                       if (endptr != valstr && errno == 0) {
> +                                               secmdline = val ? 1 : 0;
> +                                       } else {
> +                                               secmdline = 0;
> +                                       }
> +                               }
> +                               /* advance past the current substring, latter arguments take precedence */
> +                               search = tmp + sizeof("enforcing=") - 1;
>                         }
>                 }
>                 fclose(cfg);
> --
> 2.50.1
>

Re: [PATCH v5] libselinux: fix parsing of the enforcing kernel cmdline parameter

[Stephen Smalley]

On Mon, Jul 28, 2025 at 10:04 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 25, 2025 at 6:16 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Currently, parsing of the cmdline has two issues:
> > - By using atoi, no error checking is done. What happens if an argument
> >   that isn't an integer is provided, e.g. enforcing=foo? And as there
> >   is also no validation that the number provided is actually valid, 1
> >   or 0, what happens if enforcing=2?
> >
> > - After the first strstr, no arguments that follow are searched for; if
> >   I have enforcing=0 enforcing=1, the latter enforcing=1 is not taken
> >   into account. This is made even worse due to halting searching after
> >   finding the first "enforcing=" token, meaning that if the cmdline was
> >   as follows:
> >
> >   fooenforcing=0 enforcing=0
> >
> >   the enforcing parameter is entirely ignored.
> >
> > This patch fixes this by:
> >
> >   - Using strtol to actually validate that we got passed a number, and
> >     if not default to 0 (i.e. permissive mode) as per kernel behaviour.
> >     If given a positive integer that isn't 1, also treat that as 1 (so
> >     enforcing mode).
> >
> >   - Looping until the last "enforcing=" in the cmdline. Latter (valid)
> >     arguments take precedence over previous arguments.
> >
> > For the case where "enforcing=" is provided with a valid integer, 0 is
> > treated as permissive mode, and anything else (such as 1 or 2, etc) is
> > treated as enforcing mode. When "enforcing=" is passed an argument that
> > is not a valid integer (such as "on"), default to enforcing=0, i.e.
> > permissive mode. This is in line with how the kernel parses the
> > enforcing parameter.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>
> Reviewed-by: Stephen Smalley <stephen.smalley.work@gmall.com>
> Tested-by:  Stephen Smalley <stephen.smalley.work@gmall.com>

Thanks, merged.

> > ---
> >  libselinux/src/load_policy.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > v2: Follow the same argument parsing behaviour as the kernel does.
> > v3: Actually follow the kernel's behaviour where "enforcing=" is not
> >     provided with a valid integer...
> > v4: Update the commit message to also reflect the behaviour above.
> > v5: Advance past sizeof("enforcing=") - 1 instead of just 1
> >
> > diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> > index dc1e4b6e..f67e5538 100644
> > --- a/libselinux/src/load_policy.c
> > +++ b/libselinux/src/load_policy.c
> > @@ -244,17 +244,28 @@ int selinux_init_load_policy(int *enforce)
> >         rc = mount("proc", "/proc", "proc", 0, 0);
> >         cfg = fopen("/proc/cmdline", "re");
> >         if (cfg) {
> > -               char *tmp;
> >                 buf = malloc(selinux_page_size);
> >                 if (!buf) {
> >                         fclose(cfg);
> >                         return -1;
> >                 }
> > -               if (fgets(buf, selinux_page_size, cfg) &&
> > -                   (tmp = strstr(buf, "enforcing="))) {
> > -                       if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> > -                               secmdline =
> > -                                   atoi(tmp + sizeof("enforcing=") - 1);
> > +               if (fgets(buf, selinux_page_size, cfg)) {
> > +                       char *search = buf;
> > +                       char *tmp;
> > +                       while ((tmp = strstr(search, "enforcing="))) {
> > +                               if (tmp == buf || isspace((unsigned char)*(tmp - 1))) {
> > +                                       char *valstr = tmp + sizeof("enforcing=") - 1;
> > +                                       char *endptr;
> > +                                       errno = 0;
> > +                                       const long val = strtol(valstr, &endptr, 0);
> > +                                       if (endptr != valstr && errno == 0) {
> > +                                               secmdline = val ? 1 : 0;
> > +                                       } else {
> > +                                               secmdline = 0;
> > +                                       }
> > +                               }
> > +                               /* advance past the current substring, latter arguments take precedence */
> > +                               search = tmp + sizeof("enforcing=") - 1;
> >                         }
> >                 }
> >                 fclose(cfg);
> > --
> > 2.50.1
> >