On Fri, Aug 30, 2024 at 17:13:44 +0200, Andrea Bolognani wrote:
> In the case of outgoing migration, we avoid restoring the
> remembered labels for the TPM state directory because doing so
> would risk cutting off storage access for the target node.
>
> Even in that case though, we should still forget (unref) the
> remembered labels: if we don't, the source node will keep
> thinking that the state directory is in use.
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/security/security_selinux.c | 54 +++++++++++++++++++++++++++++++++
I wanted to first complain that it's missing the 'dac' driver counter
part, but weirdly enough the 'dac' security driver is completely missing
the impl for:
domainSetSecurityTPMLabels and domainRestoreSecurityTPMLabels
Do we assume that the paths for the TPM emulator have always the correct
owner?
> 1 file changed, 54 insertions(+)
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 3e213a553b..4f13d305d9 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -210,6 +210,51 @@ virSecuritySELinuxRecallLabel(const char *path,
> }
>
>
> +/**
> + * virSecuritySELinuxForgetLabels:
> + * @path: file or directory to work on
> + *
> + * Forgets rememebered SELinux labels for @path, including its
> + * children if it is a directory.
> + *
> + * This is intended to be used in cleanup paths, so failure to forget
> + * a single label is not considered fatal; instead, a best-effort
> + * attempt to continue and forget as many labels as possible will be
> + * made.
> + *
> + * Returns: 0 on success, <0 on failure
> + */
> +static int
> +virSecuritySELinuxForgetLabels(const char *path)
> +{
> + int ret = 0;
> + struct dirent *ent;
> + g_autoptr(DIR) dir = NULL;
> + g_autofree char *con = NULL;
> +
> + if (virSecuritySELinuxRecallLabel(path, &con) < 0)
> + VIR_WARN("Failed to forget remembered SELinux labels for %s, ignoring", path);
> +
> + if (!virFileIsDir(path))
> + return 0;
> +
> + if (virDirOpen(&dir, path) < 0)
> + return -1;
> +
> + while ((ret = virDirRead(dir, &ent, path)) > 0) {
> + g_autofree char *spath = NULL;
> + g_autofree char *scon = NULL;
> +
> + spath = g_strdup_printf("%s/%s", path, ent->d_name);
> +
> + if (virSecuritySELinuxRecallLabel(spath, &scon) < 0)
> + VIR_WARN("Failed to forget remembered SELinux labels for %s, ignoring", spath);
> + }
> +
> + return ret;
'ret' is never updated, thus this is 'return 0';
> +}
> +
> +
> static int virSecuritySELinuxSetFilecon(virSecurityManager *mgr,
> const char *path,
> const char *tcon,
> @@ -3709,6 +3754,15 @@ virSecuritySELinuxRestoreTPMLabels(virSecurityManager *mgr,
> if (restoreTPMStateLabel) {
> ret = virSecuritySELinuxRestoreFileLabels(mgr,
> def->tpms[i]->data.emulator.storagepath);
> + } else {
> + g_autofree char *oldlabel = NULL;
This variable is unused.
> +
> + /* Even if we're not restoring the original label for the
> + * TPM state directory, we should still forget any
> + * remembered label so that a subsequent attempt at TPM
> + * startup will not fail due to the state directory being
> + * considered as still in use */
> + ignore_value(virSecuritySELinuxForgetLabels(def->tpms[i]->data.emulator.storagepath));
Why bother returning '-1' from virDirOpen() if you ignore it here. Make
the function void if you don't care.
> }
>
> if (ret == 0 &&
This function has pre-existing very questionable logic in handling
failure:
for (i = 0; i < def->ntpms; i++) {
if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR)
continue;
if (restoreTPMStateLabel) {
ret = virSecuritySELinuxRestoreFileLabels(mgr,
def->tpms[i]->data.emulator.storagepath);
}
if (ret == 0 &&
def->tpms[i]->data.emulator.logfile) {
ret = virSecuritySELinuxRestoreFileLabels(mgr,
def->tpms[i]->data.emulator.logfile);
}
}
So, if 'restoreTPMStateLabel' is true it returns failure only if the
restore of the last TPM's labels has failed and doesn't restore the
labels for just that logfile if that happens.
If restoreTPMStateLabel is false, then if restoring of one logfile's
seclabel failed then, no other ones are restored and failure is
returned.
That makes no sense.
Obviously this is for a different patch, but since you seem to be keen
on fixing labelling for TPMs ...
Thus:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
if:
- you fix all the thing's I've pointed out
- remove the return value from virSecuritySELinuxForgetLabels
- add a note to the commit message saying that this is doing just
selinux stuff because the DAC stuff actually isn't implemented.