[libguestfs PATCH 0/2] daemon/selinux-relabel: tolerate relabeling errors
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1794518 In the "selinux-relabel" API, utilize the "-C" option of setfiles(8), if it is available. (It's going to be part of the policycoreutils-3.4 release.) See patch#2 for a bit longer explanation. Thanks Laszlo Laszlo Ersek (2): daemon/selinux-relabel: generalize setfiles_has_m_option() daemon/selinux-relabel: tolerate relabeling errors daemon/selinux-relabel.c | 36 +++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) base-commit: 08c4ac90f5a3c08b48444e2faf3d0f58d6ddc206 -- 2.19.1.3.g30247aa5d201
Allow the caller to pass in the option to check for, and to store the result in a (usually static) variable of their choice. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1794518 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- daemon/selinux-relabel.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c index 5679a29cf141..a34287fe27cb 100644 --- a/daemon/selinux-relabel.c +++ b/daemon/selinux-relabel.c @@ -38,17 +38,19 @@ optgroup_selinuxrelabel_available (void) } static int -setfiles_has_m_option (void) +setfiles_has_option (int *flag, char opt_char) { - static int flag = -1; CLEANUP_FREE char *err = NULL; - if (flag == -1) { - ignore_value (command (NULL, &err, "setfiles", "-m", NULL)); - flag = err && strstr (err, /* "invalid option -- " */ "'m'") == NULL; + if (*flag == -1) { + char option[] = { '-', opt_char, '\0' }; /* "-X" */ + char err_opt[] = { '\'', opt_char, '\'', '\0'}; /* "'X'" */ + + ignore_value (command (NULL, &err, "setfiles", option, NULL)); + *flag = err && strstr (err, /* "invalid option -- " */ err_opt) == NULL; } - return flag; + return *flag; } /* Takes optional arguments, consult optargs_bitmask. */ @@ -56,6 +58,7 @@ int do_selinux_relabel (const char *specfile, const char *path, int force) { + static int flag_m = -1; const char *argv[MAX_ARGS]; CLEANUP_FREE char *s_dev = NULL, *s_proc = NULL, *s_selinux = NULL, *s_sys = NULL, *s_specfile = NULL, *s_path = NULL; @@ -101,7 +104,7 @@ do_selinux_relabel (const char *specfile, const char *path, * setfiles puts all the mountpoints on the excludes list for no * useful reason (RHBZ#1433577). */ - if (setfiles_has_m_option ()) + if (setfiles_has_option (&flag_m, 'm')) ADD_ARG (argv, i, "-m"); /* Relabelling in a chroot. */ -- 2.19.1.3.g30247aa5d201
Option "-C" of setfiles(8) causes setfiles(8) to exit with status 1 rather than status 255 if it encounters relabeling errors, but no other (fatal) error. Pass "-C" to setfiles(8) in "selinux-relabel", because we don't want the "selinux-relabel" API to fail if setfiles(8) only encounters relabeling errors. (NB even without "-C", setfiles(8) continues traversing the directory tree(s) and relabeling files across relabeling errors, so this change is specifically about the exit status.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1794518 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- daemon/selinux-relabel.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c index a34287fe27cb..976cffe37261 100644 --- a/daemon/selinux-relabel.c +++ b/daemon/selinux-relabel.c @@ -59,11 +59,13 @@ do_selinux_relabel (const char *specfile, const char *path, int force) { static int flag_m = -1; + static int flag_C = -1; const char *argv[MAX_ARGS]; CLEANUP_FREE char *s_dev = NULL, *s_proc = NULL, *s_selinux = NULL, *s_sys = NULL, *s_specfile = NULL, *s_path = NULL; CLEANUP_FREE char *err = NULL; size_t i = 0; + int setfiles_status; s_dev = sysroot_path ("/dev"); if (!s_dev) { @@ -107,6 +109,13 @@ do_selinux_relabel (const char *specfile, const char *path, if (setfiles_has_option (&flag_m, 'm')) ADD_ARG (argv, i, "-m"); + /* Not only do we want setfiles to trudge through individual relabeling + * errors, we also want the setfiles exit status to differentiate a fatal + * error from "relabeling errors only". See RHBZ#1794518. + */ + if (setfiles_has_option (&flag_C, 'C')) + ADD_ARG (argv, i, "-C"); + /* Relabelling in a chroot. */ if (STRNEQ (sysroot, "/")) { ADD_ARG (argv, i, "-r"); @@ -124,10 +133,10 @@ do_selinux_relabel (const char *specfile, const char *path, ADD_ARG (argv, i, s_path); ADD_ARG (argv, i, NULL); - if (commandv (NULL, &err, argv) == -1) { - reply_with_error ("%s", err); - return -1; - } + setfiles_status = commandrv (NULL, &err, argv); + if ((setfiles_status == 0) || (setfiles_status == 1 && flag_C)) + return 0; - return 0; + reply_with_error ("%s", err); + return -1; } -- 2.19.1.3.g30247aa5d201
On Wed, May 11, 2022 at 02:23:45PM +0200, Laszlo Ersek wrote:
Option "-C" of setfiles(8) causes setfiles(8) to exit with status 1 rather than status 255 if it encounters relabeling errors, but no other (fatal) error. Pass "-C" to setfiles(8) in "selinux-relabel", because we don't want the "selinux-relabel" API to fail if setfiles(8) only encounters relabeling errors.
(NB even without "-C", setfiles(8) continues traversing the directory tree(s) and relabeling files across relabeling errors, so this change is specifically about the exit status.)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1794518 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- daemon/selinux-relabel.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c index a34287fe27cb..976cffe37261 100644 --- a/daemon/selinux-relabel.c +++ b/daemon/selinux-relabel.c @@ -59,11 +59,13 @@ do_selinux_relabel (const char *specfile, const char *path, int force) { static int flag_m = -1; + static int flag_C = -1; const char *argv[MAX_ARGS]; CLEANUP_FREE char *s_dev = NULL, *s_proc = NULL, *s_selinux = NULL, *s_sys = NULL, *s_specfile = NULL, *s_path = NULL; CLEANUP_FREE char *err = NULL; size_t i = 0; + int setfiles_status;
s_dev = sysroot_path ("/dev"); if (!s_dev) { @@ -107,6 +109,13 @@ do_selinux_relabel (const char *specfile, const char *path, if (setfiles_has_option (&flag_m, 'm')) ADD_ARG (argv, i, "-m");
+ /* Not only do we want setfiles to trudge through individual relabeling + * errors, we also want the setfiles exit status to differentiate a fatal + * error from "relabeling errors only". See RHBZ#1794518. + */ + if (setfiles_has_option (&flag_C, 'C')) + ADD_ARG (argv, i, "-C"); + /* Relabelling in a chroot. */ if (STRNEQ (sysroot, "/")) { ADD_ARG (argv, i, "-r"); @@ -124,10 +133,10 @@ do_selinux_relabel (const char *specfile, const char *path, ADD_ARG (argv, i, s_path); ADD_ARG (argv, i, NULL);
- if (commandv (NULL, &err, argv) == -1) { - reply_with_error ("%s", err); - return -1; - } + setfiles_status = commandrv (NULL, &err, argv); + if ((setfiles_status == 0) || (setfiles_status == 1 && flag_C)) + return 0;
- return 0; + reply_with_error ("%s", err); + return -1; }
I've been following this one for a while, thanks for all the work especially in dark corners of the SELinux code. For the series: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 05/11/22 14:31, Richard W.M. Jones wrote:
On Wed, May 11, 2022 at 02:23:45PM +0200, Laszlo Ersek wrote:
Option "-C" of setfiles(8) causes setfiles(8) to exit with status 1 rather than status 255 if it encounters relabeling errors, but no other (fatal) error. Pass "-C" to setfiles(8) in "selinux-relabel", because we don't want the "selinux-relabel" API to fail if setfiles(8) only encounters relabeling errors.
(NB even without "-C", setfiles(8) continues traversing the directory tree(s) and relabeling files across relabeling errors, so this change is specifically about the exit status.)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1794518 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- daemon/selinux-relabel.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c index a34287fe27cb..976cffe37261 100644 --- a/daemon/selinux-relabel.c +++ b/daemon/selinux-relabel.c @@ -59,11 +59,13 @@ do_selinux_relabel (const char *specfile, const char *path, int force) { static int flag_m = -1; + static int flag_C = -1; const char *argv[MAX_ARGS]; CLEANUP_FREE char *s_dev = NULL, *s_proc = NULL, *s_selinux = NULL, *s_sys = NULL, *s_specfile = NULL, *s_path = NULL; CLEANUP_FREE char *err = NULL; size_t i = 0; + int setfiles_status;
s_dev = sysroot_path ("/dev"); if (!s_dev) { @@ -107,6 +109,13 @@ do_selinux_relabel (const char *specfile, const char *path, if (setfiles_has_option (&flag_m, 'm')) ADD_ARG (argv, i, "-m");
+ /* Not only do we want setfiles to trudge through individual relabeling + * errors, we also want the setfiles exit status to differentiate a fatal + * error from "relabeling errors only". See RHBZ#1794518. + */ + if (setfiles_has_option (&flag_C, 'C')) + ADD_ARG (argv, i, "-C"); + /* Relabelling in a chroot. */ if (STRNEQ (sysroot, "/")) { ADD_ARG (argv, i, "-r"); @@ -124,10 +133,10 @@ do_selinux_relabel (const char *specfile, const char *path, ADD_ARG (argv, i, s_path); ADD_ARG (argv, i, NULL);
- if (commandv (NULL, &err, argv) == -1) { - reply_with_error ("%s", err); - return -1; - } + setfiles_status = commandrv (NULL, &err, argv); + if ((setfiles_status == 0) || (setfiles_status == 1 && flag_C)) + return 0;
- return 0; + reply_with_error ("%s", err); + return -1; }
I've been following this one for a while, thanks for all the work especially in dark corners of the SELinux code. For the series:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Commit range 08c4ac90f5a3..a39b79f6079c. Thank you! Laszlo
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones