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

[Jin Yan]

On 2020/11/20 15:49, Michal Privoznik wrote:
> 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
>
I guess the reason you got the error is that your SELinux mode is enforcing
but mine is permissive. The two modes are processed differently in
virSecuritySELinuxSetFileconImpl().

>> 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?
>
If one secdriver success (maybe should fail?) and the next fail, which
function cotrols rollback? virSecurityManagerTransactionCommit() is
executed after virSecurityStackSetAllLabel(), but the rollback
（virSecurityManagerTransactionAbort) can't restore label.

>> 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)?

I can't use the latest release for the time being, but I compared the code
and think the problem may still exist.

Yan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201120/14203f57/attachment-0001.htm>