Re: virtiofs and its optional xattr support vs. fs_use_xattr

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Dec 11, 2020 at 10:15:57AM +0100, Ondrej Mosnacek wrote:
> On Thu, Dec 10, 2020 at 11:31 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Dec 10, 2020 at 11:24:30PM +0100, Ondrej Mosnacek wrote:
> > > On Thu, Dec 10, 2020 at 11:17 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > On Thu, Dec 10, 2020 at 10:29:02AM +0100, Ondrej Mosnacek wrote:
> > > > > On Thu, Dec 10, 2020 at 3:40 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, Dec 9, 2020 at 10:37 AM James Carter <jwcart2@gmail.com> wrote:
> > > > > > > On Tue, Dec 8, 2020 at 6:45 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > > On Mon, Dec 7, 2020 at 12:17 PM James Carter <jwcart2@gmail.com> wrote:
> > > > > > > > > On Mon, Dec 7, 2020 at 9:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi everyone,
> > > > > > > > > >
> > > > > > > > > > In [1] we ran into a problem with the current handling of filesystem
> > > > > > > > > > labeling rules. Basically, it is only possible to specify either
> > > > > > > > > > genfscon or fs_use_xattr for a given filesystem, but in the case of
> > > > > > > > > > virtiofs, certain mounts may support security xattrs, while other ones
> > > > > > > > > > may not.
> > > > > > > > > >
> > > > > > > > > > So we can't use the xattr support by adding fs_use_xattr virtiofs
> > > > > > > > > > (...); to the policy, because then a non-xattr mount will fail
> > > > > > > > > > (SELinux does a mount-time check on the root inode to make sure that
> > > > > > > > > > the xattr handler works), but we also don't want to stay on genfscon,
> > > > > > > > > > because then we can't relabel files.
> > > > > > > > > >
> > > > > > > > > > So my question is how to best address this? One option is to use a
> > > > > > > > > > similar "hack" as for cgroupfs; i.e. do a kind of mixed genfs-xattr
> > > > > > > > > > labeling, but that's ugly and requires hard-coding another FS name in
> > > > > > > > > > the selinux code. The only other alternative I could come up with is
> > > > > > > > > > to add a new FS labeling statement that would specify some kind of
> > > > > > > > > > mixed genfscon / fs_use_xattr behavior. That would be a better
> > > > > > > > > > long-term solution, but leads to more questions on how such statement
> > > > > > > > > > should actually work... Should it work the cgroupfs way, giving a
> > > > > > > > > > default label to everything and allowing to set/change labels via
> > > > > > > > > > xattrs? Or should it rather just detect xattrs support and switch
> > > > > > > > > > between SECURITY_FS_USE_XATTR and SECURITY_FS_USE_GENFS behavior based
> > > > > > > > > > on that? In the latter case, should the statement specify two contexts
> > > > > > > > > > (one for fs_use_xattr and another one for genfscon) or just one for
> > > > > > > > > > both behaviors?
> > > > > > > > >
> > > > > > > > > I don't think adding a new statement is necessary. It seems like
> > > > > > > > > allowing both fs_use_xattr and genfscon rules for the filesystem in
> > > > > > > > > policy and then using the fs_use_xattr rule if xattrs are supported
> > > > > > > > > while falling back to the genfscon rule if they are not would do what
> > > > > > > > > you need.
> > > > > > > >
> > > > > > > > That seems reasonable to me so long as this ambiguity is okay with the
> > > > > > > > folks who do policy analysis.  Thinking quickly I'm not sure why it
> > > > > > > > would be a problem, but the thought did occur while I was typing up
> > > > > > > > this reply ...
> > > > > > >
> > > > > > > I don't think that it would cause a problem with policy analysis. I
> > > > > > > think that you would just assume the genfscon rule is being used,
> > > > > > > since it is less fine-grained. It wouldn't be much different from how
> > > > > > > booleans are handled.
> > > > > >
> > > > > > Makes sense to me.  Thanks Jim.
> > > > >
> > > > > Okay, so I'll look into switching between use_xattr and use_genfs
> > > > > based on the availability of xattr support and the presence of
> > > > > corresponding rules in the policy. Thanks everyone for the fruitful
> > > > > discussion!
> > > >
> > > > Hi Ondrej,
> > > >
> > > > So this is now purely a policy change and no changes required in kernel?
> > > > If yes, then the patch Dan Walsh proposed, is that good enough or
> > > > it needs to be done in a different way.
> > >
> > > No, this needs a kernel change in SELinux to interpret the policy
> > > rules slightly differently *and* basically Dan's patch (modulo the
> > > typo in the genfscon keyword).
> >
> > Ok, thanks. Is this kernel change something you will be able to take
> > care of. I am afraid that I don't know enough to make this change.
> 
> Yes, it's already on my todo list ;) But it might take some time as
> there are a lot of things competing for my attention right now...

Fair enough. Whenever you get a chance. Thank you.

Vivek