Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes

From: Daniel Colascione <dancol@google.com>
To: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>,
	Sandeep Patil <sspatil@google.com>,
	Paul Moore <paul@paul-moore.com>,
	LSM List <linux-security-module@vger.kernel.org>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	SElinux list <selinux@vger.kernel.org>,
	kvm@vger.kernel.org, Nick Kralevich <nnk@google.com>,
	Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
Date: Tue, 10 Mar 2020 14:50:27 -0700	[thread overview]
Message-ID: <CAKOZuevcz+fvfhRXPx2iZGtkk6+FjVj3ZSaGGT8DfwsOJR0k3A@mail.gmail.com> (raw)
In-Reply-To: <CAEjxPJ4+NM6-tfOeZ6UQfas6=KxtBTAk6f23GEyLomFn3K3qew@mail.gmail.com>

On Tue, Mar 10, 2020 at 11:25 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 2:11 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 10:50 AM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > On Thu, Feb 20, 2020 at 10:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >
> > > > On 2/17/2020 4:14 PM, Paul Moore wrote:
> > > > > On Thu, Feb 13, 2020 at 2:41 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > > >> Add support for labeling and controlling access to files attached to anon
> > > > >> inodes. Introduce extended interfaces for creating such files to permit
> > > > >> passing a related file as an input to decide how to label the anon
> > > > >> inode. Define a security hook for initializing the anon inode security
> > > > >> attributes. Security attributes are either inherited from a related file
> > > > >> or determined based on some combination of the creating task and policy
> > > > >> (in the case of SELinux, using type_transition rules).  As an
> > > > >> example user of the inheritance support, convert kvm to use the new
> > > > >> interface for passing the related file so that the anon inode can inherit
> > > > >> the security attributes of /dev/kvm and provide consistent access control
> > > > >> for subsequent ioctl operations.  Other users of anon inodes, including
> > > > >> userfaultfd, will default to the transition-based mechanism instead.
> > > > >>
> > > > >> Compared to the series in
> > > > >> https://lore.kernel.org/selinux/20200211225547.235083-1-dancol@google.com/,
> > > > >> this approach differs in that it does not require creation of a separate
> > > > >> anonymous inode for each file (instead storing the per-instance security
> > > > >> information in the file security blob), it applies labeling and control
> > > > >> to all users of anonymous inodes rather than requiring opt-in via a new
> > > > >> flag, it supports labeling based on a related inode if provided,
> > > > >> it relies on type transitions to compute the label of the anon inode
> > > > >> when there is no related inode, and it does not require introducing a new
> > > > >> security class for each user of anonymous inodes.
> > > > >>
> > > > >> On the other hand, the approach in this patch does expose the name passed
> > > > >> by the creator of the anon inode to the policy (an indirect mapping could
> > > > >> be provided within SELinux if these names aren't considered to be stable),
> > > > >> requires the definition of type_transition rules to distinguish userfaultfd
> > > > >> inodes from proc inodes based on type since they share the same class,
> > > > >> doesn't support denying the creation of anonymous inodes (making the hook
> > > > >> added by this patch return something other than void is problematic due to
> > > > >> it being called after the file is already allocated and error handling in
> > > > >> the callers can't presently account for this scenario and end up calling
> > > > >> release methods multiple times), and may be more expensive
> > > > >> (security_transition_sid overhead on each anon inode allocation).
> > > > >>
> > > > >> We are primarily posting this RFC patch now so that the two different
> > > > >> approaches can be concretely compared.  We anticipate a hybrid of the
> > > > >> two approaches being the likely outcome in the end.  In particular
> > > > >> if support for allocating a separate inode for each of these files
> > > > >> is acceptable, then we would favor storing the security information
> > > > >> in the inode security blob and using it instead of the file security
> > > > >> blob.
> > > > > Bringing this back up in hopes of attracting some attention from the
> > > > > fs-devel crowd and Al.  As Stephen already mentioned, from a SELinux
> > > > > perspective we would prefer to attach the security blob to the inode
> > > > > as opposed to the file struct; does anyone have any objections to
> > > > > that?
> > > >
> > > > Sorry for the delay - been sick the past few days.
> > > >
> > > > I agree that the inode is a better place than the file for information
> > > > about the inode. This is especially true for Smack, which uses
> > > > multiple extended attributes in some cases. I don't believe that any
> > > > except the access label will be relevant to anonymous inodes, but
> > > > I can imagine security modules with policies that would.
> > > >
> > > > I am always an advocate of full xattr support. It goes a long
> > > > way in reducing the number and complexity of special case interfaces.
> > >
> > > It sounds like we have broad consensus on using the inode to hold
> > > security information, implying that anon_inodes should create new
> > > inodes. Do any of the VFS people want to object?
> >
> > Ping?
>
> I'd recommend refreshing your patch series to incorporate feedback on
> the previous version and re-post,
> including viro and linux-fsdevel on the cc, and see if they have any
> comments on it.

I don't think there's anything in the patch series that needs to
change right now. AFAICT, we're still just waiting on comment from the
VFS people, who should be on this thread. Did I miss something?