On 3/31/26 14:52, Peter Krempa wrote:
> 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
Yeah, me neither. But there's hardly going to be a new one.
>
>> +
>> + 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.
>
Fair enough. I've got stuck on a corner case where I set the following
seclabel:
<seclabel type='none' model='MyOwnModel'/>
and it was formatted back into the live XML. But maybe there's nothing
wrong with that. Alright, for completeness, let me post v2 which drops
knownModels[].
Michal
