On Thu, Mar 26, 2026 at 12:51:25 +0100, Michal Privoznik via Devel wrote:
> From: Michal Privoznik <[email protected]>
>
> Ever since of commit v1.2.13-rc1~66 the model attribute of a
> <seclabel/> is validated against secdriver names enabled. In
> nearly all cases this is something users want so that domain XML
> does not claim to set seclabels of a model that's not enabled.
> However, consider the following seclabel:
>
> <seclabel type='none' model='selinux'/>
>
> It tells us to not bother setting selinux labels on given domain.
> A mgmt app might format this into domain XML if it sees selinux
> is disabled on the host. But if that's the case, selinux driver
> is not loaded and this virSecurityManagerCheckModel() doesn't
> find it and reports an error.
>
> Well, the error doesn't need to be reported as we will just
> ignore selinux as each driver callback checks if relabel is false
> (which it is for type='none'). This is true for other secdrivers
> too.
>
> Resolves: https://redhat.atlassian.net/browse/RHEL-156689
> Signed-off-by: Michal Privoznik <[email protected]>
> ---
> src/security/security_manager.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index f2f3bb4f19..7023ac2db8 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -727,7 +727,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr,
>
>
> static int virSecurityManagerCheckModel(virSecurityManager *mgr,
> - char *secmodel)
> + char *secmodel,
> + bool relabel)
> {
> g_autofree virSecurityManager **sec_managers = NULL;
> size_t i;
> @@ -744,6 +745,19 @@ static int
> virSecurityManagerCheckModel(virSecurityManager *mgr,
> }
> }
>
> + if (relabel == false) {
> + const char * const knownModels[] = {
> + "none", "apparmor", "dac", "selinux"
> + };
I don't like the hardcoded list of security drivers
> +
> + for (i = 0; i < G_N_ELEMENTS(knownModels); i++) {
If you NULL-terminate the list you can use g_strv_contains instead of
the loop.
> + if (STREQ_NULLABLE(secmodel, knownModels[i])) {
> + VIR_INFO("Ignoring seclabel with model %s and relabel=no",
> secmodel);
> + return 0;
> + }
I was considering if this should behave differently if the secdriver is
explicitly disabled rather than disabled either by autoprobe or not even
compiled in.
Since for the existing secdrivers we disable them by disabling
"relabel" (e.g. there are no necessary steps to avoid confinment) I
guess this is okay.
IMO to avoid the hardcoded list you IMO should ignore any label
requesting disabling of given security driver if the driver is not
enabled. That includes unknown ones. I don't think there's a need to
report error for those.
