| Message ID | 20200213194157.5877-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] security,anon_inodes,kvm: enable security support for anon inodes | expand |
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
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?
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.
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?
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?
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.
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?
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.
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);
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(-)