[PATCH v2] selinux label: restore all labels when some labels fail to set

[Michal Privoznik]

On 11/19/20 3:54 PM, Jin Yan wrote:
> 
> On 2020/11/13 22:33, Michal Privoznik wrote:
>> On 11/13/20 10:47 AM, Jin Yan wrote:
>>
>>> Hi Michal,
>>> I found this problem while performing migration, based on
>>>       libvirt version: 6.2.0
>>>       SELinux mode: permissive
>>>
>>> Steps:
>>> 1. start a vm configured with pipe-type serial port.
>>>       <serial type='pipe'>
>>>         <source path='/tmp/test_pipe'/>
>>>         <target type='system-serial' port='1'>
>>>           <model name='pl011'/>
>>>         </target>
>>>       </serial>
>>> 2. migrate vm to Dst-side where no '/tmp/test_pipe' exits.
>>> 3. migration failed in Dst-side qemuProcessLaunch, and the path's label
>>> that
>>> has been set is not restored ('/var/lib/libvirt/qemu/nvram/XXX.fd').
>>>
>>> I have no idea why 2)rollback you mentioned didn't work.
>>>
>>>
>> I'm not sure. I could not reproduce with the current master. Is it
>> possible for you to try the master?
>>
>> Michal
> 
> I think we can reproduce it in a more easier way, that is, starting a VM 
> whose XML is configured with a pipe file that does not exist on local host:
>      <serial type='pipe'>
>         <source path='/tmp/serial.pipe'/>
>         <target port='0'/>
>       </serial>

This is what I have in XML:

     <serial type='pipe'>
       <source path='/tmp/test_pipe'/>
       <target type='isa-serial' port='0'>
         <model name='isa-serial'/>
       </target>
     </serial>

and the file doesn't exist.

> 
> 1. Though '/tmp/serial.pipe' does not exist, this secdriver (if I'm not 
> mistaken about this concept) set SELinux-label return success, and the 
> marked items (eg. XXX.fd, XXX.iso) will not be rollback.
> 
> [call trace]:
> virSecuritySELinuxTransactionRun       -- return 0
>      virSecuritySELinuxSetFilecon           -- return 0
>          virSecuritySELinuxSetFileconImpl   -- return 1, warned unable 
> to ...
> 

I don't get a warning. I get a regular error:

error: unable to set security context 
'unconfined_u:object_r:svirt_image_t:s0:c339,c673' on '/tmp/test_pipe': 
No such file or directory


> 2. The next secdriver about setting DAC-label run in 
> virSecurityDACTransactionRun() return false because above file does not 
> exist.
> 

Oh I think get it now. So if one secdriver would fail but not the other 
then because of transactions the rollback would be ineffective?


> virSecurityManagerTransactionCommit() return false, but where is the 
> rollback performed for other secdrivers (here means setting 
> SELinux-label in 1) ? I don't quite understand the second point you 
> mentioned in your last reply:
>      ---
>      2) rollback for other secdrivers after one failed is handled in 
> virSecurityStackSetAllLabel().
>      ---
> 
> In addition, is there any wrong in virSecuritySELinuxTransactionRun 
> return success while '/tmp/serial.pipe' does not exist?

I guess that is the source of problem. Have you tried the latest release 
(or master)?

Michal