Bug 2209191

Workaround:

# mkdir -p /var/log/passt
# chown 107:107 /var/log/passt

# virsh dumpxml guest | grep logFile
      <backend type='passt' logFile='/var/log/passt/guest.log'/>

# virsh start guest
Domain 'guest' started

I have a patch for libvirt, but first, we need to extend passt capabilities a bit:

https://archives.passt.top/passt-dev/cover.1686037337.git.mprivozn@redhat.com/T/#t

Well, that didn't go as planned. But that's okay, I can work around it in libvirt:

https://listman.redhat.com/archives/libvir-list/2023-June/240279.html

Merged upstream as:

ceb4dc8e17 docs: Move passt log file in our example XML
8511b96a31 qemu_passt: Precreate passt logfile

v9.4.0-80-gceb4dc8e17

Before this patch/bug, with libvirt libvirt-9.4.0-1.el9.x86_64 or older, passt's SELinux policies limit its writing of log files to /run/user/<UID>/filename.log, where <UID> is the uid used to run the passt process.

So for root user, 
1) prepare the log directory for qemu user with uid as 107:
# mkdir /run/user/107
# chown qemu:qemu /run/user/107

2) configure the interface xml as below:
# virsh dumpxml rhel --xpath //interface 
<interface type="user">
  <mac address="52:54:00:49:84:12"/>
  <source dev="eno1"/>
  <model type="virtio"/>
  <backend type="passt" logFile="/run/user/107/passt.log"/>
</interface>

# virsh start rhel 
Domain 'rhel' started

# ll -Z /run/user/107/passt.log 
-rw-------. 1 qemu qemu system_u:object_r:user_tmp_t:s0 1738 Jun 29 23:26 /run/user/107/passt.log

For unpreviledged user, for example "test":
1) swith to "test" user:
# machinectl shell test@

Check the uid of this unpreviledged user:
$ id
uid=1000(test) gid=1000(test) groups=1000(test) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023

Check the permission of the XDG_RUNTIME_DIR for this user:
$ ll -d -Z  /run/user/1000
drwx------. 5 test test unconfined_u:object_r:user_tmp_t:s0 140 Jun 29 23:31 /run/user/1000

2) configure vm interface like as below:
$ virsh dumpxml test1 --xpath //interface 
<interface type="user">
  <mac address="52:54:00:26:44:e5"/>
  <source dev="eno1"/>
  <model type="virtio"/>
  <backend type="passt" logFile="/run/user/1000/passt.log"/>
  <address type="pci" domain="0x0000" bus="0x01" slot="0x00" function="0x0"/>
</interface>

$ virsh start test1
Domain 'test1' started

$ ll -Z /run/user/1000/passt.log
-rw-------. 1 test test unconfined_u:object_r:user_tmp_t:s0 1369 Jun 29 23:40 /run/user/1000/passt.log

Hi Michal,Laine, Could you please help to check the results with libvirt-9.5.0-0rc1.1.el9.x86_64?

1) With root user and the same settings in comment 5, the vm can not start successfully; 
No impact for unpriviledged user with settings in commet 5. Is this expected?

2) For root user, VM can not start successfully with I set log file to /tmp. Do we need to update passt as well?

3) For unpriviledged user, could it use /tmp as a log directory? 

4) Should the "/run/user/<UID>/filename.log" still be valid configuration both for root and non-root user?

Details:
#  rpm -q libvirt passt passt-selinux
libvirt-9.5.0-0rc1.1.el9.x86_64
passt-0^20230222.g4ddbcb9-4.el9.x86_64
passt-selinux-0^20230222.g4ddbcb9-4.el9.noarch

S1: root user with same settings in comment 5:
# ll -Z -d  /run/user/107/
drwxr-xr-x. 2 qemu qemu unconfined_u:object_r:user_tmp_t:s0 40 Jun 29 23:51 /run/user/107/

# virsh dumpxml rhel --xpath //interface 
<interface type="user">
  <mac address="52:54:00:49:84:12"/>
  <source dev="eno1"/>
  <model type="virtio"/>
  <backend type="passt" logFile="/run/user/107/passt.log"/>
  <address type="pci" domain="0x0000" bus="0x01" slot="0x00" function="0x0"/>
</interface>

# virsh start rhel 
error: Failed to start domain 'rhel'
error: internal error: Child process (/usr/bin/passt --one-off --socket /run/libvirt/qemu/passt/1-rhel-net0.socket --mac-addr 52:54:00:49:84:12 --pid /run/libvirt/qemu/passt/1-rhel-net0-passt.pid --interface eno1 --log-file /run/user/107/passt.log) unexpected exit status 1: Couldn't open log file /run/user/107/passt.log: Permission denied


S2: root user, with log file in /tmp(this bug fix)
# virsh dumpxml rhel --xpath //interface 
<interface type="user">
  <mac address="52:54:00:49:84:12"/>
  <source dev="eno1"/>
  <model type="virtio"/>
  <backend type="passt" logFile="/tmp/passt.log"/>
  <address type="pci" domain="0x0000" bus="0x01" slot="0x00" function="0x0"/>
</interface>

# getenforce 
Enforcing

# virsh start rhel 
error: Failed to start domain 'rhel'
error: internal error: Child process (/usr/bin/passt --one-off --socket /run/libvirt/qemu/passt/1-rhel-net0.socket --mac-addr 52:54:00:49:84:12 --pid /run/libvirt/qemu/passt/1-rhel-net0-passt.pid --interface eno1 --log-file /tmp/passt.log) unexpected exit status 1: Couldn't open log file /tmp/passt.log: Permission denied

# ll -Z -d /tmp
drwxrwxrwt. 10 root root system_u:object_r:tmp_t:s0 4096 Jun 29 23:56 /tmp

(In reply to yalzhang from comment #6) 
> 1) With root user and the same settings in comment 5, the vm can not start
> successfully; 
> No impact for unpriviledged user with settings in commet 5. Is this expected?
> 
Correct the 2nd sentence: both for user and non-root user, the vm can not start successfully.

Alright, after I reproduced the issue I see the following message in audit.log:

type=AVC msg=audit(1689149172.232:406): avc:  denied  { read append } for  pid=4898 comm="passt.avx2" name="passt.log" dev="tmpfs" ino=1577 scontext=system_u:system_r:passt_t:s0:c158,c647 tcontext=system_u:object_r:svirt_image_t:s0:c158,c647 tclass=file permissive=0

Basically, the SELinux policy denies passt_t from accessing svirt_image_t. Now, the question is how to fix this issue. We have two options:

1) Have libvirt create the file and just set uid:gid and don't touch SELinux label at all (hoping that passt_t is generous enough to allow open+append to the most used locations (like /run/user/...)), or

2) fix passt-selinux policy an allow passt_t to open+append to svirt_image_t.

Stefano, since you are maintainer of passt, which one would you pick?

(In reply to Michal Privoznik from comment #8)
> Alright, after I reproduced the issue I see the following message in
> audit.log:
> 
> type=AVC msg=audit(1689149172.232:406): avc:  denied  { read append } for 
> pid=4898 comm="passt.avx2" name="passt.log" dev="tmpfs" ino=1577
> scontext=system_u:system_r:passt_t:s0:c158,c647
> tcontext=system_u:object_r:svirt_image_t:s0:c158,c647 tclass=file
> permissive=0
> 
> Basically, the SELinux policy denies passt_t from accessing svirt_image_t.
> Now, the question is how to fix this issue. We have two options:
> 
> 1) Have libvirt create the file and just set uid:gid and don't touch SELinux
> label at all (hoping that passt_t is generous enough to allow open+append to
> the most used locations (like /run/user/...)), or
> 
> 2) fix passt-selinux policy an allow passt_t to open+append to svirt_image_t.
> 
> Stefano, since you are maintainer of passt, which one would you pick?

Stefano is on holiday this week, I'm handling most passt & pasta stuff.  Unfortunately, I'm pretty ignorant on SELinux.  Here's some questions based on what I can figure out so far.

a) passt's own selinux files define a passt_log_t type, which seems to be what we want.  Is there a reason libvirt can't label the file passt_log_t?

b) What other things have the svirt_image_t label?  Does it include VM images, as the name suggests?  If so passt really shouldn't have permission to that - it has no business accessing VM images.

(In reply to David Gibson from comment #9)
> (In reply to Michal Privoznik from comment #8)
> > Alright, after I reproduced the issue I see the following message in
> > audit.log:
> > 
> > type=AVC msg=audit(1689149172.232:406): avc:  denied  { read append } for 
> > pid=4898 comm="passt.avx2" name="passt.log" dev="tmpfs" ino=1577
> > scontext=system_u:system_r:passt_t:s0:c158,c647
> > tcontext=system_u:object_r:svirt_image_t:s0:c158,c647 tclass=file
> > permissive=0
> > 
> > Basically, the SELinux policy denies passt_t from accessing svirt_image_t.
> > Now, the question is how to fix this issue. We have two options:
> > 
> > 1) Have libvirt create the file and just set uid:gid and don't touch SELinux
> > label at all (hoping that passt_t is generous enough to allow open+append to
> > the most used locations (like /run/user/...)), or
> > 
> > 2) fix passt-selinux policy an allow passt_t to open+append to svirt_image_t.
> > 
> > Stefano, since you are maintainer of passt, which one would you pick?
> 
> Stefano is on holiday this week, I'm handling most passt & pasta stuff. 
> Unfortunately, I'm pretty ignorant on SELinux.  Here's some questions based
> on what I can figure out so far.
> 
> a) passt's own selinux files define a passt_log_t type, which seems to be
> what we want.  Is there a reason libvirt can't label the file passt_log_t?

Problem with that is missing SELinux rule that would allow virtd_t to set pass_t label:

type=AVC msg=audit(1689329031.294:1123): avc:  denied  { relabelto } for  pid=148603 comm="rpc-virtqemud" name="passt.log" dev="dm-0" ino=134350295 scontext=system_u:system_r:virtd_t:s0-s0:c0.c1023 tcontext=system_u:system_r:passt_t:s0:c425,c467 tclass=file permissive=0

At any rate, that's something that passt-selinux policy must fix.

> 
> b) What other things have the svirt_image_t label?  Does it include VM
> images, as the name suggests?  If so passt really shouldn't have permission
> to that - it has no business accessing VM images.

Yeah, that might be too wide. Let's discard this then.

Looks like the initial changes weren't enough to fix this, re-opening.

Sorry for the delay, and sorry for not actually having an answer at the moment, rather some questions:

(In reply to Michal Privoznik from comment #8)
> Alright, after I reproduced the issue I see the following message in
> audit.log:
> 
> type=AVC msg=audit(1689149172.232:406): avc:  denied  { read append } for 
> pid=4898 comm="passt.avx2" name="passt.log" dev="tmpfs" ino=1577
> scontext=system_u:system_r:passt_t:s0:c158,c647
> tcontext=system_u:object_r:svirt_image_t:s0:c158,c647 tclass=file
> permissive=0
> 
> Basically, the SELinux policy denies passt_t from accessing svirt_image_t.
> Now, the question is how to fix this issue. We have two options:
> 
> 1) Have libvirt create the file and just set uid:gid and don't touch SELinux
> label at all (hoping that passt_t is generous enough to allow open+append to
> the most used locations (like /run/user/...)), or

Isn't this what's already happening, but the pre-created file has a svirt_image_t label, and passt_t can't append to it?

(In reply to Michal Privoznik from comment #10)
>
> (In reply to David Gibson from comment #9)
>
> > a) passt's own selinux files define a passt_log_t type, which seems to be
> > what we want.  Is there a reason libvirt can't label the file passt_log_t?
> 
> Problem with that is missing SELinux rule that would allow virtd_t to set
> pass_t label:
> 
> type=AVC msg=audit(1689329031.294:1123): avc:  denied  { relabelto } for 
> pid=148603 comm="rpc-virtqemud" name="passt.log" dev="dm-0" ino=134350295
> scontext=system_u:system_r:virtd_t:s0-s0:c0.c1023
> tcontext=system_u:system_r:passt_t:s0:c425,c467 tclass=file permissive=0

Here we would need to relabel the file to passt_log_t (not passt_t), but I guess that doesn't change things.

> At any rate, that's something that passt-selinux policy must fix.

...is there any other label that libvirt could use when it pre-creates the file, so that we can update the passt-selinux policy to include appending and reading for that label?

Or do you have something else in mind as to how the passt-selinux policy could fix this?

At the moment the only direction I can think of is something on the lines of:

    https://github.com/fedora-selinux/selinux-policy/pull/1631/commits/2f450e6425a44a90594a6183ee40164e5cf60c68

but applied to libvirt's own (kind of) log files.

(In reply to Stefano Brivio from comment #15)
> Sorry for the delay, and sorry for not actually having an answer at the
> moment, rather some questions:
> 
> (In reply to Michal Privoznik from comment #8)
> > Alright, after I reproduced the issue I see the following message in
> > audit.log:
> > 
> > type=AVC msg=audit(1689149172.232:406): avc:  denied  { read append } for 
> > pid=4898 comm="passt.avx2" name="passt.log" dev="tmpfs" ino=1577
> > scontext=system_u:system_r:passt_t:s0:c158,c647
> > tcontext=system_u:object_r:svirt_image_t:s0:c158,c647 tclass=file
> > permissive=0
> > 
> > Basically, the SELinux policy denies passt_t from accessing svirt_image_t.
> > Now, the question is how to fix this issue. We have two options:
> > 
> > 1) Have libvirt create the file and just set uid:gid and don't touch SELinux
> > label at all (hoping that passt_t is generous enough to allow open+append to
> > the most used locations (like /run/user/...)), or
> 
> Isn't this what's already happening, but the pre-created file has a
> svirt_image_t label, and passt_t can't append to it?

More or less yes.

> 
> (In reply to Michal Privoznik from comment #10)
> >
> > (In reply to David Gibson from comment #9)
> >
> > > a) passt's own selinux files define a passt_log_t type, which seems to be
> > > what we want.  Is there a reason libvirt can't label the file passt_log_t?
> > 
> > Problem with that is missing SELinux rule that would allow virtd_t to set
> > pass_t label:
> > 
> > type=AVC msg=audit(1689329031.294:1123): avc:  denied  { relabelto } for 
> > pid=148603 comm="rpc-virtqemud" name="passt.log" dev="dm-0" ino=134350295
> > scontext=system_u:system_r:virtd_t:s0-s0:c0.c1023
> > tcontext=system_u:system_r:passt_t:s0:c425,c467 tclass=file permissive=0
> 
> Here we would need to relabel the file to passt_log_t (not passt_t), but I
> guess that doesn't change things.

Question is, how to get passt_log_t from SELinux policy. Libvirt uses combination of getfilecon("/usr/bin/passt", .. ) + security_compute_create() to compute the seclabel under which passt is going to run. I'm not exactly sure how to get to passt_log_t from there. Here's the upstream code:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_selinux.c#L565

I have some patches on my local branch that reuse this code to compute SELinux label for the log file, but apparently, they end up with passt_t.

> 
> > At any rate, that's something that passt-selinux policy must fix.
> 
> ...is there any other label that libvirt could use when it pre-creates the
> file, so that we can update the passt-selinux policy to include appending
> and reading for that label?

No, I don't think there's a label we could use for this. I mean, libvirt uses basically two labels for files:

system_u:object_r:svirt_image_t:s0 for allowing RW access to a file, and
system_u:object_r:virt_content_t:s0 for allowing RO access to a file.

(label are then augmented to be unique across the host by adding range)

> 
> Or do you have something else in mind as to how the passt-selinux policy
> could fix this?
> 
> At the moment the only direction I can think of is something on the lines of:
> 
>    
> https://github.com/fedora-selinux/selinux-policy/pull/1631/commits/
> 2f450e6425a44a90594a6183ee40164e5cf60c68
> 
> but applied to libvirt's own (kind of) log files.

If libvirt could somehow guess the passt_log_t then we would just need to allow the relabeling.
But at this point I start to wonder, whether this has gone too far. I mean, log file is certainly helpful but what gain there is in allowing it to exist just anywhere? Especially since we want passt to run restricted.

Since this is far from getting fixed properly, let's do the best effort approach: stop libvirt creating the file, and let passt create it once again. This has higher chance of succeeding as passt SELinux policy already handles that.
Then again, this is so specific corner case that I think is not worth investing our time into. Having domain specific log file in such generic location as /var/log/ is really unfavorable and log files in general are good for libvirt/passt developers.

Therefore I've proposed revert of one of the patches from comment 4 (turning this basically into a documentation bug):

https://listman.redhat.com/archives/libvir-list/2023-August/241060.html

Test with libvirt-9.5.0-5.el9.x86_64, the result is as the same with comment 5.
The log path need to be as /run/user/<UID>/

(In reply to yalzhang from comment #26)
> Test with libvirt-9.5.0-5.el9.x86_64, the result is as the same with comment
> 5.
> The log path need to be as /run/user/<UID>/

What UID value should be passed here? I mean, it certainly can't be the UID of the logged in user, because QEMU runs under different user (by default). But that user is not logged in, thus it doesn't have /run/user/$UID. @sbrivio I believe this is controlled by passt's selinux policy. What is the recommended log location?

First off, thanks for taking care of this -- I wanted to try out some other workarounds, but a number of (blocking) issues with SELinux interactions showed up on Fedora Rawhide meanwhile, and I just finished sorting them.

For the record, the most promising thing I found was letting libvirt not just pre-create the file, but also label it as passt_log_t. It's remarkably ugly and just works as root. So I much prefer the way you went.

An alternative I would find acceptable, I guess I mentioned it in the past, would be to take a relative path there, and use a safe, pre-defined location to write log files depending on the user. But this is inconsistent with other services handled by libvirt I guess. Excluding this, that would probably be my favourite option in terms of usability.

(In reply to Michal Privoznik from comment #27)
> (In reply to yalzhang from comment #26)
> > Test with libvirt-9.5.0-5.el9.x86_64, the result is as the same with comment
> > 5.
> > The log path need to be as /run/user/<UID>/
> 
> What UID value should be passed here? I mean, it certainly can't be the UID
> of the logged in user, because QEMU runs under different user (by default).

If you run libvirt as unprivileged user, QEMU and passt will run under that user, so it can actually be that user (that's how I actually... use it).

> But that user is not logged in, thus it doesn't have /run/user/$UID.

If you run it as root, then yes, it's qemu (for both, passt and QEMU). But... why do you say it doesn't exist? Comment #5 actually shows it does. I think it's also a natural choice -- passt runs under that user.

> @sbrivio I believe this is controlled by passt's selinux policy.

Kind of -- passt-selinux exports a 'passt_logfile_use' interface, but it's not currently used by libvirt's policy. However:

> What is the recommended log location?

passt can write and append to files under /run/user/$UID (which I would recommend for unprivileged users), and under /tmp (which can be used if started as root, if you find /run/user/107 confusing -- I don't really have a strong preference between these two).

(In reply to Stefano Brivio from comment #28)
> (In reply to Michal Privoznik from comment #27)
> > (In reply to yalzhang from comment #26)
> > > Test with libvirt-9.5.0-5.el9.x86_64, the result is as the same with comment
> > > 5.
> > > The log path need to be as /run/user/<UID>/
> > 
> > What UID value should be passed here? I mean, it certainly can't be the UID
> > of the logged in user, because QEMU runs under different user (by default).
> 
> If you run libvirt as unprivileged user, QEMU and passt will run under that
> user, so it can actually be that user (that's how I actually... use it).
> 
> > But that user is not logged in, thus it doesn't have /run/user/$UID.
> 
> If you run it as root, then yes, it's qemu (for both, passt and QEMU).
> But... why do you say it doesn't exist? Comment #5 actually shows it does. I
> think it's also a natural choice -- passt runs under that user.

I guess Michal means this "/run/user/107" directory does not exists and needs to be created manually: 
For root user, we need to create this directory and modify the permissions: 
1) prepare the log directory for qemu user with uid as 107:
# mkdir /run/user/107
# chown qemu:qemu /run/user/107

> > @sbrivio I believe this is controlled by passt's selinux policy.
> 
> Kind of -- passt-selinux exports a 'passt_logfile_use' interface, but it's
> not currently used by libvirt's policy. However:
> 
> > What is the recommended log location?
> 
> passt can write and append to files under /run/user/$UID (which I would
> recommend for unprivileged users), and under /tmp (which can be used if
> started as root, if you find /run/user/107 confusing -- I don't really have
> a strong preference between these two).

But with libvirt, when start vm as root, we can not set the log path to /tmp.
The audit log shows:
time->Wed Aug 23 04:22:25 2023
type=PROCTITLE msg=audit(1692778945.130:1322): proctitle=2F7573722F62696E2F7061737374002D2D6F6E652D6F6666002D2D736F636B6574002F72756E2F6C6962766972742F71656D752F70617373742F312D7268656C2D6E6574302E736F636B6574002D2D6D61632D616464720035323A35343A30303A37343A38383A3863002D2D706964002F72756E2F6C6962766972742F71656D
type=SYSCALL msg=audit(1692778945.130:1322): arch=c000003e syscall=257 success=no exit=-13 a0=ffffff9c a1=7ffda7a8bea5 a2=80642 a3=180 items=0 ppid=22515 pid=22587 auid=4294967295 uid=107 gid=107 euid=107 suid=107 fsuid=107 egid=107 sgid=107 fsgid=107 tty=(none) ses=4294967295 comm="passt.avx2" exe="/usr/bin/passt.avx2" subj=system_u:system_r:passt_t:s0:c201,c495 key=(null)
type=AVC msg=audit(1692778945.130:1322): avc:  denied  { create } for  pid=22587 comm="passt.avx2" name="passt.log" scontext=system_u:system_r:passt_t:s0:c201,c495 tcontext=system_u:object_r:tmp_t:s0 tclass=file permissive=0

(In reply to yalzhang from comment #29)
> But with libvirt, when start vm as root, we can not set the log path to /tmp.

Oh, sorry, I see now. That's because the 'user_tmp_t' SELinux type does *not* resolve to /tmp (which I had assumed, wrongly).

However, the current SELinux policy allows passt to write to the home directory of the user it was started as:

allow passt_t user_home_dir_t:dir { search add_name write };
allow passt_t user_home_dir_t:file { create open write };

so, for root, we could just recommend writing the log file to /root, I suppose?

(In reply to Stefano Brivio from comment #30)
> (In reply to yalzhang from comment #29)
> > But with libvirt, when start vm as root, we can not set the log path to /tmp.
> 
> Oh, sorry, I see now. That's because the 'user_tmp_t' SELinux type does
> *not* resolve to /tmp (which I had assumed, wrongly).
> 
> However, the current SELinux policy allows passt to write to the home
> directory of the user it was started as:
> 
> allow passt_t user_home_dir_t:dir { search add_name write };
> allow passt_t user_home_dir_t:file { create open write };
> 
> so, for root, we could just recommend writing the log file to /root, I
> suppose?

For root, when start vm with passt interface, the user of the passt process is qemu, not root. 
When set the log path as /root/passt.log, the vm can not start successfully.
When we start vm with root, we prepare the log like this:
# mkdir /run/user/107
# chown qemu:qemu /run/user/107
Then set the interface with log path as logFile="/run/user/107/passt.log":
# virsh dumpxml vm1 --xpath //interface 
<interface type="user">
  <mac address="52:54:00:ec:98:b6"/>
  <source dev="br0"/>
  <model type="virtio"/>
  <backend type="passt" logFile="/run/user/107/passt.log"/>
  <acpi index="1"/>
  <address type="pci" domain="0x0000" bus="0x07" slot="0x00" function="0x0"/>
</interface>
# virsh start vm1
Domain 'vm1' started
# ps aux | grep 'usr/bin/passt'
qemu       14863  0.0  0.0  76292 38560 ?        Ss   06:06   0:00 /usr/bin/passt --one-off --socket /run/libvirt/qemu/passt/15-vm1-net0.socket --mac-addr 52:54:00:ec:98:b6 --pid /run/libvirt/qemu/passt/15-vm1-net0-passt.pid --interface br0 --log-file /run/user/107/passt.log

I will create a bug to request for the document update.

File a bug to track the doc issue: https://issues.redhat.com/browse/RHEL-1833

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 (Moderate: libvirt security, bug fix, and enhancement update), 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://access.redhat.com/errata/RHSA-2023:6409