3:33 a.m.
*** BLURB HERE ***
Michal Prívozník (3):
virSecurityManagerMetadataLock: Store locked paths
security: Don't remember seclabel for paths we haven't locked
successfully
security: Don't fail if locking a file on NFS mount fails
src/security/security_dac.c | 14 ++++++++++++++
src/security/security_manager.c | 29 ++++++++++++++++++-----------
src/security/security_manager.h | 6 ++++++
src/security/security_selinux.c | 14 ++++++++++++++
4 files changed, 52 insertions(+), 11 deletions(-)
--
2.24.1
3:33 a.m.
New subject: [PATCH 1/3] virSecurityManagerMetadataLock: Store locked paths
So far, in the lock state we are storing only the file
descriptors of the files we've locked. Therefore, when unlocking
them and something does wrong the only thing we can report is FD
number, which is not user friendly at all. But if we store paths
among with FDs we can do better error reporting.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_manager.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index fe9def7fb9..aea8cc2fb9 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1246,8 +1246,9 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
struct _virSecurityManagerMetadataLockState {
- size_t nfds;
+ size_t nfds; /* Captures size of both @fds and @paths */
int *fds;
+ const char **paths;
};
@@ -1278,6 +1279,7 @@ cmpstringp(const void *p1, const void *p2)
*
* Lock passed @paths for metadata change. The returned state
* should be passed to virSecurityManagerMetadataUnlock.
+ * Passed @paths must not be freed until the corresponding unlock call.
*
* NOTE: this function is not thread safe (because of usage of
* POSIX locks).
@@ -1293,9 +1295,11 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
size_t i = 0;
size_t nfds = 0;
int *fds = NULL;
+ const char **locked_paths = NULL;
virSecurityManagerMetadataLockStatePtr ret = NULL;
- if (VIR_ALLOC_N(fds, npaths) < 0)
+ if (VIR_ALLOC_N(fds, npaths) < 0 ||
+ VIR_ALLOC_N(locked_paths, npaths) < 0)
return NULL;
/* Sort paths to lock in order to avoid deadlocks with other
@@ -1374,12 +1378,14 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
break;
} while (1);
+ locked_paths[nfds] = p;
VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
}
if (VIR_ALLOC(ret) < 0)
goto cleanup;
+ ret->paths = g_steal_pointer(&locked_paths);
ret->fds = g_steal_pointer(&fds);
ret->nfds = nfds;
nfds = 0;
@@ -1388,6 +1394,7 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
for (i = nfds; i > 0; i--)
VIR_FORCE_CLOSE(fds[i - 1]);
VIR_FREE(fds);
+ VIR_FREE(locked_paths);
return ret;
}
@@ -1403,21 +1410,23 @@ virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
for (i = 0; i < (*state)->nfds; i++) {
char ebuf[1024];
+ const char *path = (*state)->paths[i];
int fd = (*state)->fds[i];
/* Technically, unlock is not needed because it will
* happen on VIR_CLOSE() anyway. But let's play it nice. */
if (virFileUnlock(fd, METADATA_OFFSET, METADATA_LEN) < 0) {
- VIR_WARN("Unable to unlock fd %d: %s",
- fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+ VIR_WARN("Unable to unlock fd %d path %s: %s",
+ fd, path, virStrerror(errno, ebuf, sizeof(ebuf)));
}
if (VIR_CLOSE(fd) < 0) {
- VIR_WARN("Unable to close fd %d: %s",
- fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+ VIR_WARN("Unable to close fd %d path %s: %s",
+ fd, path, virStrerror(errno, ebuf, sizeof(ebuf)));
}
}
VIR_FREE((*state)->fds);
+ VIR_FREE((*state)->paths);
VIR_FREE(*state);
}
--
2.24.1
3:33 a.m.
New subject: [PATCH 2/3] security: Don't remember seclabel for paths we haven't locked successfully
There are some cases where we want to remember the original owner
of a file but we fail to lock it for XATTR change (e.g. root
squashed NFS). If that is the case we error out and refuse to
start a domain. Well, we can do better if we disable remembering
for paths we haven't locked successfully.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 14 ++++++++++++++
src/security/security_manager.c | 7 -------
src/security/security_manager.h | 6 ++++++
src/security/security_selinux.c | 14 ++++++++++++++
4 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d75b18170b..f412054d0e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -240,6 +240,20 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED,
if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
goto cleanup;
+
+ for (i = 0; i < list->nItems; i++) {
+ virSecurityDACChownItemPtr item = list->items[i];
+ size_t j;
+
+ for (j = 0; j < state->nfds; j++) {
+ if (STREQ_NULLABLE(item->path, state->paths[j]))
+ break;
+ }
+
+ /* If path wasn't locked, don't try to remember its label. */
+ if (j == state->nfds)
+ item->remember = false;
+ }
}
for (i = 0; i < list->nItems; i++) {
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index aea8cc2fb9..1e998a6579 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1245,13 +1245,6 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
}
-struct _virSecurityManagerMetadataLockState {
- size_t nfds; /* Captures size of both @fds and @paths */
- int *fds;
- const char **paths;
-};
-
-
static int
cmpstringp(const void *p1, const void *p2)
{
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index f835356b7e..b92ea5dc87 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -203,6 +203,12 @@ int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState;
typedef virSecurityManagerMetadataLockState *virSecurityManagerMetadataLockStatePtr;
+struct _virSecurityManagerMetadataLockState {
+ size_t nfds; /* Captures size of both @fds and @paths */
+ int *fds;
+ const char **paths;
+};
+
virSecurityManagerMetadataLockStatePtr
virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 3f6968a57a..2241a35e6e 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -271,6 +271,20 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED,
if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
goto cleanup;
+
+ for (i = 0; i < list->nItems; i++) {
+ virSecuritySELinuxContextItemPtr item = list->items[i];
+ size_t j;
+
+ for (j = 0; j < state->nfds; j++) {
+ if (STREQ_NULLABLE(item->path, state->paths[j]))
+ break;
+ }
+
+ /* If path wasn't locked, don't try to remember its label. */
+ if (j == state->nfds)
+ item->remember = false;
+ }
}
rv = 0;
--
2.24.1
3:33 a.m.
New subject: [PATCH 3/3] security: Don't fail if locking a file on NFS mount fails
The way that our file locking works is that we open() the file we
want to lock and then use fcntl(fd, F_SETLKW, ...) to lock it.
The problem is, we are doing all of these as root which doesn't
work if the file lives on root squashed NFS, because if it does
then the open() fails. The way to resolve this is to make this a
non fatal error and leave callers deal with this (i.e. disable
remembering) - implemented in the previous commit.
https://bugzilla.redhat.com/show_bug.cgi?id=1804672
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_manager.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 1e998a6579..9f4bd7f8df 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1345,6 +1345,11 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
}
#endif /* !WIN32 */
+ if (virFileIsSharedFS(p)) {
+ /* Probably a root squashed NFS. */
+ continue;
+ }
+
virReportSystemError(errno,
_("unable to open %s"),
p);
--
2.24.1
2:28 a.m.
On Fri, Feb 21, 2020 at 10:33:00 +0100, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (3):
virSecurityManagerMetadataLock: Store locked paths
security: Don't remember seclabel for paths we haven't locked
successfully
security: Don't fail if locking a file on NFS mount fails
I'm not quite happy about the technical debt this series is adding and
the code itself even had before. Tracking the paths and filedescriptors
in two separate arrays is one of them. The abduance of existing and
added n^2 algorithms is another. The fact that the metadata locking
function is modifying the order of the paths in the argument is also
weird and undocumented.
series:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
as this is fixing a real bug oVirt is hitting.
1732
days inactive
1736
days old
4 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa