* [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
@ 2025-07-20 12:52 Rahul Sandhu
2025-07-21 9:01 ` robinshao007
2025-07-21 12:56 ` Stephen Smalley
0 siblings, 2 replies; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-20 12:52 UTC (permalink / raw)
To: selinux; +Cc: 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-20 12:52 [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter Rahul Sandhu
@ 2025-07-21 9:01 ` robinshao007
2025-07-21 9:47 ` Rahul Sandhu
2025-07-21 12:56 ` Stephen Smalley
1 sibling, 1 reply; 21+ messages in thread
From: robinshao007 @ 2025-07-21 9:01 UTC (permalink / raw)
To: selinux, nvraxn
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-20 12:52 [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter Rahul Sandhu
2025-07-21 9:01 ` robinshao007
@ 2025-07-21 12:56 ` Stephen Smalley
2025-07-21 14:18 ` Stephen Smalley
2025-07-24 9:13 ` [PATCH v2] " Rahul Sandhu
1 sibling, 2 replies; 21+ messages in thread
From: Stephen Smalley @ 2025-07-21 12:56 UTC (permalink / raw)
To: Rahul Sandhu, Paul Moore, Ondrej Mosnacek; +Cc: selinux
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
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-21 12:56 ` Stephen Smalley
@ 2025-07-21 14:18 ` Stephen Smalley
2025-07-22 5:42 ` Rahul Sandhu
2025-07-24 9:13 ` [PATCH v2] " Rahul Sandhu
1 sibling, 1 reply; 21+ messages in thread
From: Stephen Smalley @ 2025-07-21 14:18 UTC (permalink / raw)
To: Rahul Sandhu, Paul Moore, Ondrej Mosnacek; +Cc: selinux
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
> >
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-21 14:18 ` Stephen Smalley
@ 2025-07-22 5:42 ` Rahul Sandhu
2025-07-22 13:05 ` Stephen Smalley
0 siblings, 1 reply; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-22 5:42 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: nvraxn, omosnace, paul, selinux
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-22 5:42 ` Rahul Sandhu
@ 2025-07-22 13:05 ` Stephen Smalley
2025-07-22 15:36 ` Stephen Smalley
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Smalley @ 2025-07-22 13:05 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: omosnace, paul, selinux
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-22 13:05 ` Stephen Smalley
@ 2025-07-22 15:36 ` Stephen Smalley
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2025-07-22 15:36 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: omosnace, paul, selinux
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-21 12:56 ` Stephen Smalley
2025-07-21 14:18 ` Stephen Smalley
@ 2025-07-24 9:13 ` Rahul Sandhu
2025-07-24 12:28 ` Stephen Smalley
1 sibling, 1 reply; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-24 9:13 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.
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 9:13 ` [PATCH v2] " Rahul Sandhu
@ 2025-07-24 12:28 ` Stephen Smalley
2025-07-24 12:33 ` Rahul Sandhu
2025-07-24 13:05 ` [PATCH v3] " Rahul Sandhu
0 siblings, 2 replies; 21+ 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] 21+ 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 12:33 ` Rahul Sandhu
2025-07-24 13:05 ` [PATCH v3] " Rahul Sandhu
1 sibling, 0 replies; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-24 12:33 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: nvraxn, omosnace, paul, selinux
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 12:28 ` Stephen Smalley
2025-07-24 12:33 ` Rahul Sandhu
@ 2025-07-24 13:05 ` Rahul Sandhu
2025-07-24 13:27 ` Stephen Smalley
1 sibling, 1 reply; 21+ 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] 21+ 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
2025-07-24 13:51 ` [PATCH v4] " Rahul Sandhu
0 siblings, 2 replies; 21+ 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] 21+ 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
2025-07-24 13:51 ` [PATCH v4] " Rahul Sandhu
1 sibling, 0 replies; 21+ 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] 21+ messages in thread* [PATCH v4] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 13:27 ` Stephen Smalley
2025-07-24 13:30 ` Stephen Smalley
@ 2025-07-24 13:51 ` Rahul Sandhu
2025-07-24 19:29 ` Stephen Smalley
1 sibling, 1 reply; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-24 13:51 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: jwcart2, nvraxn, omosnace, paul, plautrba, 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
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v4] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 13:51 ` [PATCH v4] " Rahul Sandhu
@ 2025-07-24 19:29 ` Stephen Smalley
2025-07-25 22:03 ` Rahul Sandhu
2025-07-25 22:15 ` [PATCH v5] " Rahul Sandhu
0 siblings, 2 replies; 21+ messages in thread
From: Stephen Smalley @ 2025-07-24 19:29 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: jwcart2, omosnace, paul, plautrba, selinux
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v4] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 19:29 ` Stephen Smalley
@ 2025-07-25 22:03 ` Rahul Sandhu
2025-07-25 22:15 ` [PATCH v5] " Rahul Sandhu
1 sibling, 0 replies; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-25 22:03 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: jwcart2, nvraxn, omosnace, paul, plautrba, selinux
> 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.
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v5] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-24 19:29 ` Stephen Smalley
2025-07-25 22:03 ` Rahul Sandhu
@ 2025-07-25 22:15 ` Rahul Sandhu
2025-07-28 14:04 ` Stephen Smalley
1 sibling, 1 reply; 21+ messages in thread
From: Rahul Sandhu @ 2025-07-25 22:15 UTC (permalink / raw)
To: stephen.smalley.work; +Cc: jwcart2, nvraxn, omosnace, paul, plautrba, 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
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-25 22:15 ` [PATCH v5] " Rahul Sandhu
@ 2025-07-28 14:04 ` Stephen Smalley
2025-07-30 13:06 ` Stephen Smalley
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Smalley @ 2025-07-28 14:04 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: jwcart2, omosnace, paul, plautrba, selinux
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5] libselinux: fix parsing of the enforcing kernel cmdline parameter
2025-07-28 14:04 ` Stephen Smalley
@ 2025-07-30 13:06 ` Stephen Smalley
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2025-07-30 13:06 UTC (permalink / raw)
To: Rahul Sandhu; +Cc: jwcart2, omosnace, paul, plautrba, selinux
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
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2025-07-30 13:06 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-20 12:52 [PATCH] libselinux: fix parsing of the enforcing kernel cmdline parameter Rahul Sandhu
2025-07-21 9:01 ` robinshao007
2025-07-21 9:47 ` Rahul Sandhu
2025-07-21 9:58 ` robinshao007
2025-07-21 12:56 ` Stephen Smalley
2025-07-21 14:18 ` Stephen Smalley
2025-07-22 5:42 ` Rahul Sandhu
2025-07-22 13:05 ` Stephen Smalley
2025-07-22 15:36 ` Stephen Smalley
2025-07-24 9:13 ` [PATCH v2] " Rahul Sandhu
2025-07-24 12:28 ` Stephen Smalley
2025-07-24 12:33 ` Rahul Sandhu
2025-07-24 13:05 ` [PATCH v3] " Rahul Sandhu
2025-07-24 13:27 ` Stephen Smalley
2025-07-24 13:30 ` Stephen Smalley
2025-07-24 13:51 ` [PATCH v4] " Rahul Sandhu
2025-07-24 19:29 ` Stephen Smalley
2025-07-25 22:03 ` Rahul Sandhu
2025-07-25 22:15 ` [PATCH v5] " Rahul Sandhu
2025-07-28 14:04 ` Stephen Smalley
2025-07-30 13:06 ` Stephen Smalley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).