| Message ID | 20200211225547.235083-4-dancol@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Harden userfaultfd | expand |
On 2/11/20 5:55 PM, Daniel Colascione wrote: > Use the secure anonymous inode LSM hook we just added to let SELinux > policy place restrictions on userfaultfd use. The create operation > applies to processes creating new instances of these file objects; > transfer between processes is covered by restrictions on read, write, > and ioctl access already checked inside selinux_file_receive. > > Signed-off-by: Daniel Colascione <dancol@google.com> (please add linux-fsdevel and viro to the cc for future versions of this patch since it changes the VFS) > --- > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 1659b59fb5d7..e178f6f40e93 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, > return 0; > } > > +static int selinux_inode_init_security_anon(struct inode *inode, > + const char *name, > + const struct file_operations *fops) > +{ > + const struct task_security_struct *tsec = selinux_cred(current_cred()); > + struct common_audit_data ad; > + struct inode_security_struct *isec; > + > + if (unlikely(IS_PRIVATE(inode))) > + return 0; Seems like this is precluded by the caller and would be a bug? If needed at all, take it to the security_inode_init_security_anon() so it doesn't have to be repeated in each security module. > + > + /* > + * We shouldn't be creating secure anonymous inodes before LSM > + * initialization completes. > + */ > + if (unlikely(!selinux_state.initialized)) > + return -EBUSY; I don't think this is viable; any arbitrary actions are possible before policy is loaded, and a Linux distro can be brought up fully with SELinux enabled and no policy loaded. You'll just need to have a default behavior prior to initialization. > + > + isec = selinux_inode(inode); > + > + /* > + * We only get here once per ephemeral inode. The inode has > + * been initialized via inode_alloc_security but is otherwise > + * untouched, so check that the state is as > + * inode_alloc_security left it. > + */ > + BUG_ON(isec->initialized != LABEL_INVALID); > + BUG_ON(isec->sclass != SECCLASS_FILE); I think the kernel discourages overuse of BUG_ON/BUG/... > + > +#ifdef CONFIG_USERFAULTFD > + if (fops == &userfaultfd_fops) > + isec->sclass = SECCLASS_UFFD; > +#endif Not sure we want or need to introduce a new security class for each user of anonymous inodes since the permissions should be the same as for file. Also not sure we want to be testing fops for each such case. We were looking at possibly leveraging the name as a key and using security_transition_sid() to generate a distinct SID/context/type for the inode via type_transition rules in policy. We have some WIP along those lines. > + > + if (isec->sclass == SECCLASS_FILE) { > + printk(KERN_WARNING "refusing to create secure anonymous inode " > + "of unknown type"); > + return -EOPNOTSUPP; > + } > + /* > + * Always give secure anonymous inodes the sid of the > + * creating task. > + */ > + > + isec->sid = tsec->sid; This doesn't generalize for other users of anonymous inodes, e.g. the /dev/kvm case where we'd rather inherit the SID and class from the original /dev/kvm inode itself.
Thanks for taking a look. On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 2/11/20 5:55 PM, Daniel Colascione wrote: > > Use the secure anonymous inode LSM hook we just added to let SELinux > > policy place restrictions on userfaultfd use. The create operation > > applies to processes creating new instances of these file objects; > > transfer between processes is covered by restrictions on read, write, > > and ioctl access already checked inside selinux_file_receive. > > > > Signed-off-by: Daniel Colascione <dancol@google.com> > > (please add linux-fsdevel and viro to the cc for future versions of this > patch since it changes the VFS) > > > --- > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 1659b59fb5d7..e178f6f40e93 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, > > return 0; > > } > > > > +static int selinux_inode_init_security_anon(struct inode *inode, > > + const char *name, > > + const struct file_operations *fops) > > +{ > > + const struct task_security_struct *tsec = selinux_cred(current_cred()); > > + struct common_audit_data ad; > > + struct inode_security_struct *isec; > > + > > + if (unlikely(IS_PRIVATE(inode))) > > + return 0; > > Seems like this is precluded by the caller and would be a bug? If > needed at all, take it to the security_inode_init_security_anon() so it > doesn't have to be repeated in each security module. > > > + > > + /* > > + * We shouldn't be creating secure anonymous inodes before LSM > > + * initialization completes. > > + */ > > + if (unlikely(!selinux_state.initialized)) > > + return -EBUSY; > > I don't think this is viable; any arbitrary actions are possible before > policy is loaded, and a Linux distro can be brought up fully with > SELinux enabled and no policy loaded. You'll just need to have a > default behavior prior to initialization. We'd have to fail open then, I think, and return an S_PRIVATE inode (the regular anon inode). > > + > > + isec = selinux_inode(inode); > > + > > + /* > > + * We only get here once per ephemeral inode. The inode has > > + * been initialized via inode_alloc_security but is otherwise > > + * untouched, so check that the state is as > > + * inode_alloc_security left it. > > + */ > > + BUG_ON(isec->initialized != LABEL_INVALID); > > + BUG_ON(isec->sclass != SECCLASS_FILE); > > I think the kernel discourages overuse of BUG_ON/BUG/... I'm not sure what counts as overuse. > > + > > +#ifdef CONFIG_USERFAULTFD > > + if (fops == &userfaultfd_fops) > > + isec->sclass = SECCLASS_UFFD; > > +#endif > > Not sure we want or need to introduce a new security class for each user > of anonymous inodes since the permissions should be the same as for > file. The purpose of this change is to apply special policy to userfaultfd FDs in particular. Isn't having a UFFD security class the best way to go about that? (There's no path.) Am I missing something? > Also not sure we want to be testing fops for each such case. I was also thinking of just providing some kind of context string (maybe the name), which might be friendlier to modules, but the loose coupling kind of scares me, and for this particular application, since UFFD is always in the core and never in a module, checking the fops seems a bit more robust and doesn't hurt anything. > We > were looking at possibly leveraging the name as a key and using > security_transition_sid() to generate a distinct SID/context/type for > the inode via type_transition rules in policy. We have some WIP along > those lines. Where? Any chance it would be ready soon? I'd rather not hold up this work for a more general mechanism. > > + > > + if (isec->sclass == SECCLASS_FILE) { > > + printk(KERN_WARNING "refusing to create secure anonymous inode " > > + "of unknown type"); > > + return -EOPNOTSUPP; > > + } > > + /* > > + * Always give secure anonymous inodes the sid of the > > + * creating task. > > + */ > > + > > + isec->sid = tsec->sid; > > This doesn't generalize for other users of anonymous inodes, e.g. the > /dev/kvm case where we'd rather inherit the SID and class from the > original /dev/kvm inode itself. I think someone mentioned on the first version of this patch that we could make it more flexible if the need arose. If we do want to do it now, we could have the anon_inode security hook accept a "parent" or "context" inode that modules could inspect for the purposes of forming the new inode's SID. Does that make sense to you?
On 2/12/20 12:19 PM, Daniel Colascione wrote: > Thanks for taking a look. > > On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> >> On 2/11/20 5:55 PM, Daniel Colascione wrote: >>> Use the secure anonymous inode LSM hook we just added to let SELinux >>> policy place restrictions on userfaultfd use. The create operation >>> applies to processes creating new instances of these file objects; >>> transfer between processes is covered by restrictions on read, write, >>> and ioctl access already checked inside selinux_file_receive. >>> >>> Signed-off-by: Daniel Colascione <dancol@google.com> >> >> (please add linux-fsdevel and viro to the cc for future versions of this >> patch since it changes the VFS) >> >>> --- >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>> index 1659b59fb5d7..e178f6f40e93 100644 >>> --- a/security/selinux/hooks.c >>> +++ b/security/selinux/hooks.c >>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, >>> + >>> + /* >>> + * We shouldn't be creating secure anonymous inodes before LSM >>> + * initialization completes. >>> + */ >>> + if (unlikely(!selinux_state.initialized)) >>> + return -EBUSY; >> >> I don't think this is viable; any arbitrary actions are possible before >> policy is loaded, and a Linux distro can be brought up fully with >> SELinux enabled and no policy loaded. You'll just need to have a >> default behavior prior to initialization. > > We'd have to fail open then, I think, and return an S_PRIVATE inode > (the regular anon inode). Not sure why. You aren't doing anything in the hook that actually relies on selinux_state.initialized being set (i.e. nothing requires a policy). The avc_has_perm() call will just succeed until a policy is loaded. So if these inodes are created prior to policy load, they will get assigned the task SID (which would be the kernel SID prior to policy load or first exec or write to /proc/self/attr/current afterward) and UFFD class (in your current code), be permitted, and then once policy is loaded any further access will get checked against the kernel SID. >>> + /* >>> + * We only get here once per ephemeral inode. The inode has >>> + * been initialized via inode_alloc_security but is otherwise >>> + * untouched, so check that the state is as >>> + * inode_alloc_security left it. >>> + */ >>> + BUG_ON(isec->initialized != LABEL_INVALID); >>> + BUG_ON(isec->sclass != SECCLASS_FILE); >> >> I think the kernel discourages overuse of BUG_ON/BUG/... > > I'm not sure what counts as overuse. Me either (not my rule) but I'm pretty sure this counts or you'd see a lot more of these kinds of BUG_ON() checks throughout. Try to reserve them for really critical cases. >>> + >>> +#ifdef CONFIG_USERFAULTFD >>> + if (fops == &userfaultfd_fops) >>> + isec->sclass = SECCLASS_UFFD; >>> +#endif >> >> Not sure we want or need to introduce a new security class for each user >> of anonymous inodes since the permissions should be the same as for >> file. > > The purpose of this change is to apply special policy to userfaultfd > FDs in particular. Isn't having a UFFD security class the best way to > go about that? (There's no path.) Am I missing something? It is probably the simplest approach; it just doesn't generalize to all users of anonymous inodes. We can distinguish them in one of two ways: use a different class like you did (requires a code change every time we add a new one and yet another duplicate of the file class) or use a different SID/context/type. The latter could be achieved by calling security_transition_sid() with the provided name wrapped in a qstr and specifying type_transition rules on the name. Then policy could define derived types for each domain, ala type_transition init self:file "[userfaultfd]" init_userfaultfd; type_transition untrusted_app self:file "[userfaultfd]" untrusted_app_userfaultfd; ... >> Also not sure we want to be testing fops for each such case. > > I was also thinking of just providing some kind of context string > (maybe the name), which might be friendlier to modules, but the loose > coupling kind of scares me, and for this particular application, since > UFFD is always in the core and never in a module, checking the fops > seems a bit more robust and doesn't hurt anything. Yes, not sure how the vfs folks feel about either coupling (the name-based one or the fops-based one). Neither seems great. >> We >> were looking at possibly leveraging the name as a key and using >> security_transition_sid() to generate a distinct SID/context/type for >> the inode via type_transition rules in policy. We have some WIP along >> those lines. > > Where? Any chance it would be ready soon? I'd rather not hold up this > work for a more general mechanism. Hopefully will have a patch available soon. But not saying this necessarily has to wait either. >>> + /* >>> + * Always give secure anonymous inodes the sid of the >>> + * creating task. >>> + */ >>> + >>> + isec->sid = tsec->sid; >> >> This doesn't generalize for other users of anonymous inodes, e.g. the >> /dev/kvm case where we'd rather inherit the SID and class from the >> original /dev/kvm inode itself. > > I think someone mentioned on the first version of this patch that we > could make it more flexible if the need arose. If we do want to do it > now, we could have the anon_inode security hook accept a "parent" or > "context" inode that modules could inspect for the purposes of forming > the new inode's SID. Does that make sense to you? Yes, that's the approach in our current WIP, except we call it a "related" inode since it isn't necessarily connected to the anon inode in any vfs sense.
On 2/12/20 1:04 PM, Stephen Smalley wrote: > On 2/12/20 12:19 PM, Daniel Colascione wrote: >> Thanks for taking a look. >> >> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> >> wrote: >>> >>> On 2/11/20 5:55 PM, Daniel Colascione wrote: >>>> Use the secure anonymous inode LSM hook we just added to let SELinux >>>> policy place restrictions on userfaultfd use. The create operation >>>> applies to processes creating new instances of these file objects; >>>> transfer between processes is covered by restrictions on read, write, >>>> and ioctl access already checked inside selinux_file_receive. >>>> >>>> Signed-off-by: Daniel Colascione <dancol@google.com> >>> >>> (please add linux-fsdevel and viro to the cc for future versions of this >>> patch since it changes the VFS) >>> >>>> --- >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>> index 1659b59fb5d7..e178f6f40e93 100644 >>>> --- a/security/selinux/hooks.c >>>> +++ b/security/selinux/hooks.c >>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct >>>> inode *inode, struct inode *dir, >>>> + >>>> + /* >>>> + * We shouldn't be creating secure anonymous inodes before LSM >>>> + * initialization completes. >>>> + */ >>>> + if (unlikely(!selinux_state.initialized)) >>>> + return -EBUSY; >>> >>> I don't think this is viable; any arbitrary actions are possible before >>> policy is loaded, and a Linux distro can be brought up fully with >>> SELinux enabled and no policy loaded. You'll just need to have a >>> default behavior prior to initialization. >> >> We'd have to fail open then, I think, and return an S_PRIVATE inode >> (the regular anon inode). > > Not sure why. You aren't doing anything in the hook that actually > relies on selinux_state.initialized being set (i.e. nothing requires a > policy). The avc_has_perm() call will just succeed until a policy is > loaded. So if these inodes are created prior to policy load, they will > get assigned the task SID (which would be the kernel SID prior to policy > load or first exec or write to /proc/self/attr/current afterward) and > UFFD class (in your current code), be permitted, and then once policy is > loaded any further access will get checked against the kernel SID. > >>>> + /* >>>> + * We only get here once per ephemeral inode. The inode has >>>> + * been initialized via inode_alloc_security but is otherwise >>>> + * untouched, so check that the state is as >>>> + * inode_alloc_security left it. >>>> + */ >>>> + BUG_ON(isec->initialized != LABEL_INVALID); >>>> + BUG_ON(isec->sclass != SECCLASS_FILE); >>> >>> I think the kernel discourages overuse of BUG_ON/BUG/... >> >> I'm not sure what counts as overuse. > > Me either (not my rule) but I'm pretty sure this counts or you'd see a > lot more of these kinds of BUG_ON() checks throughout. Try to reserve > them for really critical cases. > >>>> + >>>> +#ifdef CONFIG_USERFAULTFD >>>> + if (fops == &userfaultfd_fops) >>>> + isec->sclass = SECCLASS_UFFD; >>>> +#endif >>> >>> Not sure we want or need to introduce a new security class for each user >>> of anonymous inodes since the permissions should be the same as for >>> file. >> >> The purpose of this change is to apply special policy to userfaultfd >> FDs in particular. Isn't having a UFFD security class the best way to >> go about that? (There's no path.) Am I missing something? > > It is probably the simplest approach; it just doesn't generalize to all > users of anonymous inodes. We can distinguish them in one of two ways: > use a different class like you did (requires a code change every time we > add a new one and yet another duplicate of the file class) or use a > different SID/context/type. The latter could be achieved by calling > security_transition_sid() with the provided name wrapped in a qstr and > specifying type_transition rules on the name. Then policy could define > derived types for each domain, ala > type_transition init self:file "[userfaultfd]" init_userfaultfd; > type_transition untrusted_app self:file "[userfaultfd]" > untrusted_app_userfaultfd; > ... > >>> Also not sure we want to be testing fops for each such case. >> >> I was also thinking of just providing some kind of context string >> (maybe the name), which might be friendlier to modules, but the loose >> coupling kind of scares me, and for this particular application, since >> UFFD is always in the core and never in a module, checking the fops >> seems a bit more robust and doesn't hurt anything. > > Yes, not sure how the vfs folks feel about either coupling (the > name-based one or the fops-based one). Neither seems great. > >>> We >>> were looking at possibly leveraging the name as a key and using >>> security_transition_sid() to generate a distinct SID/context/type for >>> the inode via type_transition rules in policy. We have some WIP along >>> those lines. >> >> Where? Any chance it would be ready soon? I'd rather not hold up this >> work for a more general mechanism. > > Hopefully will have a patch available soon. But not saying this > necessarily has to wait either. > >>>> + /* >>>> + * Always give secure anonymous inodes the sid of the >>>> + * creating task. >>>> + */ >>>> + >>>> + isec->sid = tsec->sid; >>> >>> This doesn't generalize for other users of anonymous inodes, e.g. the >>> /dev/kvm case where we'd rather inherit the SID and class from the >>> original /dev/kvm inode itself. >> >> I think someone mentioned on the first version of this patch that we >> could make it more flexible if the need arose. If we do want to do it >> now, we could have the anon_inode security hook accept a "parent" or >> "context" inode that modules could inspect for the purposes of forming >> the new inode's SID. Does that make sense to you? > > Yes, that's the approach in our current WIP, except we call it a > "related" inode since it isn't necessarily connected to the anon inode > in any vfs sense. The other key difference in our WIP approach is that we assumed that we couldn't mandate allocating a separate anon inode for each of these fds and we wanted to cover all anonymous inodes (not opt-in), so we are storing the SID/class pair as additional fields in the file_security_struct and have modified file_has_perm() and others to look there for anonymous inodes.
On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 2/12/20 1:04 PM, Stephen Smalley wrote: > > On 2/12/20 12:19 PM, Daniel Colascione wrote: > >> Thanks for taking a look. > >> > >> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> > >> wrote: > >>> > >>> On 2/11/20 5:55 PM, Daniel Colascione wrote: > >>>> Use the secure anonymous inode LSM hook we just added to let SELinux > >>>> policy place restrictions on userfaultfd use. The create operation > >>>> applies to processes creating new instances of these file objects; > >>>> transfer between processes is covered by restrictions on read, write, > >>>> and ioctl access already checked inside selinux_file_receive. > >>>> > >>>> Signed-off-by: Daniel Colascione <dancol@google.com> > >>> > >>> (please add linux-fsdevel and viro to the cc for future versions of this > >>> patch since it changes the VFS) > >>> > >>>> --- > >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >>>> index 1659b59fb5d7..e178f6f40e93 100644 > >>>> --- a/security/selinux/hooks.c > >>>> +++ b/security/selinux/hooks.c > >>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct > >>>> inode *inode, struct inode *dir, > >>>> + > >>>> + /* > >>>> + * We shouldn't be creating secure anonymous inodes before LSM > >>>> + * initialization completes. > >>>> + */ > >>>> + if (unlikely(!selinux_state.initialized)) > >>>> + return -EBUSY; > >>> > >>> I don't think this is viable; any arbitrary actions are possible before > >>> policy is loaded, and a Linux distro can be brought up fully with > >>> SELinux enabled and no policy loaded. You'll just need to have a > >>> default behavior prior to initialization. > >> > >> We'd have to fail open then, I think, and return an S_PRIVATE inode > >> (the regular anon inode). > > > > Not sure why. You aren't doing anything in the hook that actually > > relies on selinux_state.initialized being set (i.e. nothing requires a > > policy). The avc_has_perm() call will just succeed until a policy is > > loaded. So if these inodes are created prior to policy load, they will > > get assigned the task SID (which would be the kernel SID prior to policy > > load or first exec or write to /proc/self/attr/current afterward) and > > UFFD class (in your current code), be permitted, and then once policy is > > loaded any further access will get checked against the kernel SID. > > > >>>> + /* > >>>> + * We only get here once per ephemeral inode. The inode has > >>>> + * been initialized via inode_alloc_security but is otherwise > >>>> + * untouched, so check that the state is as > >>>> + * inode_alloc_security left it. > >>>> + */ > >>>> + BUG_ON(isec->initialized != LABEL_INVALID); > >>>> + BUG_ON(isec->sclass != SECCLASS_FILE); > >>> > >>> I think the kernel discourages overuse of BUG_ON/BUG/... > >> > >> I'm not sure what counts as overuse. > > > > Me either (not my rule) but I'm pretty sure this counts or you'd see a > > lot more of these kinds of BUG_ON() checks throughout. Try to reserve > > them for really critical cases. > > > >>>> + > >>>> +#ifdef CONFIG_USERFAULTFD > >>>> + if (fops == &userfaultfd_fops) > >>>> + isec->sclass = SECCLASS_UFFD; > >>>> +#endif > >>> > >>> Not sure we want or need to introduce a new security class for each user > >>> of anonymous inodes since the permissions should be the same as for > >>> file. > >> > >> The purpose of this change is to apply special policy to userfaultfd > >> FDs in particular. Isn't having a UFFD security class the best way to > >> go about that? (There's no path.) Am I missing something? > > > > It is probably the simplest approach; it just doesn't generalize to all > > users of anonymous inodes. We can distinguish them in one of two ways: > > use a different class like you did (requires a code change every time we > > add a new one and yet another duplicate of the file class) or use a > > different SID/context/type. The latter could be achieved by calling > > security_transition_sid() with the provided name wrapped in a qstr and > > specifying type_transition rules on the name. Then policy could define > > derived types for each domain, ala > > type_transition init self:file "[userfaultfd]" init_userfaultfd; > > type_transition untrusted_app self:file "[userfaultfd]" > > untrusted_app_userfaultfd; > > ... > > > >>> Also not sure we want to be testing fops for each such case. > >> > >> I was also thinking of just providing some kind of context string > >> (maybe the name), which might be friendlier to modules, but the loose > >> coupling kind of scares me, and for this particular application, since > >> UFFD is always in the core and never in a module, checking the fops > >> seems a bit more robust and doesn't hurt anything. > > > > Yes, not sure how the vfs folks feel about either coupling (the > > name-based one or the fops-based one). Neither seems great. > > > >>> We > >>> were looking at possibly leveraging the name as a key and using > >>> security_transition_sid() to generate a distinct SID/context/type for > >>> the inode via type_transition rules in policy. We have some WIP along > >>> those lines. > >> > >> Where? Any chance it would be ready soon? I'd rather not hold up this > >> work for a more general mechanism. > > > > Hopefully will have a patch available soon. But not saying this > > necessarily has to wait either. > > > >>>> + /* > >>>> + * Always give secure anonymous inodes the sid of the > >>>> + * creating task. > >>>> + */ > >>>> + > >>>> + isec->sid = tsec->sid; > >>> > >>> This doesn't generalize for other users of anonymous inodes, e.g. the > >>> /dev/kvm case where we'd rather inherit the SID and class from the > >>> original /dev/kvm inode itself. > >> > >> I think someone mentioned on the first version of this patch that we > >> could make it more flexible if the need arose. If we do want to do it > >> now, we could have the anon_inode security hook accept a "parent" or > >> "context" inode that modules could inspect for the purposes of forming > >> the new inode's SID. Does that make sense to you? > > > > Yes, that's the approach in our current WIP, except we call it a > > "related" inode since it isn't necessarily connected to the anon inode > > in any vfs sense. > > The other key difference in our WIP approach is that we assumed that we > couldn't mandate allocating a separate anon inode for each of these fds > and we wanted to cover all anonymous inodes (not opt-in), so we are > storing the SID/class pair as additional fields in the > file_security_struct and have modified file_has_perm() and others to > look there for anonymous inodes. A separate inode seems like the simpler approach for now, because it means that we have fewer places to check for security information --- and it's not as if an inode is particularly expensive. We can always switch later.
On 2/12/20 2:04 PM, Daniel Colascione wrote: > On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> >> On 2/12/20 1:04 PM, Stephen Smalley wrote: >>> On 2/12/20 12:19 PM, Daniel Colascione wrote: >>>> Thanks for taking a look. >>>> >>>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> >>>> wrote: >>>>> >>>>> On 2/11/20 5:55 PM, Daniel Colascione wrote: >>>>>> Use the secure anonymous inode LSM hook we just added to let SELinux >>>>>> policy place restrictions on userfaultfd use. The create operation >>>>>> applies to processes creating new instances of these file objects; >>>>>> transfer between processes is covered by restrictions on read, write, >>>>>> and ioctl access already checked inside selinux_file_receive. >>>>>> >>>>>> Signed-off-by: Daniel Colascione <dancol@google.com> >>>>> >>>>> (please add linux-fsdevel and viro to the cc for future versions of this >>>>> patch since it changes the VFS) >>>>> >>>>>> --- >>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>>>> index 1659b59fb5d7..e178f6f40e93 100644 >>>>>> --- a/security/selinux/hooks.c >>>>>> +++ b/security/selinux/hooks.c >>>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct >>>>>> inode *inode, struct inode *dir, >>>>>> + >>>>>> + /* >>>>>> + * We shouldn't be creating secure anonymous inodes before LSM >>>>>> + * initialization completes. >>>>>> + */ >>>>>> + if (unlikely(!selinux_state.initialized)) >>>>>> + return -EBUSY; >>>>> >>>>> I don't think this is viable; any arbitrary actions are possible before >>>>> policy is loaded, and a Linux distro can be brought up fully with >>>>> SELinux enabled and no policy loaded. You'll just need to have a >>>>> default behavior prior to initialization. >>>> >>>> We'd have to fail open then, I think, and return an S_PRIVATE inode >>>> (the regular anon inode). >>> >>> Not sure why. You aren't doing anything in the hook that actually >>> relies on selinux_state.initialized being set (i.e. nothing requires a >>> policy). The avc_has_perm() call will just succeed until a policy is >>> loaded. So if these inodes are created prior to policy load, they will >>> get assigned the task SID (which would be the kernel SID prior to policy >>> load or first exec or write to /proc/self/attr/current afterward) and >>> UFFD class (in your current code), be permitted, and then once policy is >>> loaded any further access will get checked against the kernel SID. >>> >>>>>> + /* >>>>>> + * We only get here once per ephemeral inode. The inode has >>>>>> + * been initialized via inode_alloc_security but is otherwise >>>>>> + * untouched, so check that the state is as >>>>>> + * inode_alloc_security left it. >>>>>> + */ >>>>>> + BUG_ON(isec->initialized != LABEL_INVALID); >>>>>> + BUG_ON(isec->sclass != SECCLASS_FILE); >>>>> >>>>> I think the kernel discourages overuse of BUG_ON/BUG/... >>>> >>>> I'm not sure what counts as overuse. >>> >>> Me either (not my rule) but I'm pretty sure this counts or you'd see a >>> lot more of these kinds of BUG_ON() checks throughout. Try to reserve >>> them for really critical cases. >>> >>>>>> + >>>>>> +#ifdef CONFIG_USERFAULTFD >>>>>> + if (fops == &userfaultfd_fops) >>>>>> + isec->sclass = SECCLASS_UFFD; >>>>>> +#endif >>>>> >>>>> Not sure we want or need to introduce a new security class for each user >>>>> of anonymous inodes since the permissions should be the same as for >>>>> file. >>>> >>>> The purpose of this change is to apply special policy to userfaultfd >>>> FDs in particular. Isn't having a UFFD security class the best way to >>>> go about that? (There's no path.) Am I missing something? >>> >>> It is probably the simplest approach; it just doesn't generalize to all >>> users of anonymous inodes. We can distinguish them in one of two ways: >>> use a different class like you did (requires a code change every time we >>> add a new one and yet another duplicate of the file class) or use a >>> different SID/context/type. The latter could be achieved by calling >>> security_transition_sid() with the provided name wrapped in a qstr and >>> specifying type_transition rules on the name. Then policy could define >>> derived types for each domain, ala >>> type_transition init self:file "[userfaultfd]" init_userfaultfd; >>> type_transition untrusted_app self:file "[userfaultfd]" >>> untrusted_app_userfaultfd; >>> ... >>> >>>>> Also not sure we want to be testing fops for each such case. >>>> >>>> I was also thinking of just providing some kind of context string >>>> (maybe the name), which might be friendlier to modules, but the loose >>>> coupling kind of scares me, and for this particular application, since >>>> UFFD is always in the core and never in a module, checking the fops >>>> seems a bit more robust and doesn't hurt anything. >>> >>> Yes, not sure how the vfs folks feel about either coupling (the >>> name-based one or the fops-based one). Neither seems great. >>> >>>>> We >>>>> were looking at possibly leveraging the name as a key and using >>>>> security_transition_sid() to generate a distinct SID/context/type for >>>>> the inode via type_transition rules in policy. We have some WIP along >>>>> those lines. >>>> >>>> Where? Any chance it would be ready soon? I'd rather not hold up this >>>> work for a more general mechanism. >>> >>> Hopefully will have a patch available soon. But not saying this >>> necessarily has to wait either. >>> >>>>>> + /* >>>>>> + * Always give secure anonymous inodes the sid of the >>>>>> + * creating task. >>>>>> + */ >>>>>> + >>>>>> + isec->sid = tsec->sid; >>>>> >>>>> This doesn't generalize for other users of anonymous inodes, e.g. the >>>>> /dev/kvm case where we'd rather inherit the SID and class from the >>>>> original /dev/kvm inode itself. >>>> >>>> I think someone mentioned on the first version of this patch that we >>>> could make it more flexible if the need arose. If we do want to do it >>>> now, we could have the anon_inode security hook accept a "parent" or >>>> "context" inode that modules could inspect for the purposes of forming >>>> the new inode's SID. Does that make sense to you? >>> >>> Yes, that's the approach in our current WIP, except we call it a >>> "related" inode since it isn't necessarily connected to the anon inode >>> in any vfs sense. >> >> The other key difference in our WIP approach is that we assumed that we >> couldn't mandate allocating a separate anon inode for each of these fds >> and we wanted to cover all anonymous inodes (not opt-in), so we are >> storing the SID/class pair as additional fields in the >> file_security_struct and have modified file_has_perm() and others to >> look there for anonymous inodes. > > A separate inode seems like the simpler approach for now, because it > means that we have fewer places to check for security information --- > and it's not as if an inode is particularly expensive. We can always > switch later. We'd prefer having a separate inode if possible but didn't think that would fly with the vfs folks, especially if we try to apply this to all anonymous inodes. It might be ok for userfaultfd usage as a specific case but there is a reason why anonymous inodes were introduced and creating a separate inode each time defeats that purpose IIUC. It will be interesting to see how they respond.
On Wed, Feb 12, 2020 at 11:10 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 2/12/20 2:04 PM, Daniel Colascione wrote: > > On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> > >> On 2/12/20 1:04 PM, Stephen Smalley wrote: > >>> On 2/12/20 12:19 PM, Daniel Colascione wrote: > >>>> Thanks for taking a look. > >>>> > >>>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> > >>>> wrote: > >>>>> > >>>>> On 2/11/20 5:55 PM, Daniel Colascione wrote: > >>>>>> Use the secure anonymous inode LSM hook we just added to let SELinux > >>>>>> policy place restrictions on userfaultfd use. The create operation > >>>>>> applies to processes creating new instances of these file objects; > >>>>>> transfer between processes is covered by restrictions on read, write, > >>>>>> and ioctl access already checked inside selinux_file_receive. > >>>>>> > >>>>>> Signed-off-by: Daniel Colascione <dancol@google.com> > >>>>> > >>>>> (please add linux-fsdevel and viro to the cc for future versions of this > >>>>> patch since it changes the VFS) > >>>>> > >>>>>> --- > >>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >>>>>> index 1659b59fb5d7..e178f6f40e93 100644 > >>>>>> --- a/security/selinux/hooks.c > >>>>>> +++ b/security/selinux/hooks.c > >>>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct > >>>>>> inode *inode, struct inode *dir, > >>>>>> + > >>>>>> + /* > >>>>>> + * We shouldn't be creating secure anonymous inodes before LSM > >>>>>> + * initialization completes. > >>>>>> + */ > >>>>>> + if (unlikely(!selinux_state.initialized)) > >>>>>> + return -EBUSY; > >>>>> > >>>>> I don't think this is viable; any arbitrary actions are possible before > >>>>> policy is loaded, and a Linux distro can be brought up fully with > >>>>> SELinux enabled and no policy loaded. You'll just need to have a > >>>>> default behavior prior to initialization. > >>>> > >>>> We'd have to fail open then, I think, and return an S_PRIVATE inode > >>>> (the regular anon inode). > >>> > >>> Not sure why. You aren't doing anything in the hook that actually > >>> relies on selinux_state.initialized being set (i.e. nothing requires a > >>> policy). The avc_has_perm() call will just succeed until a policy is > >>> loaded. So if these inodes are created prior to policy load, they will > >>> get assigned the task SID (which would be the kernel SID prior to policy > >>> load or first exec or write to /proc/self/attr/current afterward) and > >>> UFFD class (in your current code), be permitted, and then once policy is > >>> loaded any further access will get checked against the kernel SID. > >>> > >>>>>> + /* > >>>>>> + * We only get here once per ephemeral inode. The inode has > >>>>>> + * been initialized via inode_alloc_security but is otherwise > >>>>>> + * untouched, so check that the state is as > >>>>>> + * inode_alloc_security left it. > >>>>>> + */ > >>>>>> + BUG_ON(isec->initialized != LABEL_INVALID); > >>>>>> + BUG_ON(isec->sclass != SECCLASS_FILE); > >>>>> > >>>>> I think the kernel discourages overuse of BUG_ON/BUG/... > >>>> > >>>> I'm not sure what counts as overuse. > >>> > >>> Me either (not my rule) but I'm pretty sure this counts or you'd see a > >>> lot more of these kinds of BUG_ON() checks throughout. Try to reserve > >>> them for really critical cases. > >>> > >>>>>> + > >>>>>> +#ifdef CONFIG_USERFAULTFD > >>>>>> + if (fops == &userfaultfd_fops) > >>>>>> + isec->sclass = SECCLASS_UFFD; > >>>>>> +#endif > >>>>> > >>>>> Not sure we want or need to introduce a new security class for each user > >>>>> of anonymous inodes since the permissions should be the same as for > >>>>> file. > >>>> > >>>> The purpose of this change is to apply special policy to userfaultfd > >>>> FDs in particular. Isn't having a UFFD security class the best way to > >>>> go about that? (There's no path.) Am I missing something? > >>> > >>> It is probably the simplest approach; it just doesn't generalize to all > >>> users of anonymous inodes. We can distinguish them in one of two ways: > >>> use a different class like you did (requires a code change every time we > >>> add a new one and yet another duplicate of the file class) or use a > >>> different SID/context/type. The latter could be achieved by calling > >>> security_transition_sid() with the provided name wrapped in a qstr and > >>> specifying type_transition rules on the name. Then policy could define > >>> derived types for each domain, ala > >>> type_transition init self:file "[userfaultfd]" init_userfaultfd; > >>> type_transition untrusted_app self:file "[userfaultfd]" > >>> untrusted_app_userfaultfd; > >>> ... > >>> > >>>>> Also not sure we want to be testing fops for each such case. > >>>> > >>>> I was also thinking of just providing some kind of context string > >>>> (maybe the name), which might be friendlier to modules, but the loose > >>>> coupling kind of scares me, and for this particular application, since > >>>> UFFD is always in the core and never in a module, checking the fops > >>>> seems a bit more robust and doesn't hurt anything. > >>> > >>> Yes, not sure how the vfs folks feel about either coupling (the > >>> name-based one or the fops-based one). Neither seems great. > >>> > >>>>> We > >>>>> were looking at possibly leveraging the name as a key and using > >>>>> security_transition_sid() to generate a distinct SID/context/type for > >>>>> the inode via type_transition rules in policy. We have some WIP along > >>>>> those lines. > >>>> > >>>> Where? Any chance it would be ready soon? I'd rather not hold up this > >>>> work for a more general mechanism. > >>> > >>> Hopefully will have a patch available soon. But not saying this > >>> necessarily has to wait either. > >>> > >>>>>> + /* > >>>>>> + * Always give secure anonymous inodes the sid of the > >>>>>> + * creating task. > >>>>>> + */ > >>>>>> + > >>>>>> + isec->sid = tsec->sid; > >>>>> > >>>>> This doesn't generalize for other users of anonymous inodes, e.g. the > >>>>> /dev/kvm case where we'd rather inherit the SID and class from the > >>>>> original /dev/kvm inode itself. > >>>> > >>>> I think someone mentioned on the first version of this patch that we > >>>> could make it more flexible if the need arose. If we do want to do it > >>>> now, we could have the anon_inode security hook accept a "parent" or > >>>> "context" inode that modules could inspect for the purposes of forming > >>>> the new inode's SID. Does that make sense to you? > >>> > >>> Yes, that's the approach in our current WIP, except we call it a > >>> "related" inode since it isn't necessarily connected to the anon inode > >>> in any vfs sense. > >> > >> The other key difference in our WIP approach is that we assumed that we > >> couldn't mandate allocating a separate anon inode for each of these fds > >> and we wanted to cover all anonymous inodes (not opt-in), so we are > >> storing the SID/class pair as additional fields in the > >> file_security_struct and have modified file_has_perm() and others to > >> look there for anonymous inodes. > > > > A separate inode seems like the simpler approach for now, because it > > means that we have fewer places to check for security information --- > > and it's not as if an inode is particularly expensive. We can always > > switch later. > > We'd prefer having a separate inode if possible but didn't think that > would fly with the vfs folks, Let's ask them. > especially if we try to apply this to all > anonymous inodes. For the moment, we're not. > It might be ok for userfaultfd usage as a specific > case but there is a reason why anonymous inodes were introduced and > creating a separate inode each time defeats that purpose IIUC. It will > be interesting to see how they respond. Sort of. Anonymous inodes also free other parts of the kernel from having to deal with special-purpose filesystems (like pipefs) on which to hang custom inodes. It's just a generic "just give me an inode and I don't care about the filesystem" feature, and if we actually get a new inode each time, we still do the job. Pipe seems to be good with creating inodes each time.
On 2/12/20 2:11 PM, Stephen Smalley wrote: > On 2/12/20 2:04 PM, Daniel Colascione wrote: >> On Wed, Feb 12, 2020 at 10:59 AM Stephen Smalley <sds@tycho.nsa.gov> >> wrote: >>> >>> On 2/12/20 1:04 PM, Stephen Smalley wrote: >>>> On 2/12/20 12:19 PM, Daniel Colascione wrote: >>>>> Thanks for taking a look. >>>>> >>>>> On Wed, Feb 12, 2020 at 9:04 AM Stephen Smalley <sds@tycho.nsa.gov> >>>>> wrote: >>>>>> >>>>>> On 2/11/20 5:55 PM, Daniel Colascione wrote: >>>>>>> Use the secure anonymous inode LSM hook we just added to let SELinux >>>>>>> policy place restrictions on userfaultfd use. The create operation >>>>>>> applies to processes creating new instances of these file objects; >>>>>>> transfer between processes is covered by restrictions on read, >>>>>>> write, >>>>>>> and ioctl access already checked inside selinux_file_receive. >>>>>>> >>>>>>> Signed-off-by: Daniel Colascione <dancol@google.com> >>>>>> >>>>>> (please add linux-fsdevel and viro to the cc for future versions >>>>>> of this >>>>>> patch since it changes the VFS) >>>>>> >>>>>>> --- >>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>>>>> index 1659b59fb5d7..e178f6f40e93 100644 >>>>>>> --- a/security/selinux/hooks.c >>>>>>> +++ b/security/selinux/hooks.c >>>>>>> @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct >>>>>>> inode *inode, struct inode *dir, >>>>>>> + >>>>>>> + /* >>>>>>> + * We shouldn't be creating secure anonymous inodes before LSM >>>>>>> + * initialization completes. >>>>>>> + */ >>>>>>> + if (unlikely(!selinux_state.initialized)) >>>>>>> + return -EBUSY; >>>>>> >>>>>> I don't think this is viable; any arbitrary actions are possible >>>>>> before >>>>>> policy is loaded, and a Linux distro can be brought up fully with >>>>>> SELinux enabled and no policy loaded. You'll just need to have a >>>>>> default behavior prior to initialization. >>>>> >>>>> We'd have to fail open then, I think, and return an S_PRIVATE inode >>>>> (the regular anon inode). >>>> >>>> Not sure why. You aren't doing anything in the hook that actually >>>> relies on selinux_state.initialized being set (i.e. nothing requires a >>>> policy). The avc_has_perm() call will just succeed until a policy is >>>> loaded. So if these inodes are created prior to policy load, they will >>>> get assigned the task SID (which would be the kernel SID prior to >>>> policy >>>> load or first exec or write to /proc/self/attr/current afterward) and >>>> UFFD class (in your current code), be permitted, and then once >>>> policy is >>>> loaded any further access will get checked against the kernel SID. >>>> >>>>>>> + /* >>>>>>> + * We only get here once per ephemeral inode. The inode has >>>>>>> + * been initialized via inode_alloc_security but is otherwise >>>>>>> + * untouched, so check that the state is as >>>>>>> + * inode_alloc_security left it. >>>>>>> + */ >>>>>>> + BUG_ON(isec->initialized != LABEL_INVALID); >>>>>>> + BUG_ON(isec->sclass != SECCLASS_FILE); >>>>>> >>>>>> I think the kernel discourages overuse of BUG_ON/BUG/... >>>>> >>>>> I'm not sure what counts as overuse. >>>> >>>> Me either (not my rule) but I'm pretty sure this counts or you'd see a >>>> lot more of these kinds of BUG_ON() checks throughout. Try to reserve >>>> them for really critical cases. >>>> >>>>>>> + >>>>>>> +#ifdef CONFIG_USERFAULTFD >>>>>>> + if (fops == &userfaultfd_fops) >>>>>>> + isec->sclass = SECCLASS_UFFD; >>>>>>> +#endif >>>>>> >>>>>> Not sure we want or need to introduce a new security class for >>>>>> each user >>>>>> of anonymous inodes since the permissions should be the same as for >>>>>> file. >>>>> >>>>> The purpose of this change is to apply special policy to userfaultfd >>>>> FDs in particular. Isn't having a UFFD security class the best way to >>>>> go about that? (There's no path.) Am I missing something? >>>> >>>> It is probably the simplest approach; it just doesn't generalize to all >>>> users of anonymous inodes. We can distinguish them in one of two ways: >>>> use a different class like you did (requires a code change every >>>> time we >>>> add a new one and yet another duplicate of the file class) or use a >>>> different SID/context/type. The latter could be achieved by calling >>>> security_transition_sid() with the provided name wrapped in a qstr and >>>> specifying type_transition rules on the name. Then policy could define >>>> derived types for each domain, ala >>>> type_transition init self:file "[userfaultfd]" init_userfaultfd; >>>> type_transition untrusted_app self:file "[userfaultfd]" >>>> untrusted_app_userfaultfd; >>>> ... >>>> >>>>>> Also not sure we want to be testing fops for each such case. >>>>> >>>>> I was also thinking of just providing some kind of context string >>>>> (maybe the name), which might be friendlier to modules, but the loose >>>>> coupling kind of scares me, and for this particular application, since >>>>> UFFD is always in the core and never in a module, checking the fops >>>>> seems a bit more robust and doesn't hurt anything. >>>> >>>> Yes, not sure how the vfs folks feel about either coupling (the >>>> name-based one or the fops-based one). Neither seems great. >>>> >>>>>> We >>>>>> were looking at possibly leveraging the name as a key and using >>>>>> security_transition_sid() to generate a distinct SID/context/type for >>>>>> the inode via type_transition rules in policy. We have some WIP >>>>>> along >>>>>> those lines. >>>>> >>>>> Where? Any chance it would be ready soon? I'd rather not hold up this >>>>> work for a more general mechanism. >>>> >>>> Hopefully will have a patch available soon. But not saying this >>>> necessarily has to wait either. >>>> >>>>>>> + /* >>>>>>> + * Always give secure anonymous inodes the sid of the >>>>>>> + * creating task. >>>>>>> + */ >>>>>>> + >>>>>>> + isec->sid = tsec->sid; >>>>>> >>>>>> This doesn't generalize for other users of anonymous inodes, e.g. the >>>>>> /dev/kvm case where we'd rather inherit the SID and class from the >>>>>> original /dev/kvm inode itself. >>>>> >>>>> I think someone mentioned on the first version of this patch that we >>>>> could make it more flexible if the need arose. If we do want to do it >>>>> now, we could have the anon_inode security hook accept a "parent" or >>>>> "context" inode that modules could inspect for the purposes of forming >>>>> the new inode's SID. Does that make sense to you? >>>> >>>> Yes, that's the approach in our current WIP, except we call it a >>>> "related" inode since it isn't necessarily connected to the anon inode >>>> in any vfs sense. >>> >>> The other key difference in our WIP approach is that we assumed that we >>> couldn't mandate allocating a separate anon inode for each of these fds >>> and we wanted to cover all anonymous inodes (not opt-in), so we are >>> storing the SID/class pair as additional fields in the >>> file_security_struct and have modified file_has_perm() and others to >>> look there for anonymous inodes. >> >> A separate inode seems like the simpler approach for now, because it >> means that we have fewer places to check for security information --- >> and it's not as if an inode is particularly expensive. We can always >> switch later. > > We'd prefer having a separate inode if possible but didn't think that > would fly with the vfs folks, especially if we try to apply this to all > anonymous inodes. It might be ok for userfaultfd usage as a specific > case but there is a reason why anonymous inodes were introduced and > creating a separate inode each time defeats that purpose IIUC. It will > be interesting to see how they respond. I suppose an optimization of your approach could be to only allocate a new anon inode if there isn't already one that has the same security info (SID/class pair in the SELinux case).
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 37df7c9eedb1..07b0f6e03849 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait) } } -static const struct file_operations userfaultfd_fops; - static int resolve_userfault_fork(struct userfaultfd_ctx *ctx, struct userfaultfd_ctx *new, struct uffd_msg *msg) @@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f) } #endif -static const struct file_operations userfaultfd_fops = { +const struct file_operations userfaultfd_fops = { #ifdef CONFIG_PROC_FS .show_fdinfo = userfaultfd_show_fdinfo, #endif diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index ac9d71e24b81..549c8b0cca52 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -30,6 +30,8 @@ extern int sysctl_unprivileged_userfaultfd; +extern const struct file_operations userfaultfd_fops; + extern vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason); extern ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start, diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1659b59fb5d7..e178f6f40e93 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -92,6 +92,10 @@ #include <linux/fsnotify.h> #include <linux/fanotify.h> +#ifdef CONFIG_USERFAULTFD +#include <linux/userfaultfd_k.h> +#endif + #include "avc.h" #include "objsec.h" #include "netif.h" @@ -2915,6 +2919,69 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, return 0; } +static int selinux_inode_init_security_anon(struct inode *inode, + const char *name, + const struct file_operations *fops) +{ + const struct task_security_struct *tsec = selinux_cred(current_cred()); + struct common_audit_data ad; + struct inode_security_struct *isec; + + if (unlikely(IS_PRIVATE(inode))) + return 0; + + /* + * We shouldn't be creating secure anonymous inodes before LSM + * initialization completes. + */ + if (unlikely(!selinux_state.initialized)) + return -EBUSY; + + isec = selinux_inode(inode); + + /* + * We only get here once per ephemeral inode. The inode has + * been initialized via inode_alloc_security but is otherwise + * untouched, so check that the state is as + * inode_alloc_security left it. + */ + BUG_ON(isec->initialized != LABEL_INVALID); + BUG_ON(isec->sclass != SECCLASS_FILE); + +#ifdef CONFIG_USERFAULTFD + if (fops == &userfaultfd_fops) + isec->sclass = SECCLASS_UFFD; +#endif + + if (isec->sclass == SECCLASS_FILE) { + printk(KERN_WARNING "refusing to create secure anonymous inode " + "of unknown type"); + return -EOPNOTSUPP; + } + /* + * Always give secure anonymous inodes the sid of the + * creating task. + */ + + isec->sid = tsec->sid; + isec->initialized = LABEL_INITIALIZED; + + /* + * Now that we've initialized security, check whether we're + * allowed to actually create this type of anonymous inode. + */ + + ad.type = LSM_AUDIT_DATA_INODE; + ad.u.inode = inode; + + return avc_has_perm(&selinux_state, + tsec->sid, + isec->sid, + isec->sclass, + FILE__CREATE, + &ad); +} + static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode) { return may_create(dir, dentry, SECCLASS_FILE); @@ -6923,6 +6990,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security), LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security), + LSM_HOOK_INIT(inode_init_security_anon, selinux_inode_init_security_anon), LSM_HOOK_INIT(inode_create, selinux_inode_create), LSM_HOOK_INIT(inode_link, selinux_inode_link), LSM_HOOK_INIT(inode_unlink, selinux_inode_unlink), diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 986f3ac14282..6d4f9ebf2017 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -248,6 +248,8 @@ struct security_class_mapping secclass_map[] = { {"open", "cpu", "kernel", "tracepoint", "read", "write"} }, { "lockdown", { "integrity", "confidentiality", NULL } }, + { "uffd", + { COMMON_FILE_PERMS, NULL } }, { NULL } };
Use the secure anonymous inode LSM hook we just added to let SELinux policy place restrictions on userfaultfd use. The create operation applies to processes creating new instances of these file objects; transfer between processes is covered by restrictions on read, write, and ioctl access already checked inside selinux_file_receive. Signed-off-by: Daniel Colascione <dancol@google.com> --- fs/userfaultfd.c | 4 +- include/linux/userfaultfd_k.h | 2 + security/selinux/hooks.c | 68 +++++++++++++++++++++++++++++ security/selinux/include/classmap.h | 2 + 4 files changed, 73 insertions(+), 3 deletions(-)