Hi,

This is V5 of the patches. I posted V4 here.

https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html

These will allow us to support SELinux with virtiofs. This will send
SELinux context at file creation to server and server can set it on
file.

Changes since V4
----------------
- Parse only known current size of fuse_init_in. This will make sure
  that future extension does not break existing code upon header
  update. (David Gilbert)

- Changed order of one of the patch. It is first patch in series. This
  will help fix the breakage before header update patch and code remains
  git bisectable. (David Gilbert)

- Changed %lx to %llx at one place. (David Gilbert).

Thanks
Vivek
 
Vivek Goyal (9):
  virtiofsd: Fix breakage due to fuse_init_in size change
  linux-headers: Update headers to v5.17-rc1
  virtiofsd: Parse extended "struct fuse_init_in"
  virtiofsd: Extend size of fuse_conn_info->capable and ->want fields
  virtiofsd, fuse_lowlevel.c: Add capability to parse security context
  virtiofsd: Move core file creation code in separate function
  virtiofsd: Create new file with fscreate set
  virtiofsd: Create new file using O_TMPFILE and set security context
  virtiofsd: Add an option to enable/disable security label

 docs/tools/virtiofsd.rst                      |   7 +
 include/standard-headers/asm-x86/kvm_para.h   |   1 +
 include/standard-headers/drm/drm_fourcc.h     |  11 +
 include/standard-headers/linux/ethtool.h      |   1 +
 include/standard-headers/linux/fuse.h         |  60 ++-
 include/standard-headers/linux/pci_regs.h     | 142 +++---
 include/standard-headers/linux/virtio_gpio.h  |  72 +++
 include/standard-headers/linux/virtio_i2c.h   |  47 ++
 include/standard-headers/linux/virtio_iommu.h |   8 +-
 .../standard-headers/linux/virtio_pcidev.h    |  65 +++
 include/standard-headers/linux/virtio_scmi.h  |  24 +
 linux-headers/asm-generic/unistd.h            |   5 +-
 linux-headers/asm-mips/unistd_n32.h           |   2 +
 linux-headers/asm-mips/unistd_n64.h           |   2 +
 linux-headers/asm-mips/unistd_o32.h           |   2 +
 linux-headers/asm-powerpc/unistd_32.h         |   2 +
 linux-headers/asm-powerpc/unistd_64.h         |   2 +
 linux-headers/asm-riscv/bitsperlong.h         |  14 +
 linux-headers/asm-riscv/mman.h                |   1 +
 linux-headers/asm-riscv/unistd.h              |  44 ++
 linux-headers/asm-s390/unistd_32.h            |   2 +
 linux-headers/asm-s390/unistd_64.h            |   2 +
 linux-headers/asm-x86/kvm.h                   |  16 +-
 linux-headers/asm-x86/unistd_32.h             |   1 +
 linux-headers/asm-x86/unistd_64.h             |   1 +
 linux-headers/asm-x86/unistd_x32.h            |   1 +
 linux-headers/linux/kvm.h                     |  17 +
 tools/virtiofsd/fuse_common.h                 |   9 +-
 tools/virtiofsd/fuse_i.h                      |   7 +
 tools/virtiofsd/fuse_lowlevel.c               | 162 +++++--
 tools/virtiofsd/helper.c                      |   1 +
 tools/virtiofsd/passthrough_ll.c              | 414 ++++++++++++++++--
 32 files changed, 1013 insertions(+), 132 deletions(-)
 create mode 100644 include/standard-headers/linux/virtio_gpio.h
 create mode 100644 include/standard-headers/linux/virtio_i2c.h
 create mode 100644 include/standard-headers/linux/virtio_pcidev.h
 create mode 100644 include/standard-headers/linux/virtio_scmi.h
 create mode 100644 linux-headers/asm-riscv/bitsperlong.h
 create mode 100644 linux-headers/asm-riscv/mman.h
 create mode 100644 linux-headers/asm-riscv/unistd.h

-- 
2.34.1

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Hi,
> 
> This is V5 of the patches. I posted V4 here.
> 
> https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> 
> These will allow us to support SELinux with virtiofs. This will send
> SELinux context at file creation to server and server can set it on
> file.

I think that's pretty close; I've got some minor comments I've replied
to on the individual patches.

I do worry that the number of different paths for each operation is now
quite large so hard to test.
I also wonder what happens on something other than SELinux.

Dave

> Changes since V4
> ----------------
> - Parse only known current size of fuse_init_in. This will make sure
>   that future extension does not break existing code upon header
>   update. (David Gilbert)
> 
> - Changed order of one of the patch. It is first patch in series. This
>   will help fix the breakage before header update patch and code remains
>   git bisectable. (David Gilbert)
> 
> - Changed %lx to %llx at one place. (David Gilbert).
> 
> Thanks
> Vivek
>  
> Vivek Goyal (9):
>   virtiofsd: Fix breakage due to fuse_init_in size change
>   linux-headers: Update headers to v5.17-rc1
>   virtiofsd: Parse extended "struct fuse_init_in"
>   virtiofsd: Extend size of fuse_conn_info->capable and ->want fields
>   virtiofsd, fuse_lowlevel.c: Add capability to parse security context
>   virtiofsd: Move core file creation code in separate function
>   virtiofsd: Create new file with fscreate set
>   virtiofsd: Create new file using O_TMPFILE and set security context
>   virtiofsd: Add an option to enable/disable security label
> 
>  docs/tools/virtiofsd.rst                      |   7 +
>  include/standard-headers/asm-x86/kvm_para.h   |   1 +
>  include/standard-headers/drm/drm_fourcc.h     |  11 +
>  include/standard-headers/linux/ethtool.h      |   1 +
>  include/standard-headers/linux/fuse.h         |  60 ++-
>  include/standard-headers/linux/pci_regs.h     | 142 +++---
>  include/standard-headers/linux/virtio_gpio.h  |  72 +++
>  include/standard-headers/linux/virtio_i2c.h   |  47 ++
>  include/standard-headers/linux/virtio_iommu.h |   8 +-
>  .../standard-headers/linux/virtio_pcidev.h    |  65 +++
>  include/standard-headers/linux/virtio_scmi.h  |  24 +
>  linux-headers/asm-generic/unistd.h            |   5 +-
>  linux-headers/asm-mips/unistd_n32.h           |   2 +
>  linux-headers/asm-mips/unistd_n64.h           |   2 +
>  linux-headers/asm-mips/unistd_o32.h           |   2 +
>  linux-headers/asm-powerpc/unistd_32.h         |   2 +
>  linux-headers/asm-powerpc/unistd_64.h         |   2 +
>  linux-headers/asm-riscv/bitsperlong.h         |  14 +
>  linux-headers/asm-riscv/mman.h                |   1 +
>  linux-headers/asm-riscv/unistd.h              |  44 ++
>  linux-headers/asm-s390/unistd_32.h            |   2 +
>  linux-headers/asm-s390/unistd_64.h            |   2 +
>  linux-headers/asm-x86/kvm.h                   |  16 +-
>  linux-headers/asm-x86/unistd_32.h             |   1 +
>  linux-headers/asm-x86/unistd_64.h             |   1 +
>  linux-headers/asm-x86/unistd_x32.h            |   1 +
>  linux-headers/linux/kvm.h                     |  17 +
>  tools/virtiofsd/fuse_common.h                 |   9 +-
>  tools/virtiofsd/fuse_i.h                      |   7 +
>  tools/virtiofsd/fuse_lowlevel.c               | 162 +++++--
>  tools/virtiofsd/helper.c                      |   1 +
>  tools/virtiofsd/passthrough_ll.c              | 414 ++++++++++++++++--
>  32 files changed, 1013 insertions(+), 132 deletions(-)
>  create mode 100644 include/standard-headers/linux/virtio_gpio.h
>  create mode 100644 include/standard-headers/linux/virtio_i2c.h
>  create mode 100644 include/standard-headers/linux/virtio_pcidev.h
>  create mode 100644 include/standard-headers/linux/virtio_scmi.h
>  create mode 100644 linux-headers/asm-riscv/bitsperlong.h
>  create mode 100644 linux-headers/asm-riscv/mman.h
>  create mode 100644 linux-headers/asm-riscv/unistd.h
> 
> -- 
> 2.34.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Mon, Feb 07, 2022 at 12:49:24PM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > Hi,
> > 
> > This is V5 of the patches. I posted V4 here.
> > 
> > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > 
> > These will allow us to support SELinux with virtiofs. This will send
> > SELinux context at file creation to server and server can set it on
> > file.
> 
> I think that's pretty close; I've got some minor comments I've replied
> to on the individual patches.
> 
> I do worry that the number of different paths for each operation is now
> quite large so hard to test.

It is indeed many combinations to test. During development, I have made
sure to test every path atleast once to make sure it works.

> I also wonder what happens on something other than SELinux.

As of now this pretty much works only for SELinux. Especially usage of
fscreate knob is very specific to SELinux.

In some cases, it will work with some other LSM other than SELinux
as well. But lets not go there. 

If we want to support multiple security contexts at some point of time,
fuse procotol changes have been written in such a way so that fuse
can send mutiple security context and then we will have to modify
code to be able to deal with that.

In short, for now, this code is pretty much expectin one security
context that too SELinux. This is very much in line with ceph and
nfs.

Vivek
> 
> Dave
> 
> > Changes since V4
> > ----------------
> > - Parse only known current size of fuse_init_in. This will make sure
> >   that future extension does not break existing code upon header
> >   update. (David Gilbert)
> > 
> > - Changed order of one of the patch. It is first patch in series. This
> >   will help fix the breakage before header update patch and code remains
> >   git bisectable. (David Gilbert)
> > 
> > - Changed %lx to %llx at one place. (David Gilbert).
> > 
> > Thanks
> > Vivek
> >  
> > Vivek Goyal (9):
> >   virtiofsd: Fix breakage due to fuse_init_in size change
> >   linux-headers: Update headers to v5.17-rc1
> >   virtiofsd: Parse extended "struct fuse_init_in"
> >   virtiofsd: Extend size of fuse_conn_info->capable and ->want fields
> >   virtiofsd, fuse_lowlevel.c: Add capability to parse security context
> >   virtiofsd: Move core file creation code in separate function
> >   virtiofsd: Create new file with fscreate set
> >   virtiofsd: Create new file using O_TMPFILE and set security context
> >   virtiofsd: Add an option to enable/disable security label
> > 
> >  docs/tools/virtiofsd.rst                      |   7 +
> >  include/standard-headers/asm-x86/kvm_para.h   |   1 +
> >  include/standard-headers/drm/drm_fourcc.h     |  11 +
> >  include/standard-headers/linux/ethtool.h      |   1 +
> >  include/standard-headers/linux/fuse.h         |  60 ++-
> >  include/standard-headers/linux/pci_regs.h     | 142 +++---
> >  include/standard-headers/linux/virtio_gpio.h  |  72 +++
> >  include/standard-headers/linux/virtio_i2c.h   |  47 ++
> >  include/standard-headers/linux/virtio_iommu.h |   8 +-
> >  .../standard-headers/linux/virtio_pcidev.h    |  65 +++
> >  include/standard-headers/linux/virtio_scmi.h  |  24 +
> >  linux-headers/asm-generic/unistd.h            |   5 +-
> >  linux-headers/asm-mips/unistd_n32.h           |   2 +
> >  linux-headers/asm-mips/unistd_n64.h           |   2 +
> >  linux-headers/asm-mips/unistd_o32.h           |   2 +
> >  linux-headers/asm-powerpc/unistd_32.h         |   2 +
> >  linux-headers/asm-powerpc/unistd_64.h         |   2 +
> >  linux-headers/asm-riscv/bitsperlong.h         |  14 +
> >  linux-headers/asm-riscv/mman.h                |   1 +
> >  linux-headers/asm-riscv/unistd.h              |  44 ++
> >  linux-headers/asm-s390/unistd_32.h            |   2 +
> >  linux-headers/asm-s390/unistd_64.h            |   2 +
> >  linux-headers/asm-x86/kvm.h                   |  16 +-
> >  linux-headers/asm-x86/unistd_32.h             |   1 +
> >  linux-headers/asm-x86/unistd_64.h             |   1 +
> >  linux-headers/asm-x86/unistd_x32.h            |   1 +
> >  linux-headers/linux/kvm.h                     |  17 +
> >  tools/virtiofsd/fuse_common.h                 |   9 +-
> >  tools/virtiofsd/fuse_i.h                      |   7 +
> >  tools/virtiofsd/fuse_lowlevel.c               | 162 +++++--
> >  tools/virtiofsd/helper.c                      |   1 +
> >  tools/virtiofsd/passthrough_ll.c              | 414 ++++++++++++++++--
> >  32 files changed, 1013 insertions(+), 132 deletions(-)
> >  create mode 100644 include/standard-headers/linux/virtio_gpio.h
> >  create mode 100644 include/standard-headers/linux/virtio_i2c.h
> >  create mode 100644 include/standard-headers/linux/virtio_pcidev.h
> >  create mode 100644 include/standard-headers/linux/virtio_scmi.h
> >  create mode 100644 linux-headers/asm-riscv/bitsperlong.h
> >  create mode 100644 linux-headers/asm-riscv/mman.h
> >  create mode 100644 linux-headers/asm-riscv/unistd.h
> > 
> > -- 
> > 2.34.1
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Mon, Feb 07, 2022 at 12:49:24PM +0000, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > Hi,
> > > 
> > > This is V5 of the patches. I posted V4 here.
> > > 
> > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > > 
> > > These will allow us to support SELinux with virtiofs. This will send
> > > SELinux context at file creation to server and server can set it on
> > > file.
> > 
> > I think that's pretty close; I've got some minor comments I've replied
> > to on the individual patches.
> > 
> > I do worry that the number of different paths for each operation is now
> > quite large so hard to test.
> 
> It is indeed many combinations to test. During development, I have made
> sure to test every path atleast once to make sure it works.
> 
> > I also wonder what happens on something other than SELinux.
> 
> As of now this pretty much works only for SELinux. Especially usage of
> fscreate knob is very specific to SELinux.
> 
> In some cases, it will work with some other LSM other than SELinux
> as well. But lets not go there. 
> 
> If we want to support multiple security contexts at some point of time,
> fuse procotol changes have been written in such a way so that fuse
> can send mutiple security context and then we will have to modify
> code to be able to deal with that.
> 
> In short, for now, this code is pretty much expectin one security
> context that too SELinux. This is very much in line with ceph and
> nfs.

OK, as long as we have some feel for what we'd need to do if something
hit that.

Dave

> Vivek
> > 
> > Dave
> > 
> > > Changes since V4
> > > ----------------
> > > - Parse only known current size of fuse_init_in. This will make sure
> > >   that future extension does not break existing code upon header
> > >   update. (David Gilbert)
> > > 
> > > - Changed order of one of the patch. It is first patch in series. This
> > >   will help fix the breakage before header update patch and code remains
> > >   git bisectable. (David Gilbert)
> > > 
> > > - Changed %lx to %llx at one place. (David Gilbert).
> > > 
> > > Thanks
> > > Vivek
> > >  
> > > Vivek Goyal (9):
> > >   virtiofsd: Fix breakage due to fuse_init_in size change
> > >   linux-headers: Update headers to v5.17-rc1
> > >   virtiofsd: Parse extended "struct fuse_init_in"
> > >   virtiofsd: Extend size of fuse_conn_info->capable and ->want fields
> > >   virtiofsd, fuse_lowlevel.c: Add capability to parse security context
> > >   virtiofsd: Move core file creation code in separate function
> > >   virtiofsd: Create new file with fscreate set
> > >   virtiofsd: Create new file using O_TMPFILE and set security context
> > >   virtiofsd: Add an option to enable/disable security label
> > > 
> > >  docs/tools/virtiofsd.rst                      |   7 +
> > >  include/standard-headers/asm-x86/kvm_para.h   |   1 +
> > >  include/standard-headers/drm/drm_fourcc.h     |  11 +
> > >  include/standard-headers/linux/ethtool.h      |   1 +
> > >  include/standard-headers/linux/fuse.h         |  60 ++-
> > >  include/standard-headers/linux/pci_regs.h     | 142 +++---
> > >  include/standard-headers/linux/virtio_gpio.h  |  72 +++
> > >  include/standard-headers/linux/virtio_i2c.h   |  47 ++
> > >  include/standard-headers/linux/virtio_iommu.h |   8 +-
> > >  .../standard-headers/linux/virtio_pcidev.h    |  65 +++
> > >  include/standard-headers/linux/virtio_scmi.h  |  24 +
> > >  linux-headers/asm-generic/unistd.h            |   5 +-
> > >  linux-headers/asm-mips/unistd_n32.h           |   2 +
> > >  linux-headers/asm-mips/unistd_n64.h           |   2 +
> > >  linux-headers/asm-mips/unistd_o32.h           |   2 +
> > >  linux-headers/asm-powerpc/unistd_32.h         |   2 +
> > >  linux-headers/asm-powerpc/unistd_64.h         |   2 +
> > >  linux-headers/asm-riscv/bitsperlong.h         |  14 +
> > >  linux-headers/asm-riscv/mman.h                |   1 +
> > >  linux-headers/asm-riscv/unistd.h              |  44 ++
> > >  linux-headers/asm-s390/unistd_32.h            |   2 +
> > >  linux-headers/asm-s390/unistd_64.h            |   2 +
> > >  linux-headers/asm-x86/kvm.h                   |  16 +-
> > >  linux-headers/asm-x86/unistd_32.h             |   1 +
> > >  linux-headers/asm-x86/unistd_64.h             |   1 +
> > >  linux-headers/asm-x86/unistd_x32.h            |   1 +
> > >  linux-headers/linux/kvm.h                     |  17 +
> > >  tools/virtiofsd/fuse_common.h                 |   9 +-
> > >  tools/virtiofsd/fuse_i.h                      |   7 +
> > >  tools/virtiofsd/fuse_lowlevel.c               | 162 +++++--
> > >  tools/virtiofsd/helper.c                      |   1 +
> > >  tools/virtiofsd/passthrough_ll.c              | 414 ++++++++++++++++--
> > >  32 files changed, 1013 insertions(+), 132 deletions(-)
> > >  create mode 100644 include/standard-headers/linux/virtio_gpio.h
> > >  create mode 100644 include/standard-headers/linux/virtio_i2c.h
> > >  create mode 100644 include/standard-headers/linux/virtio_pcidev.h
> > >  create mode 100644 include/standard-headers/linux/virtio_scmi.h
> > >  create mode 100644 linux-headers/asm-riscv/bitsperlong.h
> > >  create mode 100644 linux-headers/asm-riscv/mman.h
> > >  create mode 100644 linux-headers/asm-riscv/unistd.h
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > -- 
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> Hi,
> 
> This is V5 of the patches. I posted V4 here.
> 
> https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> 
> These will allow us to support SELinux with virtiofs. This will send
> SELinux context at file creation to server and server can set it on
> file.

I've not entirely figured it out from the code, so easier for me
to ask...

How is the SELinux labelled stored on the host side ? It is stored
directly in the security.* xattr namespace, or is is subject to
xattr remapping that virtiofsd already supports.

Storing directly means virtiofsd has to run in an essentially
unconfined context, to let it do arbitrary  changes on security.*
xattrs without being blocked by SELinux) and has risk that guest
initiated changes can open holes in the host confinement if
the exported FS is generally visible to processes on the host.


Using remapping lets virtiofsd be strictly isolated by SELinux
policy on the host, and ensures that guest context changes
can't open up holes in the host.

Both are valid use cases, so I'd ultimately expect us to want
to support both, but my preference for a "default" behaviour
would be remapping.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > Hi,
> > 
> > This is V5 of the patches. I posted V4 here.
> > 
> > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > 
> > These will allow us to support SELinux with virtiofs. This will send
> > SELinux context at file creation to server and server can set it on
> > file.
> 
> I've not entirely figured it out from the code, so easier for me
> to ask...
> 
> How is the SELinux labelled stored on the host side ? It is stored
> directly in the security.* xattr namespace, or is is subject to
> xattr remapping that virtiofsd already supports.
> 
> Storing directly means virtiofsd has to run in an essentially
> unconfined context, to let it do arbitrary  changes on security.*
> xattrs without being blocked by SELinux) and has risk that guest
> initiated changes can open holes in the host confinement if
> the exported FS is generally visible to processes on the host.
> 
> 
> Using remapping lets virtiofsd be strictly isolated by SELinux
> policy on the host, and ensures that guest context changes
> can't open up holes in the host.
> 
> Both are valid use cases, so I'd ultimately expect us to want
> to support both, but my preference for a "default" behaviour
> would be remapping.

I am expecting users to configure virtiofsd to remap "security.selinux"
to "trusted.virtiofsd.security.selinux" and that will allow guest
and host security selinux to co-exist and allow separate SELinux policies
for guest and host.

I agree that my preference for a default behavior is remapping as well.
That makes most sense. 

One downside of mapping to trusted namespace is that it requires
CAP_SYS_ADMIN for virtiofsd.

Having said that, these patches don't enforce the remapping default. That
has to come from the user because it also needs to be given CAP_SYS_ADMIN.
So out of box default is no remapping and passthrough SELinux.

Thanks
Vivek

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

On Mon, Feb 07, 2022 at 08:24:08AM -0500, Vivek Goyal wrote:
> On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > > Hi,
> > > 
> > > This is V5 of the patches. I posted V4 here.
> > > 
> > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > > 
> > > These will allow us to support SELinux with virtiofs. This will send
> > > SELinux context at file creation to server and server can set it on
> > > file.
> > 
> > I've not entirely figured it out from the code, so easier for me
> > to ask...
> > 
> > How is the SELinux labelled stored on the host side ? It is stored
> > directly in the security.* xattr namespace, or is is subject to
> > xattr remapping that virtiofsd already supports.
> > 
> > Storing directly means virtiofsd has to run in an essentially
> > unconfined context, to let it do arbitrary  changes on security.*
> > xattrs without being blocked by SELinux) and has risk that guest
> > initiated changes can open holes in the host confinement if
> > the exported FS is generally visible to processes on the host.
> > 
> > 
> > Using remapping lets virtiofsd be strictly isolated by SELinux
> > policy on the host, and ensures that guest context changes
> > can't open up holes in the host.
> > 
> > Both are valid use cases, so I'd ultimately expect us to want
> > to support both, but my preference for a "default" behaviour
> > would be remapping.
> 
> I am expecting users to configure virtiofsd to remap "security.selinux"
> to "trusted.virtiofsd.security.selinux" and that will allow guest
> and host security selinux to co-exist and allow separate SELinux policies
> for guest and host.
> 
> I agree that my preference for a default behavior is remapping as well.
> That makes most sense. 
> 
> One downside of mapping to trusted namespace is that it requires
> CAP_SYS_ADMIN for virtiofsd.
> 
> Having said that, these patches don't enforce the remapping default. That
> has to come from the user because it also needs to be given CAP_SYS_ADMIN.
> So out of box default is no remapping and passthrough SELinux.

Ok, that all makes sense then. My only suggestion then is to put something
more explicit in the man page docs to highlight the implications /
interaction beteen the new command line options for labelling and the
likely need for remapping security.*


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Feb 07, 2022 at 01:30:16PM +0000, Daniel P. Berrangé wrote:
> On Mon, Feb 07, 2022 at 08:24:08AM -0500, Vivek Goyal wrote:
> > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > > > Hi,
> > > > 
> > > > This is V5 of the patches. I posted V4 here.
> > > > 
> > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > > > 
> > > > These will allow us to support SELinux with virtiofs. This will send
> > > > SELinux context at file creation to server and server can set it on
> > > > file.
> > > 
> > > I've not entirely figured it out from the code, so easier for me
> > > to ask...
> > > 
> > > How is the SELinux labelled stored on the host side ? It is stored
> > > directly in the security.* xattr namespace, or is is subject to
> > > xattr remapping that virtiofsd already supports.
> > > 
> > > Storing directly means virtiofsd has to run in an essentially
> > > unconfined context, to let it do arbitrary  changes on security.*
> > > xattrs without being blocked by SELinux) and has risk that guest
> > > initiated changes can open holes in the host confinement if
> > > the exported FS is generally visible to processes on the host.
> > > 
> > > 
> > > Using remapping lets virtiofsd be strictly isolated by SELinux
> > > policy on the host, and ensures that guest context changes
> > > can't open up holes in the host.
> > > 
> > > Both are valid use cases, so I'd ultimately expect us to want
> > > to support both, but my preference for a "default" behaviour
> > > would be remapping.
> > 
> > I am expecting users to configure virtiofsd to remap "security.selinux"
> > to "trusted.virtiofsd.security.selinux" and that will allow guest
> > and host security selinux to co-exist and allow separate SELinux policies
> > for guest and host.
> > 
> > I agree that my preference for a default behavior is remapping as well.
> > That makes most sense. 
> > 
> > One downside of mapping to trusted namespace is that it requires
> > CAP_SYS_ADMIN for virtiofsd.
> > 
> > Having said that, these patches don't enforce the remapping default. That
> > has to come from the user because it also needs to be given CAP_SYS_ADMIN.
> > So out of box default is no remapping and passthrough SELinux.
> 
> Ok, that all makes sense then. My only suggestion then is to put something
> more explicit in the man page docs to highlight the implications /
> interaction beteen the new command line options for labelling and the
> likely need for remapping security.*

Ok, will do. While describing this new command line option, will also
mention the likely need of remapping and additional capability and
security implication. Or may be I will create a small new section for
SELinux in same file.

Thanks
Vivek

On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > Hi,
> > 
> > This is V5 of the patches. I posted V4 here.
> > 
> > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > 
> > These will allow us to support SELinux with virtiofs. This will send
> > SELinux context at file creation to server and server can set it on
> > file.
> 
> I've not entirely figured it out from the code, so easier for me
> to ask...
> 
> How is the SELinux labelled stored on the host side ? It is stored
> directly in the security.* xattr namespace,

[ CC Dan Walsh ]

I just tried to test the mode where I don't do xattr remapping and try
to set /proc/pid/attr/fscreate with the context I want to set. It will
set security.selinux xattr on host.

But write to /proc/pid/attr/fscreate fails if host does not recognize
the label sent by guest. I am running virtiofsd with unconfined_t but
it still fails because guest is trying to create a file with
"test_filesystem_filetranscon_t" and host does not recognize this
label. Seeing following in audit logs.

type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0"

So if we don't remap xattrs and host has SELinux enabled, then it probably
work in very limited circumstances where host and guest policies don't
conflict. I guess its like running fedora 34 guest on fedora 34 host. 
I suspect that this will see very limited use. Though I have put the
code in for the sake of completeness.

Thanks
Vivek

> or is is subject to
> xattr remapping that virtiofsd already supports.
> 
> Storing directly means virtiofsd has to run in an essentially
> unconfined context, to let it do arbitrary  changes on security.*
> xattrs without being blocked by SELinux) and has risk that guest
> initiated changes can open holes in the host confinement if
> the exported FS is generally visible to processes on the host.
> 
> 
> Using remapping lets virtiofsd be strictly isolated by SELinux
> policy on the host, and ensures that guest context changes
> can't open up holes in the host.
> 
> Both are valid use cases, so I'd ultimately expect us to want
> to support both, but my preference for a "default" behaviour
> would be remapping.
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

On 2/7/22 16:19, Vivek Goyal wrote:
> On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
>> On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
>>> Hi,
>>>
>>> This is V5 of the patches. I posted V4 here.
>>>
>>> https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
>>>
>>> These will allow us to support SELinux with virtiofs. This will send
>>> SELinux context at file creation to server and server can set it on
>>> file.
>> I've not entirely figured it out from the code, so easier for me
>> to ask...
>>
>> How is the SELinux labelled stored on the host side ? It is stored
>> directly in the security.* xattr namespace,
> [ CC Dan Walsh ]
>
> I just tried to test the mode where I don't do xattr remapping and try
> to set /proc/pid/attr/fscreate with the context I want to set. It will
> set security.selinux xattr on host.
>
> But write to /proc/pid/attr/fscreate fails if host does not recognize
> the label sent by guest. I am running virtiofsd with unconfined_t but
> it still fails because guest is trying to create a file with
> "test_filesystem_filetranscon_t" and host does not recognize this
> label. Seeing following in audit logs.
>
> type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0"
>
> So if we don't remap xattrs and host has SELinux enabled, then it probably
> work in very limited circumstances where host and guest policies don't
> conflict. I guess its like running fedora 34 guest on fedora 34 host.
> I suspect that this will see very limited use. Though I have put the
> code in for the sake of completeness.
>
> Thanks
> Vivek
>
>> or is is subject to
>> xattr remapping that virtiofsd already supports.
>>
>> Storing directly means virtiofsd has to run in an essentially
>> unconfined context, to let it do arbitrary  changes on security.*
>> xattrs without being blocked by SELinux) and has risk that guest
>> initiated changes can open holes in the host confinement if
>> the exported FS is generally visible to processes on the host.
>>
>>
>> Using remapping lets virtiofsd be strictly isolated by SELinux
>> policy on the host, and ensures that guest context changes
>> can't open up holes in the host.
>>
>> Both are valid use cases, so I'd ultimately expect us to want
>> to support both, but my preference for a "default" behaviour
>> would be remapping.
>>
>> Regards,
>> Daniel
>> -- 
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>>
I had a bug today, where someone tried to run a --privileged container 
with RHEL9 which attempted to place a label onto a volume from the RHEL8 
host and got a MAC_ADMIN avc and the label was denied.

Even unconfined domains are not allowed to place non-understood labels 
on disk.  The problem is when the processes on RHEL8 would look at the 
labeled file, they would just see it as unlabeled_t.

On Mon, Feb 07, 2022 at 04:19:38PM -0500, Vivek Goyal wrote:
> On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > > Hi,
> > > 
> > > This is V5 of the patches. I posted V4 here.
> > > 
> > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > > 
> > > These will allow us to support SELinux with virtiofs. This will send
> > > SELinux context at file creation to server and server can set it on
> > > file.
> > 
> > I've not entirely figured it out from the code, so easier for me
> > to ask...
> > 
> > How is the SELinux labelled stored on the host side ? It is stored
> > directly in the security.* xattr namespace,
> 
> [ CC Dan Walsh ]
> 
> I just tried to test the mode where I don't do xattr remapping and try
> to set /proc/pid/attr/fscreate with the context I want to set. It will
> set security.selinux xattr on host.
> 
> But write to /proc/pid/attr/fscreate fails if host does not recognize
> the label sent by guest. I am running virtiofsd with unconfined_t but
> it still fails because guest is trying to create a file with
> "test_filesystem_filetranscon_t" and host does not recognize this
> label. Seeing following in audit logs.
> 
> type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0"

Yes, that's to be expected if the host policy doesn't know about the
label that the guest is using.

IOW, non-mapping case is only useful if you have a very good match
between host + guest OS policy. This could be useful for an app
like Kata because their guest is not a full OS, it is something
special purpose and tightly controlled.

> So if we don't remap xattrs and host has SELinux enabled, then it probably
> work in very limited circumstances where host and guest policies don't
> conflict. I guess its like running fedora 34 guest on fedora 34 host. 
> I suspect that this will see very limited use. Though I have put the
> code in for the sake of completeness.

For general purpose guest OS virtualization remapping is going to be
effectuively mandatory.  The non-mapped case only usable when you tightly
control the guest OS packages from the host.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, Feb 8, 2022 at 11:44 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Feb 07, 2022 at 04:19:38PM -0500, Vivek Goyal wrote:
> > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > > > Hi,
> > > >
> > > > This is V5 of the patches. I posted V4 here.
> > > >
> > > >
> https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > > >
> > > > These will allow us to support SELinux with virtiofs. This will send
> > > > SELinux context at file creation to server and server can set it on
> > > > file.
> > >
> > > I've not entirely figured it out from the code, so easier for me
> > > to ask...
> > >
> > > How is the SELinux labelled stored on the host side ? It is stored
> > > directly in the security.* xattr namespace,
> >
> > [ CC Dan Walsh ]
> >
> > I just tried to test the mode where I don't do xattr remapping and try
> > to set /proc/pid/attr/fscreate with the context I want to set. It will
> > set security.selinux xattr on host.
> >
> > But write to /proc/pid/attr/fscreate fails if host does not recognize
> > the label sent by guest. I am running virtiofsd with unconfined_t but
> > it still fails because guest is trying to create a file with
> > "test_filesystem_filetranscon_t" and host does not recognize this
> > label. Seeing following in audit logs.
> >
> > type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate
> invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0"
>
> Yes, that's to be expected if the host policy doesn't know about the
> label that the guest is using.
>
> IOW, non-mapping case is only useful if you have a very good match
> between host + guest OS policy. This could be useful for an app
> like Kata because their guest is not a full OS, it is something
> special purpose and tightly controlled.
>
> > So if we don't remap xattrs and host has SELinux enabled, then it
> probably
> > work in very limited circumstances where host and guest policies don't
> > conflict. I guess its like running fedora 34 guest on fedora 34 host.
> > I suspect that this will see very limited use. Though I have put the
> > code in for the sake of completeness.
>
> For general purpose guest OS virtualization remapping is going to be
> effectuively mandatory.  The non-mapped case only usable when you tightly
> control the guest OS packages from the host.
>


If remap is recommended, why not make it mandatory or automatic?,
for instance, '-o security_label' either requires '-o xattrmap=' or
automatically makes
the mapping with the 'trusted' prefix, while  '-o security_label=nomap'
doesn't, so you
can choose whatever you want.

(I'm not suggesting the 'nomap' name, I'm terrible choosing names)

-- 
German

On Wed, Feb 09, 2022 at 11:24:29AM +0100, German Maglione wrote:
> On Tue, Feb 8, 2022 at 11:44 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Feb 07, 2022 at 04:19:38PM -0500, Vivek Goyal wrote:
> > > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote:
> > > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote:
> > > > > Hi,
> > > > >
> > > > > This is V5 of the patches. I posted V4 here.
> > > > >
> > > > >
> > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html
> > > > >
> > > > > These will allow us to support SELinux with virtiofs. This will send
> > > > > SELinux context at file creation to server and server can set it on
> > > > > file.
> > > >
> > > > I've not entirely figured it out from the code, so easier for me
> > > > to ask...
> > > >
> > > > How is the SELinux labelled stored on the host side ? It is stored
> > > > directly in the security.* xattr namespace,
> > >
> > > [ CC Dan Walsh ]
> > >
> > > I just tried to test the mode where I don't do xattr remapping and try
> > > to set /proc/pid/attr/fscreate with the context I want to set. It will
> > > set security.selinux xattr on host.
> > >
> > > But write to /proc/pid/attr/fscreate fails if host does not recognize
> > > the label sent by guest. I am running virtiofsd with unconfined_t but
> > > it still fails because guest is trying to create a file with
> > > "test_filesystem_filetranscon_t" and host does not recognize this
> > > label. Seeing following in audit logs.
> > >
> > > type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate
> > invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0"
> >
> > Yes, that's to be expected if the host policy doesn't know about the
> > label that the guest is using.
> >
> > IOW, non-mapping case is only useful if you have a very good match
> > between host + guest OS policy. This could be useful for an app
> > like Kata because their guest is not a full OS, it is something
> > special purpose and tightly controlled.
> >
> > > So if we don't remap xattrs and host has SELinux enabled, then it
> > probably
> > > work in very limited circumstances where host and guest policies don't
> > > conflict. I guess its like running fedora 34 guest on fedora 34 host.
> > > I suspect that this will see very limited use. Though I have put the
> > > code in for the sake of completeness.
> >
> > For general purpose guest OS virtualization remapping is going to be
> > effectuively mandatory.  The non-mapped case only usable when you tightly
> > control the guest OS packages from the host.
> >
> 
> 
> If remap is recommended, why not make it mandatory or automatic?,
> for instance, '-o security_label' either requires '-o xattrmap=' or
> automatically makes
> the mapping with the 'trusted' prefix, while  '-o security_label=nomap'
> doesn't, so you
> can choose whatever you want.

It is a recommended settings but not a mandatory setting. So enforcing
any kind of policy will work for some and not for others.

We could refine it down the line depending on how it is used and where
people find it useful.

For now, primary focus is to get basic support patches in the tree.

Thanks
Vivek

> 
> (I'm not suggesting the 'nomap' name, I'm terrible choosing names)
> 
> -- 
> German