* Re: [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
[not found] <CAEjxPJ6-ZbOKxtpbpD4NixZeQy gU6Z3T8C8jLRvCPDHC-mL3w@mail.gmail.com>
@ 2025-07-24 13:40 ` Rahul Sandhu
0 siblings, 0 replies; 4+ messages in thread
From: Rahul Sandhu @ 2025-07-24 13:40 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: jwcart2, nvraxn, omosnace, paul, plautrba, selinux
> 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...
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] libselinux: fix parsing of the enforcing kernel cmdline parameter
@ 2025-07-24 12:28 Stephen Smalley
2025-07-24 13:05 ` [PATCH v3] " Rahul Sandhu
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Smalley @ 2025-07-24 12:28 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: omosnace, paul, selinux
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 12:28 [PATCH v2] " Stephen Smalley
@ 2025-07-24 13:05 ` Rahul Sandhu
2025-07-24 13:27 ` Stephen Smalley
0 siblings, 1 reply; 4+ messages in thread
From: Rahul Sandhu @ 2025-07-24 13:05 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: nvraxn, omosnace, paul, selinux
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 13:05 ` [PATCH v3] " Rahul Sandhu
@ 2025-07-24 13:27 ` Stephen Smalley
2025-07-24 13:30 ` Stephen Smalley
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Smalley @ 2025-07-24 13:27 UTC (permalink / raw)
To: Rahul Sandhu, Petr Lautrbach, James Carter; +Cc: omosnace, paul, selinux
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 13:27 ` Stephen Smalley
@ 2025-07-24 13:30 ` Stephen Smalley
0 siblings, 0 replies; 4+ messages in thread
From: Stephen Smalley @ 2025-07-24 13:30 UTC (permalink / raw)
To: Rahul Sandhu, Petr Lautrbach, James Carter; +Cc: omosnace, paul, selinux
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
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-07-24 13:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAEjxPJ6-ZbOKxtpbpD4NixZeQy gU6Z3T8C8jLRvCPDHC-mL3w@mail.gmail.com>
2025-07-24 13:40 ` [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter Rahul Sandhu
2025-07-24 12:28 [PATCH v2] " Stephen Smalley
2025-07-24 13:05 ` [PATCH v3] " Rahul Sandhu
2025-07-24 13:27 ` Stephen Smalley
2025-07-24 13:30 ` Stephen Smalley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.