On 12/20/18 12:39 AM, John Ferlan wrote:
On 12/12/18 7:40 AM, Michal Privoznik wrote:
Similarly to what I did in DAC driver, this also requires the same SELinux label to be used for shared paths. If a path is already in use by a domain (or domains) then and the domain we are starting now wants to access the path it has to have the same SELinux label. This might look too restrictive as the new label can still guarantee access to already running domains but in reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 177 +++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 47 deletions(-)
[...]
static int @@ -1362,7 +1429,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * errors that the caller(s) are already dealing with */ static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, - const char *path) + const char *path, + bool recall) { bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; @@ -1386,26 +1454,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; }
- if (stat(newpath, &buf) != 0) { - VIR_WARN("cannot stat %s: %s", newpath, - virStrerror(errno, ebuf, sizeof(ebuf))); - goto cleanup; - } - - if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { - /* Any user created path likely does not have a default label, - * which makes this an expected non error - */ - VIR_WARN("cannot lookup default selinux label for %s", newpath); - ret = 0; - goto cleanup; - } - - if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0) return -1; else if (rc > 0) return 0;
Since you've touched the code, Coverity looks again and determines that @newpath can be leaked above
Ah, right. this should have been "goto cleanup" instead of "return -1" and "{ret = 0; goto cleanup}" instead of "return 0". Michal