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

[Jin Yan <jinyan12@xxxxxxxxxx>]

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