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

Jin Yan posted 1 patch 3 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201112140707.164131-1-jinyan12@huawei.com
src/qemu/qemu_security.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
[PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Jin Yan 3 years, 4 months ago
When migration fails, qemuMigrationDstPrepareAny will call qemuProcessStop
to restore labels only after all labels are successfully set. If some labels
fail to set, the labels that have been set will not be restored.

Signed-off-by: Jin Yan <jinyan12@huawei.com>
---
 src/qemu/qemu_security.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 3bda96272c..0cb90c840a 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -51,16 +51,24 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
                                       incomingPath,
                                       priv->chardevStdioLogd,
                                       migrated) < 0)
-        goto cleanup;
+        goto restorelabel;
 
     if (virSecurityManagerTransactionCommit(driver->securityManager,
                                             pid, priv->rememberOwner) < 0)
-        goto cleanup;
+        goto restorelabel;
 
     ret = 0;
+
  cleanup:
     virSecurityManagerTransactionAbort(driver->securityManager);
     return ret;
+
+ restorelabel:
+    virSecurityManagerRestoreAllLabel(driver->securityManager,
+                                      vm->def,
+                                      migrated,
+                                      priv->chardevStdioLogd);
+    goto cleanup;
 }
 
 
-- 
2.23.0


Re: [PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Michal Privoznik 3 years, 4 months ago
On 11/12/20 3:07 PM, Jin Yan wrote:
> When migration fails, qemuMigrationDstPrepareAny will call qemuProcessStop
> to restore labels only after all labels are successfully set. If some labels
> fail to set, the labels that have been set will not be restored.
> 
> Signed-off-by: Jin Yan <jinyan12@huawei.com>
> ---
>   src/qemu/qemu_security.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 3bda96272c..0cb90c840a 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -51,16 +51,24 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>                                         incomingPath,
>                                         priv->chardevStdioLogd,
>                                         migrated) < 0)
> -        goto cleanup;
> +        goto restorelabel;
>   
>       if (virSecurityManagerTransactionCommit(driver->securityManager,
>                                               pid, priv->rememberOwner) < 0)
> -        goto cleanup;
> +        goto restorelabel;
>   
>       ret = 0;
> +
>    cleanup:
>       virSecurityManagerTransactionAbort(driver->securityManager);
>       return ret;
> +
> + restorelabel:
> +    virSecurityManagerRestoreAllLabel(driver->securityManager,
> +                                      vm->def,
> +                                      migrated,
> +                                      priv->chardevStdioLogd);
> +    goto cleanup;
>   }
>   
>   
> 

I don't think this is correct. Firstly, this restores labels for ALL 
paths, and not just the failed ones (which messes up seclabel 
remembering and its refcounting), but more importantly:

1) rollback within one secdriver is handled in .transactionCommit 
callback, well virSecurity*TransactionRun() actually,

2) rollback for other secdrivers after one failed is handled in 
virSecurityStackSetAllLabel().


Is this not working properly? What version do you run?

Michal

Re: [PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Jin Yan 3 years, 4 months ago
On 2020/11/13 2:54, Michal Privoznik wrote:
> On 11/12/20 3:07 PM, Jin Yan wrote:
>> When migration fails, qemuMigrationDstPrepareAny will call qemuProcessStop
>> to restore labels only after all labels are successfully set. If some labels
>> fail to set, the labels that have been set will not be restored.
>>
>> Signed-off-by: Jin Yan <jinyan12@huawei.com>
>> ---
>>    src/qemu/qemu_security.c | 12 ++++++++++--
>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 3bda96272c..0cb90c840a 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -51,16 +51,24 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>>                                          incomingPath,
>>                                          priv->chardevStdioLogd,
>>                                          migrated) < 0)
>> -        goto cleanup;
>> +        goto restorelabel;
>>    
>>        if (virSecurityManagerTransactionCommit(driver->securityManager,
>>                                                pid, priv->rememberOwner) < 0)
>> -        goto cleanup;
>> +        goto restorelabel;
>>    
>>        ret = 0;
>> +
>>     cleanup:
>>        virSecurityManagerTransactionAbort(driver->securityManager);
>>        return ret;
>> +
>> + restorelabel:
>> +    virSecurityManagerRestoreAllLabel(driver->securityManager,
>> +                                      vm->def,
>> +                                      migrated,
>> +                                      priv->chardevStdioLogd);
>> +    goto cleanup;
>>    }
>>    
>>    
>>
> I don't think this is correct. Firstly, this restores labels for ALL
> paths, and not just the failed ones (which messes up seclabel
> remembering and its refcounting), but more importantly:
>
> 1) rollback within one secdriver is handled in .transactionCommit
> callback, well virSecurity*TransactionRun() actually,
>
> 2) rollback for other secdrivers after one failed is handled in
> virSecurityStackSetAllLabel().
>
>
> Is this not working properly? What version do you run?

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.

Re: [PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Michal Privoznik 3 years, 4 months ago
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

Re: [PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Jin Yan 3 years, 4 months ago
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>

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 ...

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

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?

Yan

Re: [PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Michal Privoznik 3 years, 4 months ago
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

Re: [PATCH v2] selinux label: restore all labels when some labels fail to set
Posted by Jin Yan 3 years, 4 months ago
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