[libvirt] [PATCH 0/6] security_selinux: Don't store XATTRs if FS fakes SELinux
For full explanation see 6/6, but here's a digest: GlusterFS via FUSE supports XATTRs but doesn't allow any SELinux label change (which is fortunate for us because migrations work at least). However, we need to treat this situation as "don't remember any seclabels" because if the source sets XATTRs and the migration destination tries to set different label this fails. Michal Prívozník (6): virSecuritySELinuxGetProcessLabel: Fix comment virSecuritySELinuxSetFileconImpl: Drop @optional argument security_selinux: DropvirSecuritySELinuxSetFileconOptional() security_selinux: Drop @optional from _virSecuritySELinuxContextItem security_selinux: Drop virSecuritySELinuxSetFileconHelper security_selinux: Play nicely with network FS that only emulates SELinux src/security/security_selinux.c | 141 ++++++++++++++++---------------- 1 file changed, 70 insertions(+), 71 deletions(-) -- 2.21.0
This function has funny approach to retvals. Document them more clearly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9857223bbf..0523613d4a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1257,9 +1257,20 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } -/* Attempt to change the label of PATH to TCON. If OPTIONAL is true, - * return 1 if labelling was not possible. Otherwise, require a label - * change, and return 0 for success, -1 for failure. */ +/** + * virSecuritySELinuxSetFileconImpl: + * @path: path to the file to set context on + * @tcon: target context to set + * @optional: whether to treat errors as fatal + * @privileged: whether running as privileged user + * + * Set @tcon SELinux context on @path. If unable to do so, check SELinux + * configuration and produce sensible error message suggesting solution. + * + * Returns: -1 if failed to set context and SELinux is in enforcing mode + * 1 if failed to set context and @optional is true + * 0 otherwise. + */ static int virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, bool optional, bool privileged) -- 2.21.0
On Thu, Aug 22, 2019 at 05:19:04PM +0200, Michal Privoznik wrote:
This function has funny approach to retvals. Document them more clearly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9857223bbf..0523613d4a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1257,9 +1257,20 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; }
-/* Attempt to change the label of PATH to TCON. If OPTIONAL is true, - * return 1 if labelling was not possible. Otherwise, require a label - * change, and return 0 for success, -1 for failure. */ +/** + * virSecuritySELinuxSetFileconImpl: + * @path: path to the file to set context on + * @tcon: target context to set + * @optional: whether to treat errors as fatal + * @privileged: whether running as privileged user + * + * Set @tcon SELinux context on @path. If unable to do so, check SELinux + * configuration and produce sensible error message suggesting solution. + * + * Returns: -1 if failed to set context and SELinux is in enforcing mode + * 1 if failed to set context and @optional is true + * 0 otherwise.
It's not really how it behaves right now. Not that it matters, but maybe fixing the function and then commenting it would be easier. I would prefer If you fixed up the description for return values related to review in PATCH 2 or squashed PATCHes 1 and 2 together. Either way: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The only thing that the @optional argument does is that it makes the function return 1 instead of 0 if setting SELinux context failed in a non-critical fashion. Drop the argument then and return 1 in that case. This enables caller to learn if SELinux context was set or not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0523613d4a..35385f4a23 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1261,19 +1261,23 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * virSecuritySELinuxSetFileconImpl: * @path: path to the file to set context on * @tcon: target context to set - * @optional: whether to treat errors as fatal * @privileged: whether running as privileged user * * Set @tcon SELinux context on @path. If unable to do so, check SELinux * configuration and produce sensible error message suggesting solution. + * It may happen that setting context fails but hypervisor will be able to + * open the @path successfully. This is because some file systems don't + * support SELinux, are RO, or the @path had the correct context from the + * start. If that is the case, a positive one is returned. * * Returns: -1 if failed to set context and SELinux is in enforcing mode - * 1 if failed to set context and @optional is true - * 0 otherwise. + * 1 if failed to set context, + * 0 if context was set successfully. */ static int -virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, - bool optional, bool privileged) +virSecuritySELinuxSetFileconImpl(const char *path, + const char *tcon, + bool privileged) { security_context_t econ; @@ -1289,7 +1293,7 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ - return optional ? 1 : 0; + return 1; } freecon(econ); } @@ -1326,9 +1330,9 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, VIR_INFO("Setting security context '%s' on '%s' not supported", tcon, path); } - if (optional) - return 1; } + + return 1; } return 0; } @@ -1388,7 +1392,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) + if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0) goto cleanup; ret = 0; @@ -1553,7 +1557,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(newpath, fcon, false, privileged) < 0) + if (virSecuritySELinuxSetFileconImpl(newpath, fcon, privileged) < 0) goto cleanup; ret = 0; -- 2.21.0
On Thu, Aug 22, 2019 at 05:19:05PM +0200, Michal Privoznik wrote:
The only thing that the @optional argument does is that it makes the function return 1 instead of 0 if setting SELinux context failed in a non-critical fashion. Drop the argument then and return 1 in that case. This enables caller to learn if SELinux context was set or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0523613d4a..35385f4a23 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1261,19 +1261,23 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * virSecuritySELinuxSetFileconImpl: * @path: path to the file to set context on * @tcon: target context to set - * @optional: whether to treat errors as fatal * @privileged: whether running as privileged user * * Set @tcon SELinux context on @path. If unable to do so, check SELinux * configuration and produce sensible error message suggesting solution. + * It may happen that setting context fails but hypervisor will be able to + * open the @path successfully. This is because some file systems don't + * support SELinux, are RO, or the @path had the correct context from the + * start. If that is the case, a positive one is returned. * * Returns: -1 if failed to set context and SELinux is in enforcing mode - * 1 if failed to set context and @optional is true - * 0 otherwise. + * 1 if failed to set context, + * 0 if context was set successfully.
This is where this does not agree with following patches. The code works, but the comment is still a bit unclear, even though it is at least a tad less confusing (well, the code doesn't really help with constructing a proper documentation). I would go with: * Returns: 0 if context was set successfully * 1 if setting the context failed in a non-critical fashion * -1 in case of error or something along the lines of the above, critical change being the description of return value 1. Feel free to split the change between PATCH 1 and this one in a way that makes sense or just squash them together, that's fine as well. If you go with my suggestion for the comment, then: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There is no real difference between virSecuritySELinuxSetFilecon() and virSecuritySELinuxSetFileconOptional(). Drop the latter in favour of the former. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 53 ++++++++++++++------------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 35385f4a23..0d9790829e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1419,15 +1419,6 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, - const char *path, - const char *tcon, - bool remember) -{ - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, remember); -} - static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, @@ -1884,28 +1875,28 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, parent_seclabel->label, remember); } else if (!parent || parent == src) { if (src->shared) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->file_context, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + data->file_context, + remember); } else if (src->readonly) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->content_context, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + data->content_context, + remember); } else if (secdef->imagelabel) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - secdef->imagelabel, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + secdef->imagelabel, + remember); } else { ret = 0; } } else { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->content_context, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + data->content_context, + remember); } if (ret == 1 && !disk_seclabel) { @@ -2045,14 +2036,14 @@ virSecuritySELinuxSetSCSILabel(virSCSIDevicePtr dev, return 0; if (virSCSIDeviceGetShareable(dev)) - return virSecuritySELinuxSetFileconOptional(mgr, file, - data->file_context, true); + return virSecuritySELinuxSetFilecon(mgr, file, + data->file_context, true); else if (virSCSIDeviceGetReadonly(dev)) - return virSecuritySELinuxSetFileconOptional(mgr, file, - data->content_context, true); + return virSecuritySELinuxSetFilecon(mgr, file, + data->content_context, true); else - return virSecuritySELinuxSetFileconOptional(mgr, file, - secdef->imagelabel, true); + return virSecuritySELinuxSetFilecon(mgr, file, + secdef->imagelabel, true); } static int -- 2.21.0
On Thu, Aug 22, 2019 at 05:19:06PM +0200, Michal Privoznik wrote:
There is no real difference between virSecuritySELinuxSetFilecon() and virSecuritySELinuxSetFileconOptional(). Drop the latter in favour of the former.
And s/Dropvir/Drop vir/ in the subject. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Now, that we don't need to remember if setting context is 'optional' (the argument only made virSecuritySELinuxSetFileconImpl() return a different success code), we can drop it from the _virSecuritySELinuxContextItem structure as we don't need to remember it in transactions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0d9790829e..e7cf5f2e53 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -81,7 +81,6 @@ typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr; struct _virSecuritySELinuxContextItem { char *path; char *tcon; - bool optional; bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is 'set' or 'restore' */ }; @@ -122,7 +121,6 @@ static int virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *path, const char *tcon, - bool optional, bool remember, bool restore) { @@ -135,7 +133,6 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, if (VIR_STRDUP(item->path, path) < 0 || VIR_STRDUP(item->tcon, tcon) < 0) goto cleanup; - item->optional = optional; item->remember = remember; item->restore = restore; @@ -170,7 +167,6 @@ virSecuritySELinuxContextListFree(void *opaque) * virSecuritySELinuxTransactionAppend: * @path: Path to chown * @tcon: target context - * @optional: true if setting @tcon is optional * @remember: if the original owner should be recorded/recalled * @restore: if current operation is set or restore * @@ -187,7 +183,6 @@ virSecuritySELinuxContextListFree(void *opaque) static int virSecuritySELinuxTransactionAppend(const char *path, const char *tcon, - bool optional, bool remember, bool restore) { @@ -198,7 +193,7 @@ virSecuritySELinuxTransactionAppend(const char *path, return 0; if (virSecuritySELinuxContextListAppend(list, path, tcon, - optional, remember, restore) < 0) + remember, restore) < 0) return -1; return 1; @@ -234,7 +229,6 @@ virSecuritySELinuxRecallLabel(const char *path, static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, const char *path, const char *tcon, - bool optional, bool remember); @@ -290,7 +284,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, rv = virSecuritySELinuxSetFileconHelper(list->manager, item->path, item->tcon, - item->optional, remember); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, @@ -1342,7 +1335,6 @@ static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, const char *path, const char *tcon, - bool optional, bool remember) { bool privileged = virSecurityManagerGetPrivileged(mgr); @@ -1353,7 +1345,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, int ret = -1; if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, - optional, remember, false)) < 0) + remember, false)) < 0) return -1; else if (rc > 0) return 0; @@ -1425,7 +1417,7 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *tcon, bool remember) { - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, remember); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, remember); } static int @@ -1512,7 +1504,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, } if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, - false, recall, true)) < 0) { + recall, true)) < 0) { goto cleanup; } else if (rc > 0) { ret = 0; -- 2.21.0
On Thu, Aug 22, 2019 at 05:19:07PM +0200, Michal Privoznik wrote:
Now, that we don't need to remember if setting context is 'optional' (the argument only made virSecuritySELinuxSetFileconImpl() return a different success code), we can drop it from the _virSecuritySELinuxContextItem structure as we don't need to remember it in transactions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This function is no longer needed because after previous commits it's just an alias to virSecuritySELinuxSetFilecon. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e7cf5f2e53..855eaafdda 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -226,7 +226,7 @@ virSecuritySELinuxRecallLabel(const char *path, } -static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, +static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, const char *tcon, bool remember); @@ -281,10 +281,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, const bool remember = item->remember && list->lock; if (!item->restore) { - rv = virSecuritySELinuxSetFileconHelper(list->manager, - item->path, - item->tcon, - remember); + rv = virSecuritySELinuxSetFilecon(list->manager, + item->path, + item->tcon, + remember); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, item->path, @@ -1332,10 +1332,10 @@ virSecuritySELinuxSetFileconImpl(const char *path, static int -virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, - const char *path, - const char *tcon, - bool remember) +virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, + const char *path, + const char *tcon, + bool remember) { bool privileged = virSecurityManagerGetPrivileged(mgr); security_context_t econ = NULL; @@ -1411,15 +1411,6 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, - const char *path, - const char *tcon, - bool remember) -{ - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, remember); -} - static int virSecuritySELinuxFSetFilecon(int fd, char *tcon) { -- 2.21.0
On Thu, Aug 22, 2019 at 05:19:08PM +0200, Michal Privoznik wrote:
This function is no longer needed because after previous commits it's just an alias to virSecuritySELinuxSetFilecon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e7cf5f2e53..855eaafdda 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -226,7 +226,7 @@ virSecuritySELinuxRecallLabel(const char *path, }
-static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, +static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, const char *tcon, bool remember);
With indentation here fixed: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There are some network file systems that do support XATTRs (e.g. gluster via FUSE). And they appear to support SELinux too. However, not really. Problem is, that it is impossible to change SELinux label of a file stored there, and yet we claim success (rightfully - hypervisor succeeds in opening the file). But this creates a problem for us - from XATTR bookkeeping POV, we haven't changed the label and thus if we remembered any label, we must roll back and remove it. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740506 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 855eaafdda..4d0c7a46ae 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1384,12 +1384,22 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0) + if ((rc = virSecuritySELinuxSetFileconImpl(path, tcon, privileged)) < 0) goto cleanup; + /* At this point, we can claim success. However, + * virSecuritySELinuxSetFileconImpl() could returned 0 + * (SELinux label changed) or 1 (SELinux label NOT changed in + * a non-critical fashion). If the label was NOT changed, we + * must remove remembered label then - there's nothing to + * remember, is there? But of the label was changed, don't + * remove the remembered label. It's valid. */ + if (rc == 0) + rollback = false; + ret = 0; cleanup: - if (ret < 0 && rollback) { + if (rollback) { virErrorPtr origerr; virErrorPreserveLast(&origerr); -- 2.21.0
On Thu, Aug 22, 2019 at 05:19:09PM +0200, Michal Privoznik wrote:
There are some network file systems that do support XATTRs (e.g. gluster via FUSE). And they appear to support SELinux too. However, not really. Problem is, that it is impossible to change SELinux label of a file stored there, and yet we claim success (rightfully - hypervisor succeeds in opening the file). But this creates a problem for us - from XATTR bookkeeping POV, we haven't changed the label and thus if we remembered any label, we must roll back and remove it.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740506
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 855eaafdda..4d0c7a46ae 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1384,12 +1384,22 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, } }
- if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0) + if ((rc = virSecuritySELinuxSetFileconImpl(path, tcon, privileged)) < 0)
I wonder why so many people try to stuff as much as possible into the condition instead of doing: rc = func(); if (rc < 0) But anyway, this is not related to this commit, just a place to rent.
goto cleanup;
+ /* At this point, we can claim success. However, + * virSecuritySELinuxSetFileconImpl() could returned 0 + * (SELinux label changed) or 1 (SELinux label NOT changed in + * a non-critical fashion). If the label was NOT changed, we + * must remove remembered label then - there's nothing to + * remember, is there? But of the label was changed, don't
s/of/if/, but I think it is overcomplicated. Why don't you just: /* Do not try restoring the label if it was not changed * (setting it failed in a non-critical fashion) */ Either way: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On Thu, Aug 22, 2019 at 05:19:03PM +0200, Michal Privoznik wrote:
For full explanation see 6/6, but here's a digest:
GlusterFS via FUSE supports XATTRs but doesn't allow any SELinux label change (which is fortunate for us because migrations work at least). However, we need to treat this situation as "don't remember any seclabels" because if the source sets XATTRs and the migration destination tries to set different label this fails.
The series is safe for freeze as it was sent before the freeze and it fixes a bug, but we also need this in so that we get clean xattr values as soon as possible. One suggestion for a patch coming up as 7/6 (I couldn't look at that part of the code, but it was pre-existing and you didn't touch that part, unfortunately).
Michal Prívozník (6): virSecuritySELinuxGetProcessLabel: Fix comment virSecuritySELinuxSetFileconImpl: Drop @optional argument security_selinux: DropvirSecuritySELinuxSetFileconOptional() security_selinux: Drop @optional from _virSecuritySELinuxContextItem security_selinux: Drop virSecuritySELinuxSetFileconHelper security_selinux: Play nicely with network FS that only emulates SELinux
src/security/security_selinux.c | 141 ++++++++++++++++---------------- 1 file changed, 70 insertions(+), 71 deletions(-)
-- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I guess the reason for that was the automatic interpretation/stringification of setfilecon_errno, but the code was not nice to read and it was a bit confusing. Also, the logs and error states get cleaner this way. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- I'm still waiting for the build in travis to finish, so don't stone me if it fails. The link is here: https://travis-ci.org/nertpinx/libvirt/builds/578418517 src/security/security_selinux.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 4d0c7a46ae23..bbb5318aa0ee 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1301,14 +1301,18 @@ virSecuritySELinuxSetFileconImpl(const char *path, if (setfilecon_errno != EOPNOTSUPP && setfilecon_errno != ENOTSUP && setfilecon_errno != EROFS) { VIR_WARNINGS_RESET - virReportSystemError(setfilecon_errno, - _("unable to set security context '%s' on '%s'"), - tcon, path); /* However, don't claim error if SELinux is in Enforcing mode and * we are running as unprivileged user and we really did see EPERM. * Otherwise we want to return error if SELinux is Enforcing. */ - if (security_getenforce() == 1 && (setfilecon_errno != EPERM || privileged)) + if (security_getenforce() == 1 && + (setfilecon_errno != EPERM || privileged)) { + virReportSystemError(setfilecon_errno, + _("unable to set security context '%s' on '%s'"), + tcon, path); return -1; + } + VIR_WARN(_("unable to set security context '%s' on '%s' (errno %d)"), + tcon, path, setfilecon_errno); } else { const char *msg; if (virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 1 && -- 2.23.0
On Thu, Aug 29, 2019 at 05:39:11PM +0200, Martin Kletzander wrote:
I guess the reason for that was the automatic interpretation/stringification of setfilecon_errno, but the code was not nice to read and it was a bit confusing. Also, the logs and error states get cleaner this way.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- I'm still waiting for the build in travis to finish, so don't stone me if it fails. The link is here: https://travis-ci.org/nertpinx/libvirt/builds/578418517
Of course I sent the wrong link. Right one is here: https://travis-ci.org/nertpinx/libvirt/builds/578419238
On 8/29/19 5:39 PM, Martin Kletzander wrote:
I guess the reason for that was the automatic interpretation/stringification of setfilecon_errno, but the code was not nice to read and it was a bit confusing. Also, the logs and error states get cleaner this way.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- I'm still waiting for the build in travis to finish, so don't stone me if it fails. The link is here: https://travis-ci.org/nertpinx/libvirt/builds/578418517
src/security/security_selinux.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> And safe for freeze. Michal
participants (2)
-
Martin Kletzander -
Michal Privoznik