* [PATCH] libselinux: prioritize local literal fcontext definitions
@ 2025-04-17 19:08 Christian Göttsche
2025-04-18 19:13 ` James Carter
0 siblings, 1 reply; 5+ messages in thread
From: Christian Göttsche @ 2025-04-17 19:08 UTC (permalink / raw)
To: selinux; +Cc: Christian Göttsche
From: Christian Göttsche <cgzones@googlemail.com>
For literal file context definitions respect overrides from homedirs or
local configurations by ordering them first.
Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
Reported-by: Paul Holzinger
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2360183
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
libselinux/src/label_file.c | 5 +++--
libselinux/src/label_file.h | 10 +++++++++-
libselinux/src/selinux_internal.h | 2 ++
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 5d73fb84..61a9ddb6 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -480,7 +480,7 @@ static int load_mmap_ctxarray(struct mmap_area *mmap_area, const char *path, str
return 0;
}
-static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
+static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating, uint8_t inputno,
struct literal_spec *lspec, const struct context_array *ctx_array)
{
uint32_t data_u32, ctx_id;
@@ -489,6 +489,7 @@ static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
int rc;
lspec->from_mmap = true;
+ lspec->inputno = inputno;
/*
@@ -742,7 +743,7 @@ static int load_mmap_spec_node(struct mmap_area *mmap_area, const char *path, bo
node->literal_specs_alloc = lspec_num;
for (uint32_t i = 0; i < lspec_num; i++) {
- rc = load_mmap_literal_spec(mmap_area, validating, &node->literal_specs[i], ctx_array);
+ rc = load_mmap_literal_spec(mmap_area, validating, inputno, &node->literal_specs[i], ctx_array);
if (rc)
return -1;
}
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index 67db78e5..b7aed0bc 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -96,6 +96,7 @@ struct literal_spec {
char *regex_str; /* original regular expression string for diagnostics */
char *literal_match; /* simplified string from regular expression */
uint16_t prefix_len; /* length of fixed path prefix, i.e. length of the literal match */
+ uint8_t inputno; /* Input number of source file */
uint8_t file_kind; /* file type */
bool any_matches; /* whether any pathname match */
bool from_mmap; /* whether this spec is from an mmap of the data */
@@ -368,7 +369,13 @@ static inline int compare_literal_spec(const void *p1, const void *p2)
return ret;
/* Order wildcard mode (0) last */
- return (l1->file_kind < l2->file_kind) - (l1->file_kind > l2->file_kind);
+ ret = spaceship_cmp(l1->file_kind, l2->file_kind);
+ if (ret)
+ return -ret;
+
+ /* Order by input number (higher number means added later, means higher priority) */
+ ret = spaceship_cmp(l1->inputno, l2->inputno);
+ return -ret;
}
static inline int compare_spec_node(const void *p1, const void *p2)
@@ -754,6 +761,7 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
.regex_str = regex,
.prefix_len = prefix_len,
.literal_match = literal_regex,
+ .inputno = inputno,
.file_kind = file_kind,
.any_matches = false,
.lr.ctx_raw = context,
diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
index 964b8418..3fe7d4c3 100644
--- a/libselinux/src/selinux_internal.h
+++ b/libselinux/src/selinux_internal.h
@@ -150,4 +150,6 @@ static inline void fclose_errno_safe(FILE *stream)
# define unlikely(x) (x)
#endif /* __GNUC__ */
+#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
+
#endif /* SELINUX_INTERNAL_H_ */
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] libselinux: prioritize local literal fcontext definitions
2025-04-17 19:08 [PATCH] libselinux: prioritize local literal fcontext definitions Christian Göttsche
@ 2025-04-18 19:13 ` James Carter
2025-04-25 17:19 ` Christian Göttsche
0 siblings, 1 reply; 5+ messages in thread
From: James Carter @ 2025-04-18 19:13 UTC (permalink / raw)
To: Christian Göttsche; +Cc: selinux
On Thu, Apr 17, 2025 at 3:18 PM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> For literal file context definitions respect overrides from homedirs or
> local configurations by ordering them first.
>
> Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
> Reported-by: Paul Holzinger
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2360183
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> libselinux/src/label_file.c | 5 +++--
> libselinux/src/label_file.h | 10 +++++++++-
> libselinux/src/selinux_internal.h | 2 ++
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index 5d73fb84..61a9ddb6 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -480,7 +480,7 @@ static int load_mmap_ctxarray(struct mmap_area *mmap_area, const char *path, str
> return 0;
> }
>
> -static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
> +static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating, uint8_t inputno,
> struct literal_spec *lspec, const struct context_array *ctx_array)
> {
> uint32_t data_u32, ctx_id;
> @@ -489,6 +489,7 @@ static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
> int rc;
>
> lspec->from_mmap = true;
> + lspec->inputno = inputno;
>
>
> /*
> @@ -742,7 +743,7 @@ static int load_mmap_spec_node(struct mmap_area *mmap_area, const char *path, bo
> node->literal_specs_alloc = lspec_num;
>
> for (uint32_t i = 0; i < lspec_num; i++) {
> - rc = load_mmap_literal_spec(mmap_area, validating, &node->literal_specs[i], ctx_array);
> + rc = load_mmap_literal_spec(mmap_area, validating, inputno, &node->literal_specs[i], ctx_array);
> if (rc)
> return -1;
> }
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> index 67db78e5..b7aed0bc 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -96,6 +96,7 @@ struct literal_spec {
> char *regex_str; /* original regular expression string for diagnostics */
> char *literal_match; /* simplified string from regular expression */
> uint16_t prefix_len; /* length of fixed path prefix, i.e. length of the literal match */
> + uint8_t inputno; /* Input number of source file */
> uint8_t file_kind; /* file type */
> bool any_matches; /* whether any pathname match */
> bool from_mmap; /* whether this spec is from an mmap of the data */
> @@ -368,7 +369,13 @@ static inline int compare_literal_spec(const void *p1, const void *p2)
> return ret;
>
> /* Order wildcard mode (0) last */
> - return (l1->file_kind < l2->file_kind) - (l1->file_kind > l2->file_kind);
> + ret = spaceship_cmp(l1->file_kind, l2->file_kind);
> + if (ret)
> + return -ret;
> +
These four lines should be removed. It makes sense to consider the
wildcard mode as less specific and give priority to a rule that is not
using a wildcard, but that is not how it was done in the past and that
is not (from my testing) what is being done if a regex is involved. So
for both consistency and in keeping with past practice, we should not
use the file kind to sort here.
By my testing, everything works as expected with those lines removed.
The rest of the patch looks good to me.
Thanks,
Jim
> + /* Order by input number (higher number means added later, means higher priority) */
> + ret = spaceship_cmp(l1->inputno, l2->inputno);
> + return -ret;
> }
>
> static inline int compare_spec_node(const void *p1, const void *p2)
> @@ -754,6 +761,7 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
> .regex_str = regex,
> .prefix_len = prefix_len,
> .literal_match = literal_regex,
> + .inputno = inputno,
> .file_kind = file_kind,
> .any_matches = false,
> .lr.ctx_raw = context,
> diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
> index 964b8418..3fe7d4c3 100644
> --- a/libselinux/src/selinux_internal.h
> +++ b/libselinux/src/selinux_internal.h
> @@ -150,4 +150,6 @@ static inline void fclose_errno_safe(FILE *stream)
> # define unlikely(x) (x)
> #endif /* __GNUC__ */
>
> +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
> +
> #endif /* SELINUX_INTERNAL_H_ */
> --
> 2.49.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libselinux: prioritize local literal fcontext definitions
2025-04-18 19:13 ` James Carter
@ 2025-04-25 17:19 ` Christian Göttsche
2025-04-25 17:52 ` James Carter
0 siblings, 1 reply; 5+ messages in thread
From: Christian Göttsche @ 2025-04-25 17:19 UTC (permalink / raw)
To: James Carter; +Cc: selinux
On Fri, 18 Apr 2025 at 21:13, James Carter <jwcart2@gmail.com> wrote:
>
> On Thu, Apr 17, 2025 at 3:18 PM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
> >
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > For literal file context definitions respect overrides from homedirs or
> > local configurations by ordering them first.
> >
> > Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
> > Reported-by: Paul Holzinger
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2360183
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> > libselinux/src/label_file.c | 5 +++--
> > libselinux/src/label_file.h | 10 +++++++++-
> > libselinux/src/selinux_internal.h | 2 ++
> > 3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index 5d73fb84..61a9ddb6 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -480,7 +480,7 @@ static int load_mmap_ctxarray(struct mmap_area *mmap_area, const char *path, str
> > return 0;
> > }
> >
> > -static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
> > +static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating, uint8_t inputno,
> > struct literal_spec *lspec, const struct context_array *ctx_array)
> > {
> > uint32_t data_u32, ctx_id;
> > @@ -489,6 +489,7 @@ static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
> > int rc;
> >
> > lspec->from_mmap = true;
> > + lspec->inputno = inputno;
> >
> >
> > /*
> > @@ -742,7 +743,7 @@ static int load_mmap_spec_node(struct mmap_area *mmap_area, const char *path, bo
> > node->literal_specs_alloc = lspec_num;
> >
> > for (uint32_t i = 0; i < lspec_num; i++) {
> > - rc = load_mmap_literal_spec(mmap_area, validating, &node->literal_specs[i], ctx_array);
> > + rc = load_mmap_literal_spec(mmap_area, validating, inputno, &node->literal_specs[i], ctx_array);
> > if (rc)
> > return -1;
> > }
> > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> > index 67db78e5..b7aed0bc 100644
> > --- a/libselinux/src/label_file.h
> > +++ b/libselinux/src/label_file.h
> > @@ -96,6 +96,7 @@ struct literal_spec {
> > char *regex_str; /* original regular expression string for diagnostics */
> > char *literal_match; /* simplified string from regular expression */
> > uint16_t prefix_len; /* length of fixed path prefix, i.e. length of the literal match */
> > + uint8_t inputno; /* Input number of source file */
> > uint8_t file_kind; /* file type */
> > bool any_matches; /* whether any pathname match */
> > bool from_mmap; /* whether this spec is from an mmap of the data */
> > @@ -368,7 +369,13 @@ static inline int compare_literal_spec(const void *p1, const void *p2)
> > return ret;
> >
> > /* Order wildcard mode (0) last */
> > - return (l1->file_kind < l2->file_kind) - (l1->file_kind > l2->file_kind);
> > + ret = spaceship_cmp(l1->file_kind, l2->file_kind);
> > + if (ret)
> > + return -ret;
> > +
> These four lines should be removed. It makes sense to consider the
> wildcard mode as less specific and give priority to a rule that is not
> using a wildcard, but that is not how it was done in the past and that
> is not (from my testing) what is being done if a regex is involved. So
> for both consistency and in keeping with past practice, we should not
> use the file kind to sort here.
>
> By my testing, everything works as expected with those lines removed.
The only difference I am seeing is when omitting specifying the file
type to lookup (-t for selabel_lookup, -m for matchpathcon).
Previously the last defined definition was returned, now the one with
the highest LABEL_FILE_KIND_* value (e.g. regular has currently the
highest value of 7).
But in practice that should not matter since for corretly labeling
files the type is important, and one could argue it is unspecified
what definition is returned if the type is omitted.
If the order should not depend on the mode one would need to store the
file linenumber in the compiled format (i.e. a binary format change),
since otherwise the order would be dependent on the used sorting
algorithm/implementation.
> The rest of the patch looks good to me.
>
> Thanks,
> Jim
>
> > + /* Order by input number (higher number means added later, means higher priority) */
> > + ret = spaceship_cmp(l1->inputno, l2->inputno);
> > + return -ret;
> > }
> >
> > static inline int compare_spec_node(const void *p1, const void *p2)
> > @@ -754,6 +761,7 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
> > .regex_str = regex,
> > .prefix_len = prefix_len,
> > .literal_match = literal_regex,
> > + .inputno = inputno,
> > .file_kind = file_kind,
> > .any_matches = false,
> > .lr.ctx_raw = context,
> > diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
> > index 964b8418..3fe7d4c3 100644
> > --- a/libselinux/src/selinux_internal.h
> > +++ b/libselinux/src/selinux_internal.h
> > @@ -150,4 +150,6 @@ static inline void fclose_errno_safe(FILE *stream)
> > # define unlikely(x) (x)
> > #endif /* __GNUC__ */
> >
> > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
> > +
> > #endif /* SELINUX_INTERNAL_H_ */
> > --
> > 2.49.0
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libselinux: prioritize local literal fcontext definitions
2025-04-25 17:19 ` Christian Göttsche
@ 2025-04-25 17:52 ` James Carter
2025-05-15 9:52 ` Petr Lautrbach
0 siblings, 1 reply; 5+ messages in thread
From: James Carter @ 2025-04-25 17:52 UTC (permalink / raw)
To: Christian Göttsche; +Cc: selinux
On Fri, Apr 25, 2025 at 1:19 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Fri, 18 Apr 2025 at 21:13, James Carter <jwcart2@gmail.com> wrote:
> >
> > On Thu, Apr 17, 2025 at 3:18 PM Christian Göttsche
> > <cgoettsche@seltendoof.de> wrote:
> > >
> > > From: Christian Göttsche <cgzones@googlemail.com>
> > >
> > > For literal file context definitions respect overrides from homedirs or
> > > local configurations by ordering them first.
> > >
> > > Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
> > > Reported-by: Paul Holzinger
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2360183
> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > ---
> > > libselinux/src/label_file.c | 5 +++--
> > > libselinux/src/label_file.h | 10 +++++++++-
> > > libselinux/src/selinux_internal.h | 2 ++
> > > 3 files changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > > index 5d73fb84..61a9ddb6 100644
> > > --- a/libselinux/src/label_file.c
> > > +++ b/libselinux/src/label_file.c
> > > @@ -480,7 +480,7 @@ static int load_mmap_ctxarray(struct mmap_area *mmap_area, const char *path, str
> > > return 0;
> > > }
> > >
> > > -static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
> > > +static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating, uint8_t inputno,
> > > struct literal_spec *lspec, const struct context_array *ctx_array)
> > > {
> > > uint32_t data_u32, ctx_id;
> > > @@ -489,6 +489,7 @@ static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
> > > int rc;
> > >
> > > lspec->from_mmap = true;
> > > + lspec->inputno = inputno;
> > >
> > >
> > > /*
> > > @@ -742,7 +743,7 @@ static int load_mmap_spec_node(struct mmap_area *mmap_area, const char *path, bo
> > > node->literal_specs_alloc = lspec_num;
> > >
> > > for (uint32_t i = 0; i < lspec_num; i++) {
> > > - rc = load_mmap_literal_spec(mmap_area, validating, &node->literal_specs[i], ctx_array);
> > > + rc = load_mmap_literal_spec(mmap_area, validating, inputno, &node->literal_specs[i], ctx_array);
> > > if (rc)
> > > return -1;
> > > }
> > > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> > > index 67db78e5..b7aed0bc 100644
> > > --- a/libselinux/src/label_file.h
> > > +++ b/libselinux/src/label_file.h
> > > @@ -96,6 +96,7 @@ struct literal_spec {
> > > char *regex_str; /* original regular expression string for diagnostics */
> > > char *literal_match; /* simplified string from regular expression */
> > > uint16_t prefix_len; /* length of fixed path prefix, i.e. length of the literal match */
> > > + uint8_t inputno; /* Input number of source file */
> > > uint8_t file_kind; /* file type */
> > > bool any_matches; /* whether any pathname match */
> > > bool from_mmap; /* whether this spec is from an mmap of the data */
> > > @@ -368,7 +369,13 @@ static inline int compare_literal_spec(const void *p1, const void *p2)
> > > return ret;
> > >
> > > /* Order wildcard mode (0) last */
> > > - return (l1->file_kind < l2->file_kind) - (l1->file_kind > l2->file_kind);
> > > + ret = spaceship_cmp(l1->file_kind, l2->file_kind);
> > > + if (ret)
> > > + return -ret;
> > > +
> > These four lines should be removed. It makes sense to consider the
> > wildcard mode as less specific and give priority to a rule that is not
> > using a wildcard, but that is not how it was done in the past and that
> > is not (from my testing) what is being done if a regex is involved. So
> > for both consistency and in keeping with past practice, we should not
> > use the file kind to sort here.
> >
> > By my testing, everything works as expected with those lines removed.
>
> The only difference I am seeing is when omitting specifying the file
> type to lookup (-t for selabel_lookup, -m for matchpathcon).
> Previously the last defined definition was returned, now the one with
> the highest LABEL_FILE_KIND_* value (e.g. regular has currently the
> highest value of 7).
That is the difference.
> But in practice that should not matter since for corretly labeling
> files the type is important, and one could argue it is unspecified
> what definition is returned if the type is omitted.
>
It is not unspecified. It means "all file types"
> If the order should not depend on the mode one would need to store the
> file linenumber in the compiled format (i.e. a binary format change),
> since otherwise the order would be dependent on the used sorting
> algorithm/implementation.
>
I am not following what you are saying here.
In my testing, the file type only mattered when there was no regex in the path.
If you specify a file context with a regex in an fc file, then
regardless of whether the file type was a regular file or it was not
specified, setting the file context with semanage would override it.
But if you specify a file context that does not have a regex in an fc
file, then setting the file context with semanage would override it
only if you set the file type or if the file context in the fc file
did not specify a file type.
So it is more consistent to not sort based on the file type here.
Also not sorting based on file type matches the old behavior.
If this was new and we did not have to worry about regressions, then I
would agree with you. I think that it would make sense to have a fc
rule that specified a specific file type be selected over one that did
not (everything else being equal).
Thanks,
Jim
> > The rest of the patch looks good to me.
> >
> > Thanks,
> > Jim
> >
> > > + /* Order by input number (higher number means added later, means higher priority) */
> > > + ret = spaceship_cmp(l1->inputno, l2->inputno);
> > > + return -ret;
> > > }
> > >
> > > static inline int compare_spec_node(const void *p1, const void *p2)
> > > @@ -754,6 +761,7 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
> > > .regex_str = regex,
> > > .prefix_len = prefix_len,
> > > .literal_match = literal_regex,
> > > + .inputno = inputno,
> > > .file_kind = file_kind,
> > > .any_matches = false,
> > > .lr.ctx_raw = context,
> > > diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
> > > index 964b8418..3fe7d4c3 100644
> > > --- a/libselinux/src/selinux_internal.h
> > > +++ b/libselinux/src/selinux_internal.h
> > > @@ -150,4 +150,6 @@ static inline void fclose_errno_safe(FILE *stream)
> > > # define unlikely(x) (x)
> > > #endif /* __GNUC__ */
> > >
> > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
> > > +
> > > #endif /* SELINUX_INTERNAL_H_ */
> > > --
> > > 2.49.0
> > >
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libselinux: prioritize local literal fcontext definitions
2025-04-25 17:52 ` James Carter
@ 2025-05-15 9:52 ` Petr Lautrbach
0 siblings, 0 replies; 5+ messages in thread
From: Petr Lautrbach @ 2025-05-15 9:52 UTC (permalink / raw)
To: James Carter, Christian Göttsche; +Cc: selinux
James Carter <jwcart2@gmail.com> writes:
> On Fri, Apr 25, 2025 at 1:19 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
>>
>> On Fri, 18 Apr 2025 at 21:13, James Carter <jwcart2@gmail.com> wrote:
>> >
>> > On Thu, Apr 17, 2025 at 3:18 PM Christian Göttsche
>> > <cgoettsche@seltendoof.de> wrote:
>> > >
>> > > From: Christian Göttsche <cgzones@googlemail.com>
>> > >
>> > > For literal file context definitions respect overrides from homedirs or
>> > > local configurations by ordering them first.
>> > >
>> > > Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
>> > > Reported-by: Paul Holzinger
>> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2360183
>> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>> > > ---
>> > > libselinux/src/label_file.c | 5 +++--
>> > > libselinux/src/label_file.h | 10 +++++++++-
>> > > libselinux/src/selinux_internal.h | 2 ++
>> > > 3 files changed, 14 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> > > index 5d73fb84..61a9ddb6 100644
>> > > --- a/libselinux/src/label_file.c
>> > > +++ b/libselinux/src/label_file.c
>> > > @@ -480,7 +480,7 @@ static int load_mmap_ctxarray(struct mmap_area *mmap_area, const char *path, str
>> > > return 0;
>> > > }
>> > >
>> > > -static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
>> > > +static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating, uint8_t inputno,
>> > > struct literal_spec *lspec, const struct context_array *ctx_array)
>> > > {
>> > > uint32_t data_u32, ctx_id;
>> > > @@ -489,6 +489,7 @@ static int load_mmap_literal_spec(struct mmap_area *mmap_area, bool validating,
>> > > int rc;
>> > >
>> > > lspec->from_mmap = true;
>> > > + lspec->inputno = inputno;
>> > >
>> > >
>> > > /*
>> > > @@ -742,7 +743,7 @@ static int load_mmap_spec_node(struct mmap_area *mmap_area, const char *path, bo
>> > > node->literal_specs_alloc = lspec_num;
>> > >
>> > > for (uint32_t i = 0; i < lspec_num; i++) {
>> > > - rc = load_mmap_literal_spec(mmap_area, validating, &node->literal_specs[i], ctx_array);
>> > > + rc = load_mmap_literal_spec(mmap_area, validating, inputno, &node->literal_specs[i], ctx_array);
>> > > if (rc)
>> > > return -1;
>> > > }
>> > > diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
>> > > index 67db78e5..b7aed0bc 100644
>> > > --- a/libselinux/src/label_file.h
>> > > +++ b/libselinux/src/label_file.h
>> > > @@ -96,6 +96,7 @@ struct literal_spec {
>> > > char *regex_str; /* original regular expression string for diagnostics */
>> > > char *literal_match; /* simplified string from regular expression */
>> > > uint16_t prefix_len; /* length of fixed path prefix, i.e. length of the literal match */
>> > > + uint8_t inputno; /* Input number of source file */
>> > > uint8_t file_kind; /* file type */
>> > > bool any_matches; /* whether any pathname match */
>> > > bool from_mmap; /* whether this spec is from an mmap of the data */
>> > > @@ -368,7 +369,13 @@ static inline int compare_literal_spec(const void *p1, const void *p2)
>> > > return ret;
>> > >
>> > > /* Order wildcard mode (0) last */
>> > > - return (l1->file_kind < l2->file_kind) - (l1->file_kind > l2->file_kind);
>> > > + ret = spaceship_cmp(l1->file_kind, l2->file_kind);
>> > > + if (ret)
>> > > + return -ret;
>> > > +
>> > These four lines should be removed. It makes sense to consider the
>> > wildcard mode as less specific and give priority to a rule that is not
>> > using a wildcard, but that is not how it was done in the past and that
>> > is not (from my testing) what is being done if a regex is involved. So
>> > for both consistency and in keeping with past practice, we should not
>> > use the file kind to sort here.
>> >
>> > By my testing, everything works as expected with those lines removed.
>>
>> The only difference I am seeing is when omitting specifying the file
>> type to lookup (-t for selabel_lookup, -m for matchpathcon).
>> Previously the last defined definition was returned, now the one with
>> the highest LABEL_FILE_KIND_* value (e.g. regular has currently the
>> highest value of 7).
>
> That is the difference.
>
>> But in practice that should not matter since for corretly labeling
>> files the type is important, and one could argue it is unspecified
>> what definition is returned if the type is omitted.
>>
>
> It is not unspecified. It means "all file types"
>
>> If the order should not depend on the mode one would need to store the
>> file linenumber in the compiled format (i.e. a binary format change),
>> since otherwise the order would be dependent on the used sorting
>> algorithm/implementation.
>>
>
> I am not following what you are saying here.
>
> In my testing, the file type only mattered when there was no regex in the path.
> If you specify a file context with a regex in an fc file, then
> regardless of whether the file type was a regular file or it was not
> specified, setting the file context with semanage would override it.
> But if you specify a file context that does not have a regex in an fc
> file, then setting the file context with semanage would override it
> only if you set the file type or if the file context in the fc file
> did not specify a file type.
>
> So it is more consistent to not sort based on the file type here.
> Also not sorting based on file type matches the old behavior.
>
> If this was new and we did not have to worry about regressions, then I
> would agree with you. I think that it would make sense to have a fc
> rule that specified a specific file type be selected over one that did
> not (everything else being equal).
>
I'd like to push this forward with the change Jim proposed as it follows
the original behavior.
The patch itself does not fix the regression:
# semanage fcontext -a -t bin_t /usr/bin/sshd
# matchpathcon /usr/bin/sshd
/usr/bin/sshd system_u:object_r:bin_t:s0
Without those 4 lines
- /* Order wildcard mode (0) last */
- ret = spaceship_cmp(l1->file_kind, l2->file_kind);
- if (ret)
- return -ret;
-
# semanage fcontext -a -t bin_t /usr/bin/sshd
# matchpathcon /usr/bin/sshd
/usr/bin/sshd system_u:object_r:bin_t:s0
Christian, are you ok to update your patch?
>
>> > The rest of the patch looks good to me.
>> >
>> > Thanks,
>> > Jim
>> >
>> > > + /* Order by input number (higher number means added later, means higher priority) */
>> > > + ret = spaceship_cmp(l1->inputno, l2->inputno);
>> > > + return -ret;
>> > > }
>> > >
>> > > static inline int compare_spec_node(const void *p1, const void *p2)
>> > > @@ -754,6 +761,7 @@ static int insert_spec(const struct selabel_handle *rec, struct saved_data *data
>> > > .regex_str = regex,
>> > > .prefix_len = prefix_len,
>> > > .literal_match = literal_regex,
>> > > + .inputno = inputno,
>> > > .file_kind = file_kind,
>> > > .any_matches = false,
>> > > .lr.ctx_raw = context,
>> > > diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
>> > > index 964b8418..3fe7d4c3 100644
>> > > --- a/libselinux/src/selinux_internal.h
>> > > +++ b/libselinux/src/selinux_internal.h
>> > > @@ -150,4 +150,6 @@ static inline void fclose_errno_safe(FILE *stream)
>> > > # define unlikely(x) (x)
>> > > #endif /* __GNUC__ */
>> > >
>> > > +#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
>> > > +
>> > > #endif /* SELINUX_INTERNAL_H_ */
>> > > --
>> > > 2.49.0
>> > >
>> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-05-15 9:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-17 19:08 [PATCH] libselinux: prioritize local literal fcontext definitions Christian Göttsche
2025-04-18 19:13 ` James Carter
2025-04-25 17:19 ` Christian Göttsche
2025-04-25 17:52 ` James Carter
2025-05-15 9:52 ` Petr Lautrbach
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).