Bug 1179564

For reference, the <hostdev> portion of this was discovered during verification of the following bz:

https://bugzilla.redhat.com/show_bug.cgi?id=1082521

As "work-around" it is possible to provide both the <readonly/> and <shareable/> attributes.

Something I was hoping would be clearer when I asked for this bug to be filed is if the <disk...> case was also failing using the same libvirt release as used to test the other bug in the "failure" case.  That is - prior to libvirt-1.2.8-11.el7.x86_64 when the <shareable/> didn't work for <hostdev>, did setting <readonly/> on the <disk...> work? 

Nothing changed in the <disk...> code path and the <hostdev...> code path mimics the <disk...> code path, so if prior to this change the <disk...> wasn't working correctly then it's been broken for a while (or in IOW - bug for bug compatible).

Furthermore, just because <readonly/> was working for <hostdev...> prior to libvirt-1.2.8-11.el7.x86_64 doesn't necessarily mean it was working correctly since it would have used the same label (eg, secdev->imagelabel rather than data->content_context) as the non-readonly <hostdev...> which may have been wrong. I'm not "fluent" in speaking security labels and their proper usage.

If you can test something before libvirt-1.2.8-11.el7.x86_64 and even RHEL7.0 in order to determine if scenario 2 still fails and let me know what results you have - that would be useful.

The answer will help to determine whether this is something we must fix for RHEL 7.1 or whether it can wait for 7.2.

I did try the 7.0 test, but didn't take the time to convince the beaker system I got to create an iSCSI device for me to use.  My instructions work for Fedora, but RHEL uses targetcli, which I'm not that familiar with.

I think the root cause is this:

# sesearch --allow -s svirt_t -t virt_content_t
Found 7 semantic av rules:
   allow virt_domain file_type : dir { getattr search open } ; 
   allow virt_domain virt_content_t : file { ioctl read getattr lock open } ; 
   allow virt_domain virt_content_t : dir { ioctl read getattr lock search open } ; 
   allow virt_domain virt_content_t : lnk_file { read getattr } ; 
   allow virt_domain virt_content_t : blk_file { ioctl read getattr lock open } ; 
   allow virt_domain user_home_type : dir { getattr search open } ; 
   allow virt_domain user_home_type : lnk_file { read getattr } ; 

seliunx not allow chr_file read for virt_domain when chr_file label is virt_content_t.

And the AVC report is like this:

type=SYSCALL msg=audit(1420685468.653:169625): arch=c000003e syscall=2 success=no exit=-13 a0=7f5c675b0690 a1=80800 a2=0 a3=0 items=0 ppid=1 pid=3496 auid=4294967295 uid=107 gid=107 euid=107 suid=107 fsuid=107 egid=107 sgid=107 fsgid=107 tty=(none) ses=4294967295 comm="qemu-kvm" exe="/usr/libexec/qemu-kvm" subj=system_u:system_r:svirt_t:s0-s0:c286,c758 key=(null)
type=AVC msg=audit(1420685468.653:169626): avc:  denied  { read } for  pid=3496 comm="qemu-kvm" name="sg13" dev="devtmpfs" ino=128489628 scontext=system_u:system_r:svirt_t:s0-s0:c286,c758 tcontext=system_u:object_r:virt_content_t:s0 tclass=chr_file

So we will failed to start the guest because selinux forbid us.

if add this line in virt.te and rebuild the selinux and install, guest will start success.

--- a/virt.te
+++ b/virt.te
@@ -698,6 +698,7 @@ allow virt_domain self:netlink_kobject_u

 list_dirs_pattern(virt_domain, virt_content_t, virt_content_t)
 read_files_pattern(virt_domain, virt_content_t, virt_content_t)
+read_chr_files_pattern(virt_domain,virt_content_t,virt_content_t)
 dontaudit virt_domain virt_content_t:file write_file_perms;
 dontaudit virt_domain virt_content_t:dir write;

So I think maybe need move this bug to selinux, because libvirt do the right things here(set the virt_content_t to chr_file).

https://bugzilla.redhat.com/show_bug.cgi?id=1082521 c#16 says it is regression as "When I executed Scenario 2 with libvirt-1.2.8-10.el7.x86_64, the guest can be started successfully. But when I used libvirt-1.2.8-11.el7.x86_64 to test, the guest started to fail"

Add Regression keyword firstly.

With regard to comment 3 and the needinfo placed on me... If you are asking if I agree this should be moved, then the answer is yes as that seems reasonable to me and my very limited experience with selinux labels. But someone with more knowledge of selinux should be the arbiter of whether the fix is right.

Although I also note that min zhan moved this to 7.2; however, later on the regression keyword was added which seems to have added the blocker flag.

Again, I'll reiterate whether this is a libvirt regression is a matter of opinion. Also, IIRC the scsi_host hostdev was *added* for RHEL7.1 (eg, not in RHEL7.0). So it cannot be a regression from RHEL7.0. With respect to whether having <readonly/> fail for 7.1 is a regression for <disk...>'s still requires someone (see the needinfo from comment 2) to determine whether setting <readonly/> for a <disk...> in a RHEL7.0 environment works or not.

ug... seems my response cleared both needinfo flags - so I'm placing it back on submittor for resolution of comment 2.

(In reply to John Ferlan from comment #6)
> With regard to comment 3 and the needinfo placed on me... If you are asking
> if I agree this should be moved, then the answer is yes as that seems
> reasonable to me and my very limited experience with selinux labels. But
> someone with more knowledge of selinux should be the arbiter of whether the
> fix is right.
> 

Good.

# ls -Z /dev/sdf
brw-rw----. root disk system_u:object_r:fixed_disk_device_t:s0 /dev/sdf
# ls -Z /dev/sg7
crw-rw----. root disk system_u:object_r:scsi_generic_device_t:s0 /dev/sg7

/dev/sd* will work well because it is a blk_file and 
/dev/sg* will forbid use because it is a chr_file.

> Although I also note that min zhan moved this to 7.2; however, later on the
> regression keyword was added which seems to have added the blocker flag.
> 
> Again, I'll reiterate whether this is a libvirt regression is a matter of
> opinion. Also, IIRC the scsi_host hostdev was *added* for RHEL7.1 (eg, not
> in RHEL7.0). So it cannot be a regression from RHEL7.0. With respect to
> whether having <readonly/> fail for 7.1 is a regression for <disk...>'s
> still requires someone (see the needinfo from comment 2) to determine
> whether setting <readonly/> for a <disk...> in a RHEL7.0 environment works
> or not.

Maybe i can offer some information, because when i debug this issue, i had test these.

<readonly/> work well with <disk> for 7.0.z and 7.1 :

Steps:
1.# lsscsi -sg
[0:0:0:0]    disk    ATA      WDC WD10EZEX-60Z 80.0  /dev/sda   /dev/sg0   1.00TB
[2:0:0:0]    cd/dvd  hp       DVD-RAM GHA3N    RH06  /dev/sr0   /dev/sg1        -
[11:0:0:0]   disk    LIO-ORG  shareddata       4.0   /dev/sdb   /dev/sg2    104MB
[11:0:0:1]   disk    LIO-ORG  file1            4.0   /dev/sdc   /dev/sg3    209MB
[11:0:0:2]   disk    LIO-ORG  file2            4.0   /dev/sde   /dev/sg5    209MB
[12:0:0:0]   disk    LIO-ORG  shareddata       4.0   /dev/sdd   /dev/sg4    104MB
[12:0:0:1]   disk    LIO-ORG  file1            4.0   /dev/sdg   /dev/sg7    209MB

2.# rpm -q libvirt
libvirt-1.2.8-12.el7.x86_64

3. # virsh dumpxml test3

    <disk type='block' device='disk'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/sdg'/>
      <target dev='sdb' bus='scsi'/>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='1'/>
    </disk>

4.# virsh start test3
Domain test3 started

# ls -Z /dev/sdg
brw-rw----. qemu qemu system_u:object_r:virt_content_t:s0 /dev/sdg

4.# sesearch --allow -s svirt_t -t virt_content_t
Found 7 semantic av rules:
   allow virt_domain file_type : dir { getattr search open } ; 
   allow virt_domain virt_content_t : file { ioctl read getattr lock open } ; 
   allow virt_domain virt_content_t : dir { ioctl read getattr lock search open } ; 
   allow virt_domain virt_content_t : lnk_file { read getattr } ; 
   allow virt_domain virt_content_t : blk_file { ioctl read getattr lock open } ; 
   allow virt_domain user_home_type : dir { getattr search open } ; 
   allow virt_domain user_home_type : lnk_file { read getattr } ; 

5. also test with libvirt-1.1.1-29.el7_0.4.x86_64.

Interesting a bit different than scenario2 from the description section...

Description has:

    <disk type='block' device='disk'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/sg11'/>
      <target dev='sdb' bus='scsi'/>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='1'/>
    </disk>

and fails

Comment 8 has:

    <disk type='block' device='disk'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/sdg'/>
      <target dev='sdb' bus='scsi'/>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='1'/>
    </disk>

and succeeds.  

Based on your 'block' vs. 'char' I understand why and perhaps the Description should have used '/dev/sdf' instead of '/dev/sg11' in order to make things work.

Still points at selinux needing the 'chr_file' and thus not a libvirt issue.

(In reply to John Ferlan from comment #9)
> Interesting a bit different than scenario2 from the description section...
> 
> Description has:
> 
>     <disk type='block' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source dev='/dev/sg11'/>
>       <target dev='sdb' bus='scsi'/>
>       <readonly/>
>       <address type='drive' controller='0' bus='0' target='0' unit='1'/>
>     </disk>
> 
> and fails
> 
> Comment 8 has:
> 
>     <disk type='block' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source dev='/dev/sdg'/>
>       <target dev='sdb' bus='scsi'/>
>       <readonly/>
>       <address type='drive' controller='0' bus='0' target='0' unit='1'/>
>     </disk>
> 
> and succeeds.  
> 
> Based on your 'block' vs. 'char' I understand why and perhaps the
> Description should have used '/dev/sdf' instead of '/dev/sg11' in order to
> make things work.
> 
> Still points at selinux needing the 'chr_file' and thus not a libvirt issue.

Yes, i guess this is root cause.

BTW we have supported this feature <hostdev...> in 7.0, and please see this bug
(Thanks dyuan pointing out this, i don't know much about storage)

https://bugzilla.redhat.com/show_bug.cgi?id=957292#c8

(In reply to John Ferlan from comment #2)
> For reference, the <hostdev> portion of this was discovered during
> verification of the following bz:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1082521
> 
> As "work-around" it is possible to provide both the <readonly/> and
> <shareable/> attributes.
> 
> Something I was hoping would be clearer when I asked for this bug to be
> filed is if the <disk...> case was also failing using the same libvirt
> release as used to test the other bug in the "failure" case.  That is -
> prior to libvirt-1.2.8-11.el7.x86_64 when the <shareable/> didn't work for
> <hostdev>, did setting <readonly/> on the <disk...> work? 
> 
> Nothing changed in the <disk...> code path and the <hostdev...> code path
> mimics the <disk...> code path, so if prior to this change the <disk...>
> wasn't working correctly then it's been broken for a while (or in IOW - bug
> for bug compatible).
> 
> Furthermore, just because <readonly/> was working for <hostdev...> prior to
> libvirt-1.2.8-11.el7.x86_64 doesn't necessarily mean it was working
> correctly since it would have used the same label (eg, secdev->imagelabel
> rather than data->content_context) as the non-readonly <hostdev...> which
> may have been wrong. I'm not "fluent" in speaking security labels and their
> proper usage.
> 
> If you can test something before libvirt-1.2.8-11.el7.x86_64 and even
> RHEL7.0 in order to determine if scenario 2 still fails and let me know what
> results you have - that would be useful.
> 
> The answer will help to determine whether this is something we must fix for
> RHEL 7.1 or whether it can wait for 7.2.
> 

Sorry, I made a mistake.  Just as comment #8, the scenario 2 should use the following xml.

    <disk type='block' device='disk'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/sdb'/>                    <== Not /dev/sg* 
      <target dev='sdb' bus='scsi'/>
      <readonly/>
      <address type='drive' controller='0' bus='0' target='0' unit='1'/>
    </disk>


I tested this scenario with libvirt-1.2.8-10.el7.x86_64, libvirt-1.2.8-11.el7.x86_64 and libvirt-1.2.8-12.el7.x86_64. The results were the same as comment #8. <readonly/> work well with <disk> for rhel 7.1.

So is the "process" to create a new bug filed against selinux to request the fix suggested in comment 3?  And then make this bug depend on that?  Or does this bug get "reassigned" to selinux?  

If it's needed for 7.1, then it should be done soon as that window is closing fast.

(In reply to John Ferlan from comment #12)
> So is the "process" to create a new bug filed against selinux to request the
> fix suggested in comment 3?  And then make this bug depend on that?  Or does
> this bug get "reassigned" to selinux?  
> 
> If it's needed for 7.1, then it should be done soon as that window is
> closing fast.

I guess maybe reassigned to selinux then set depends on bug 1082521 is better.
Because libvirt already give enough patch for this issue (in bug 1082521), we just need selinux allow this 'chr_file' with this label 'virt_content_t'.

OK - So hopefully I did the right steps and reassigned this to the right component (selinux_policy) per the ongoing discussion. 

The important part of the history here is comment #3.  I removed the 7.2 flag and returned the 7.1 flag since it's verified that this is a regression from 7.0 for a similar configuration and a blocker flag was already added.

Could you tell me why you assigned this back to libvirt?

I apologize, we need to update SELinux rules.

commit fe7a92cba311ef0c9c8fddafb1d98089bb764224
Author: Miroslav Grepl <mgrepl>
Date:   Tue Jan 20 19:14:46 2015 +0100

    Update virt_read_content() interface to allow read also char devices.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-0458.html