[PATCH 2/2] security: Avoid calling virSecurityManagerCheckModel with NULL model

[Jim Fehlig]

On 12/3/20 7:01 AM, Michal Privoznik wrote:
> On 12/3/20 3:57 AM, Jim Fehlig wrote:
>> Attempting to create a domain with <seclabel type='none'/> results in
>>
>> virsh --connect lxc:/// create distro_nosec.xml
>> error: Failed to create domain from distro_nosec.xml
>> error: unsupported configuration: Security driver model '(null)' is not available
>>
>> With <seclabel type='none'/>, the model field of virSecurityLabelDef will
>> be NULL, causing virSecurityManagerCheckModel() to fail with the above
>> error. Avoid calling virSecurityManagerCheckModel() when they seclabel
>> type is VIR_DOMAIN_SECLABEL_NONE.
>>
>> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
>> ---
>>
>> This could also be fixed by checking for a NULL secmodel in
>> virSecurityManagerCheckModel, but it seems more appropriate to check for
>> a valid seclabel type before checking the model.
>>
>>   src/security/security_manager.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
>> index be81ee5e44..789e24d273 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
>> @@ -781,6 +781,9 @@ virSecurityManagerCheckDomainLabel(virSecurityManagerPtr mgr,
>>       size_t i;
>>       for (i = 0; i < def->nseclabels; i++) {
>> +        if (def->seclabels[i]->type == VIR_DOMAIN_SECLABEL_NONE)
>> +            continue;
>> +
>>           if (virSecurityManagerCheckModel(mgr, def->seclabels[i]->model) < 0)
>>               return -1;
>>       }
>>
> 
> This doesn't look right. If type='none' then ->model should contain "none" 
> string which in turn means virSecurityManagerCheckModel() is a NOP.
> 
> Looking into the code I've found the following in src/conf/domain_conf.c line 9239:
> 
>    /* Copy model from host. */
>    VIR_DEBUG("Found seclabel without a model, using '%s'",
>              xmlopt->config.defSecModel);
>    def->seclabels[0]->model = g_strdup(xmlopt->config.defSecModel);
> 
> and looking around and on git blame I found the following commit: 
> 638ffa222847acc74dd2d84d2088590ecbf8eb70 (let's not get into who reviewed it 
> O:-)) which made ->model initialization happen only if drvier initialized xmlopt 
> .defSecModel which is happening only in qemu driver and not LXC.

Ah, thanks for a pointer to the commit. That's a nice hint!

> Therefore I think we need something like this:
> 
> diff --git i/src/lxc/lxc_conf.c w/src/lxc/lxc_conf.c
> index 13da6c4586..fc26a0a38b 100644
> --- i/src/lxc/lxc_conf.c
> +++ w/src/lxc/lxc_conf.c
> @@ -212,6 +212,7 @@ virDomainXMLOptionPtr
>   lxcDomainXMLConfInit(virLXCDriverPtr driver)
>   {
>       virLXCDriverDomainDefParserConfig.priv = driver;
> +    virLXCDriverDomainDefParserConfig.defSecModel = "none";
>       return virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig,
>                                    &virLXCDriverPrivateDataCallbacks,
>                                    &virLXCDriverDomainXMLNamespace,
> 
> 
> But that makes lxcxml2xmltest fail, because the output XMLs don't contain the 
> model. I'm wondering if that's because we did not/do not pass caps with 
> host->nsecModels > 0 and thus this autofill wasn't run but with the change I'm 
> suggesting it is now.

I took a similar approach as the qemu driver and initialized the defSecModel 
from the driver's securityManager (ignoring stacked managers since afaict the 
lxc driver doesn't support it). With this approach lxcxml2xmltest passes and my 
issue is resolved :-).

https://www.redhat.com/archives/libvir-list/2020-December/msg00291.html

Regards,
Jim