[RFC] security,anon_inodes,kvm: enable security support for anon inodes
diff mbox series

Message ID 20200213194157.5877-1-sds@tycho.nsa.gov
State RFC
Headers show
Series
  • [RFC] security,anon_inodes,kvm: enable security support for anon inodes
Related show

Commit Message

Stephen Smalley Feb. 13, 2020, 7:41 p.m. UTC
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.

This patch only converts kvm to use a related inode (/dev/kvm) for the
creation of anon inodes as an example user. We would look to
incrementally convert other subsystems where applicable. This could
potentially cause policy breakage if policies are written for a subsystem
using the type_transition rules and then the subsystem is later converted
to use a related inode, so some means of compatible evolution would be
required.

There are a number of file permission checks in SELinux that do not
currently use the file_has_perm() helper and therefore do not pick up
the logic for handling these anonymous inodes. These will need to be looked
at further to see whether they are relevant to anonymous inodes and should
be updated with the new logic.

Any hooks that are directly passed a dentry or inode and not a file will
still skip permission checking by virtue of the S_ISPRIVATE() checks in
security/security.c and in security/selinux/hooks.c. At some point they
need to be audited to determine whether they are relevant to anonymous
inodes.

This change may create compatibility issues since it unconditionally
enables labeling and access control for all anonymous inodes; it is
known to break two selinux-testsuite bpf tests without adjusting its
policy.  It will likely need to be wrapped by a SELinux policy
capability before being merged to prevent breaking existing policies.

An example of a sample program and policy will follow in a follow-up
to this patch to demonstrate the effect on userfaultfd and kvm.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 fs/anon_inodes.c                  | 53 +++++++++++++++++-------
 include/linux/anon_inodes.h       |  7 ++++
 include/linux/lsm_hooks.h         | 11 +++++
 include/linux/security.h          |  8 ++++
 security/security.c               |  7 ++++
 security/selinux/hooks.c          | 69 ++++++++++++++++++++++++++++---
 security/selinux/include/objsec.h |  4 ++
 virt/kvm/kvm_main.c               | 27 +++++++-----
 8 files changed, 155 insertions(+), 31 deletions(-)

Comments

Stephen Smalley Feb. 13, 2020, 7:47 p.m. UTC | #1
On 2/13/20 2:41 PM, Stephen Smalley wrote:
> An example of a sample program and policy will follow in a follow-up
> to this patch to demonstrate the effect on userfaultfd and kvm.

Attached are example test programs and policies to demonstrate the 
change in behavior before and after this RFC patch for userfaultfd and 
kvm.  The test policies can be edited to selectively allow specific 
permissions for testing various scenarios, but with the defaults in 
them, one should see the following behavior:

sudo semodule -i kvm.cil userfaultfd.cil
make kvm userfaultfd

Before:

(no labeling/access control applied by SELinux to userfaultfd files or 
to anon inodes created by kvm)

$ ./userfaultfd
api: 170
features: 510
ioctls: 9223372036854775811

read: Resource temporarily unavailable

$ ./kvm
api version: 12

created vm

created vcpu

rax: 0
rbx: 0
rcx: 0
rdx: 1536
rdi: 0
rsi: 0
rsp: 0
rbp: 0
r8: 0
r9: 0
r10: 0
r11: 0
r12: 0
r13: 0
r14: 0
r15: 0
rip: 65520
rflags: 2

created device

checked device attr

After:

(SELinux ioctl whitelisting used to selectively deny access)

./userfaultfd
UFFDIO_API: Permission denied

$ ./kvm
api version: 12

created vm

created vcpu

KVM_GET_REGS: Permission denied
Paul Moore Feb. 18, 2020, 12:14 a.m. UTC | #2
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?
Casey Schaufler Feb. 20, 2020, 6:11 p.m. UTC | #3
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.
Daniel Colascione Feb. 20, 2020, 6:50 p.m. UTC | #4
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?
Daniel Colascione March 10, 2020, 6:09 p.m. UTC | #5
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?
Stephen Smalley March 10, 2020, 6:26 p.m. UTC | #6
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.
Daniel Colascione March 10, 2020, 9:50 p.m. UTC | #7
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?
Stephen Smalley March 11, 2020, 1:31 p.m. UTC | #8
On Tue, Mar 10, 2020 at 5:51 PM Daniel Colascione <dancol@google.com> wrote:
>
> 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:
> > > > > >> 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?

There was some discussion on the SELinux bits in patch 2/3.  I would
take the silence on
the vfs bits as implicit acceptance until you hear otherwise and just
submit a v2 that addresses
the SELinux bits.

Patch
diff mbox series

diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..4579913cb33e 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -21,6 +21,7 @@ 
 #include <linux/magic.h>
 #include <linux/anon_inodes.h>
 #include <linux/pseudo_fs.h>
+#include <linux/security.h>
 
 #include <linux/uaccess.h>
 
@@ -55,15 +56,25 @@  static struct file_system_type anon_inode_fs_type = {
 	.kill_sb	= kill_anon_super,
 };
 
+struct file *anon_inode_getfile(const char *name,
+				const struct file_operations *fops,
+				void *priv, int flags)
+{
+	return anon_inode_getfile_inherit(name, fops, priv, flags, NULL);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
 /**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- *                      anonymous inode, and a dentry that describe the "class"
- *                      of the file
+ * anon_inode_getfile_inherit - creates a new file instance by hooking it up to
+ *                              an anonymous inode, and a dentry that describe
+ *                              the "class" of the file
  *
  * @name:    [in]    name of the "class" of the new file
  * @fops:    [in]    file operations for the new file
  * @priv:    [in]    private data for the new file (will be file's private_data)
  * @flags:   [in]    flags
+ * @related: [in optional]    a related file that security attributes will be
+ *                            inherited from.
  *
  * Creates a new file by hooking it on a single inode. This is useful for files
  * that do not need to have a full-fledged inode in order to operate correctly.
@@ -71,9 +82,10 @@  static struct file_system_type anon_inode_fs_type = {
  * hence saving memory and avoiding code duplication for the file/inode/dentry
  * setup.  Returns the newly created file* or an error pointer.
  */
-struct file *anon_inode_getfile(const char *name,
-				const struct file_operations *fops,
-				void *priv, int flags)
+struct file *anon_inode_getfile_inherit(const char *name,
+					const struct file_operations *fops,
+					void *priv, int flags,
+					const struct file *related)
 {
 	struct file *file;
 
@@ -97,6 +109,8 @@  struct file *anon_inode_getfile(const char *name,
 
 	file->private_data = priv;
 
+	security_anon_file_init(file, related, name);
+
 	return file;
 
 err:
@@ -104,17 +118,27 @@  struct file *anon_inode_getfile(const char *name,
 	module_put(fops->owner);
 	return file;
 }
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
+EXPORT_SYMBOL_GPL(anon_inode_getfile_inherit);
+
+int anon_inode_getfd(const char *name,
+		     const struct file_operations *fops, void *priv,
+		     int flags)
+{
+	return anon_inode_getfd_inherit(name, fops, priv, flags, NULL);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd);
 
 /**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- *                    anonymous inode, and a dentry that describe the "class"
- *                    of the file
+ * anon_inode_getfd_inherit - creates and installs a new file instance by
+ *                            hooking it up to an anonymous inode, and a dentry
+ *                            that describe the "class" of the file
  *
  * @name:    [in]    name of the "class" of the new file
  * @fops:    [in]    file operations for the new file
  * @priv:    [in]    private data for the new file (will be file's private_data)
  * @flags:   [in]    flags
+ * @related: [in optional]    a related file that security attributes will be
+ *                            inherited from.
  *
  * Creates a new file by hooking it on a single inode. This is useful for files
  * that do not need to have a full-fledged inode in order to operate correctly.
@@ -122,8 +146,9 @@  EXPORT_SYMBOL_GPL(anon_inode_getfile);
  * hence saving memory and avoiding code duplication for the file/inode/dentry
  * setup.  Returns new descriptor or an error code.
  */
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
-		     void *priv, int flags)
+int anon_inode_getfd_inherit(const char *name,
+			     const struct file_operations *fops, void *priv,
+			     int flags, const struct file *related)
 {
 	int error, fd;
 	struct file *file;
@@ -133,7 +158,7 @@  int anon_inode_getfd(const char *name, const struct file_operations *fops,
 		return error;
 	fd = error;
 
-	file = anon_inode_getfile(name, fops, priv, flags);
+	file = anon_inode_getfile_inherit(name, fops, priv, flags, related);
 	if (IS_ERR(file)) {
 		error = PTR_ERR(file);
 		goto err_put_unused_fd;
@@ -146,7 +171,7 @@  int anon_inode_getfd(const char *name, const struct file_operations *fops,
 	put_unused_fd(fd);
 	return error;
 }
-EXPORT_SYMBOL_GPL(anon_inode_getfd);
+EXPORT_SYMBOL_GPL(anon_inode_getfd_inherit);
 
 static int __init anon_inode_init(void)
 {
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..c385eff5d852 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -14,8 +14,15 @@  struct file_operations;
 struct file *anon_inode_getfile(const char *name,
 				const struct file_operations *fops,
 				void *priv, int flags);
+struct file *anon_inode_getfile_inherit(const char *name,
+					const struct file_operations *fops,
+					void *priv, int flags,
+					const struct file *related);
 int anon_inode_getfd(const char *name, const struct file_operations *fops,
 		     void *priv, int flags);
+int anon_inode_getfd_inherit(const char *name,
+			     const struct file_operations *fops, void *priv,
+			     int flags, const struct file *related);
 
 #endif /* _LINUX_ANON_INODES_H */
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..253ecffeda0b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1454,6 +1454,14 @@ 
  *     code execution in kernel space should be permitted.
  *
  *     @what: kernel feature being accessed
+ *
+ * @anon_file_init:
+ *     Initialize the security attributes of a file attached to an anonymous
+ *     inode.
+ *     @file contains the file to initialize
+ *     @related contains a related file that security attributes will be
+ *     inherited from.
+ *     @name contains the name of the "class" of the file
  */
 union security_list_options {
 	int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1818,6 +1826,8 @@  union security_list_options {
 	void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
 #endif /* CONFIG_BPF_SYSCALL */
 	int (*locked_down)(enum lockdown_reason what);
+	void (*anon_file_init)(struct file *file, const struct file *related,
+				const char *name);
 #ifdef CONFIG_PERF_EVENTS
 	int (*perf_event_open)(struct perf_event_attr *attr, int type);
 	int (*perf_event_alloc)(struct perf_event *event);
@@ -2068,6 +2078,7 @@  struct security_hook_heads {
 	struct hlist_head bpf_prog_free_security;
 #endif /* CONFIG_BPF_SYSCALL */
 	struct hlist_head locked_down;
+	struct hlist_head anon_file_init;
 #ifdef CONFIG_PERF_EVENTS
 	struct hlist_head perf_event_open;
 	struct hlist_head perf_event_alloc;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..6ecef70566c1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -447,6 +447,8 @@  int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
 int security_locked_down(enum lockdown_reason what);
+void security_anon_file_init(struct file *file, const struct file *related,
+			const char *name);
 #else /* CONFIG_SECURITY */
 
 static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1274,6 +1276,12 @@  static inline int security_locked_down(enum lockdown_reason what)
 {
 	return 0;
 }
+static inline void security_anon_file_init(struct file *file,
+					   const struct file *related,
+					   const char *name);
+{
+	return 0;
+}
 #endif	/* CONFIG_SECURITY */
 
 #ifdef CONFIG_SECURITY_NETWORK
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..ce5d24c6cae8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2437,6 +2437,13 @@  int security_locked_down(enum lockdown_reason what)
 }
 EXPORT_SYMBOL(security_locked_down);
 
+void security_anon_file_init(struct file *file, const struct file *related,
+			const char *name)
+{
+	call_void_hook(anon_file_init, file, related, name);
+}
+EXPORT_SYMBOL(security_anon_file_init);
+
 #ifdef CONFIG_PERF_EVENTS
 int security_perf_event_open(struct perf_event_attr *attr, int type)
 {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 44f6f4e20cba..5193b93fae4f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1725,8 +1725,13 @@  static int file_has_perm(const struct cred *cred,
 
 	/* av is zero if only checking access to the descriptor. */
 	rc = 0;
-	if (av)
-		rc = inode_has_perm(cred, inode, av, &ad);
+	if (av) {
+		if (unlikely(fsec->anon))
+			rc = avc_has_perm(&selinux_state, sid, fsec->isid,
+					  fsec->anon_sclass, av, &ad);
+		else
+			rc = inode_has_perm(cred, inode, av, &ad);
+	}
 
 out:
 	return rc;
@@ -3558,6 +3563,8 @@  static int ioctl_has_perm(const struct cred *cred, struct file *file,
 	struct inode_security_struct *isec;
 	struct lsm_ioctlop_audit ioctl;
 	u32 ssid = cred_sid(cred);
+	u32 tsid;
+	u16 tclass;
 	int rc;
 	u8 driver = cmd >> 8;
 	u8 xperm = cmd & 0xff;
@@ -3577,12 +3584,21 @@  static int ioctl_has_perm(const struct cred *cred, struct file *file,
 			goto out;
 	}
 
-	if (unlikely(IS_PRIVATE(inode)))
-		return 0;
+	if (unlikely(fsec->anon)) {
+		tsid = fsec->isid;
+		tclass = fsec->anon_sclass;
+	} else {
+		if (unlikely(IS_PRIVATE(inode)))
+			return 0;
+
+		isec = inode_security(inode);
+
+		tsid = isec->sid;
+		tclass = isec->sclass;
+	}
 
-	isec = inode_security(inode);
 	rc = avc_has_extended_perms(&selinux_state,
-				    ssid, isec->sid, isec->sclass,
+				    ssid, tsid, tclass,
 				    requested, driver, xperm, &ad);
 out:
 	return rc;
@@ -6805,6 +6821,46 @@  static int selinux_lockdown(enum lockdown_reason what)
 				    LOCKDOWN__CONFIDENTIALITY, &ad);
 }
 
+static void selinux_anon_file_init(struct file *file,
+				const struct file *related, const char *name)
+{
+	struct file_security_struct *fsec = selinux_file(file);
+	const struct file_security_struct *pfsec;
+	const struct inode *pinode;
+	const struct inode_security_struct *pisec;
+	u32 sid;
+	struct qstr qname = {{{0}}, name};
+	int error;
+
+	fsec->anon = true;
+
+	if (related) {
+		pfsec = selinux_file(related);
+
+		if (pfsec->anon) {
+			fsec->isid = pfsec->isid;
+			fsec->anon_sclass = pfsec->anon_sclass;
+		} else {
+			pinode = file_inode(related);
+			pisec = selinux_inode(pinode);
+
+			fsec->isid = pisec->sid;
+			fsec->anon_sclass = pisec->sclass;
+		}
+	} else {
+		fsec->isid = SECINITSID_UNLABELED;
+		fsec->anon_sclass = SECCLASS_FILE;
+
+		sid = cred_sid(current_cred());
+
+		error = security_transition_sid(&selinux_state, sid, sid,
+						SECCLASS_FILE, &qname,
+						&fsec->isid);
+		if (error)
+			fsec->isid = SECINITSID_UNLABELED;
+	}
+}
+
 struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
 	.lbs_cred = sizeof(struct task_security_struct),
 	.lbs_file = sizeof(struct file_security_struct),
@@ -7108,6 +7164,7 @@  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 #endif
 
 	LSM_HOOK_INIT(locked_down, selinux_lockdown),
+	LSM_HOOK_INIT(anon_file_init, selinux_anon_file_init),
 
 	/*
 	 * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 330b7b6d44e0..8aa620e40a46 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -57,6 +57,10 @@  struct file_security_struct {
 	u32 sid;		/* SID of open file description */
 	u32 fown_sid;		/* SID of file owner (for SIGIO) */
 	u32 isid;		/* SID of inode at the time of file open */
+	bool anon;              /* Is file connected to an anonymous inode */
+	u16 anon_sclass;        /* inode security class if connected
+				 * to an anonymous inode
+				 */
 	u32 pseqno;		/* Policy seqno at the time of file open */
 };
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 67ae2d5c37b2..ad9161de6273 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2756,12 +2756,13 @@  static struct file_operations kvm_vcpu_fops = {
 /*
  * Allocates an inode for the vcpu.
  */
-static int create_vcpu_fd(struct kvm_vcpu *vcpu)
+static int create_vcpu_fd(struct kvm_vcpu *vcpu, const struct file *filp)
 {
 	char name[8 + 1 + ITOA_MAX_LEN + 1];
 
 	snprintf(name, sizeof(name), "kvm-vcpu:%d", vcpu->vcpu_id);
-	return anon_inode_getfd(name, &kvm_vcpu_fops, vcpu, O_RDWR | O_CLOEXEC);
+	return anon_inode_getfd_inherit(name, &kvm_vcpu_fops, vcpu,
+					O_RDWR | O_CLOEXEC, filp);
 }
 
 static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
@@ -2783,7 +2784,8 @@  static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 /*
  * Creates some virtual cpus.  Good luck creating more than one.
  */
-static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
+static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id,
+				    const struct file *filp)
 {
 	int r;
 	struct kvm_vcpu *vcpu;
@@ -2838,7 +2840,7 @@  static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 
 	/* Now it's all set up, let userspace reach it */
 	kvm_get_kvm(kvm);
-	r = create_vcpu_fd(vcpu);
+	r = create_vcpu_fd(vcpu, filp);
 	if (r < 0) {
 		kvm_put_kvm_no_destroy(kvm);
 		goto unlock_vcpu_destroy;
@@ -3239,7 +3241,8 @@  void kvm_unregister_device_ops(u32 type)
 }
 
 static int kvm_ioctl_create_device(struct kvm *kvm,
-				   struct kvm_create_device *cd)
+				   struct kvm_create_device *cd,
+				   const struct file *filp)
 {
 	const struct kvm_device_ops *ops = NULL;
 	struct kvm_device *dev;
@@ -3279,7 +3282,8 @@  static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops->init(dev);
 
 	kvm_get_kvm(kvm);
-	ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
+	ret = anon_inode_getfd_inherit(ops->name, &kvm_device_fops, dev,
+				       O_RDWR | O_CLOEXEC, filp);
 	if (ret < 0) {
 		kvm_put_kvm_no_destroy(kvm);
 		mutex_lock(&kvm->lock);
@@ -3369,7 +3373,7 @@  static long kvm_vm_ioctl(struct file *filp,
 		return -EIO;
 	switch (ioctl) {
 	case KVM_CREATE_VCPU:
-		r = kvm_vm_ioctl_create_vcpu(kvm, arg);
+		r = kvm_vm_ioctl_create_vcpu(kvm, arg, filp);
 		break;
 	case KVM_ENABLE_CAP: {
 		struct kvm_enable_cap cap;
@@ -3526,7 +3530,7 @@  static long kvm_vm_ioctl(struct file *filp,
 		if (copy_from_user(&cd, argp, sizeof(cd)))
 			goto out;
 
-		r = kvm_ioctl_create_device(kvm, &cd);
+		r = kvm_ioctl_create_device(kvm, &cd, filp);
 		if (r)
 			goto out;
 
@@ -3595,7 +3599,7 @@  static struct file_operations kvm_vm_fops = {
 	KVM_COMPAT(kvm_vm_compat_ioctl),
 };
 
-static int kvm_dev_ioctl_create_vm(unsigned long type)
+static int kvm_dev_ioctl_create_vm(unsigned long type, const struct file *filp)
 {
 	int r;
 	struct kvm *kvm;
@@ -3613,7 +3617,8 @@  static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (r < 0)
 		goto put_kvm;
 
-	file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
+	file = anon_inode_getfile_inherit("kvm-vm", &kvm_vm_fops, kvm, O_RDWR,
+					  filp);
 	if (IS_ERR(file)) {
 		put_unused_fd(r);
 		r = PTR_ERR(file);
@@ -3653,7 +3658,7 @@  static long kvm_dev_ioctl(struct file *filp,
 		r = KVM_API_VERSION;
 		break;
 	case KVM_CREATE_VM:
-		r = kvm_dev_ioctl_create_vm(arg);
+		r = kvm_dev_ioctl_create_vm(arg, filp);
 		break;
 	case KVM_CHECK_EXTENSION:
 		r = kvm_vm_ioctl_check_extension_generic(NULL, arg);