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

[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...