[PATCH 0/3] security: Don't fail if locking a file on NFS mount fails
*** 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
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@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
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@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
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@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
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@redhat.com> as this is fixing a real bug oVirt is hitting.
participants (2)
-
Michal Privoznik -
Peter Krempa