| Message ID | 20171004182932.140028-5-chenbofeng.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/04/2017 08:29 PM, Chenbo Feng wrote: > From: Chenbo Feng <fengc@google.com> > > Introduce a bpf object related check when sending and receiving files > through unix domain socket as well as binder. It checks if the receiving > process have privilege to read/write the bpf map or use the bpf program. > This check is necessary because the bpf maps and programs are using a > anonymous inode as their shared inode so the normal way of checking the > files and sockets when passing between processes cannot work properly on > eBPF object. This check only works when the BPF_SYSCALL is configured. [...] > +/* This function will check the file pass through unix socket or binder to see > + * if it is a bpf related object. And apply correspinding checks on the bpf > + * object based on the type. The bpf maps and programs, not like other files and > + * socket, are using a shared anonymous inode inside the kernel as their inode. > + * So checking that inode cannot identify if the process have privilege to > + * access the bpf object and that's why we have to add this additional check in > + * selinux_file_receive and selinux_binder_transfer_files. > + */ [...] If selinux/lsm folks have some input on this one in particular, would be great. The issue is not really tied to bpf specifically, but to the use of anon-inodes wrt fd passing. Maybe some generic resolution can be found to tackle this ... -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/05/2017 01:44 AM, Daniel Borkmann wrote: > On 10/04/2017 08:29 PM, Chenbo Feng wrote: >> From: Chenbo Feng <fengc@google.com> >> >> Introduce a bpf object related check when sending and receiving files >> through unix domain socket as well as binder. It checks if the receiving >> process have privilege to read/write the bpf map or use the bpf program. >> This check is necessary because the bpf maps and programs are using a >> anonymous inode as their shared inode so the normal way of checking the >> files and sockets when passing between processes cannot work properly on >> eBPF object. This check only works when the BPF_SYSCALL is configured. > > [...] >> +/* This function will check the file pass through unix socket or binder to see >> + * if it is a bpf related object. And apply correspinding checks on the bpf >> + * object based on the type. The bpf maps and programs, not like other files and >> + * socket, are using a shared anonymous inode inside the kernel as their inode. >> + * So checking that inode cannot identify if the process have privilege to >> + * access the bpf object and that's why we have to add this additional check in >> + * selinux_file_receive and selinux_binder_transfer_files. >> + */ > [...] > > If selinux/lsm folks have some input on this one in particular, would > be great. The issue is not really tied to bpf specifically, but to the > use of anon-inodes wrt fd passing. Maybe some generic resolution can > be found to tackle this ... Perhaps even just a generic callback in struct file_operations might be better in order to just retrieve the secctx from the underlying object in case of anon-inodes and then have a generic check in selinux_file_receive() for the case when such callback is set, such that we don't need to put specific bpf logic there. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote: > From: Chenbo Feng <fengc@google.com> > > Introduce a bpf object related check when sending and receiving files > through unix domain socket as well as binder. It checks if the > receiving > process have privilege to read/write the bpf map or use the bpf > program. > This check is necessary because the bpf maps and programs are using a > anonymous inode as their shared inode so the normal way of checking > the > files and sockets when passing between processes cannot work properly > on > eBPF object. This check only works when the BPF_SYSCALL is > configured. > > Signed-off-by: Chenbo Feng <fengc@google.com> > --- > include/linux/bpf.h | 3 +++ > kernel/bpf/syscall.c | 4 ++-- > security/selinux/hooks.c | 57 > +++++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 61 insertions(+), 3 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index d757ea3f2228..ac8428a36d56 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, > const union bpf_attr *kattr, > #ifdef CONFIG_BPF_SYSCALL > DECLARE_PER_CPU(int, bpf_prog_active); > > +extern const struct file_operations bpf_map_fops; > +extern const struct file_operations bpf_prog_fops; > + > #define BPF_PROG_TYPE(_id, _ops) \ > extern const struct bpf_verifier_ops _ops; > #define BPF_MAP_TYPE(_id, _ops) \ > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 58ff769d58ab..5789a5359f0a 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp, > const char __user *buf, > return -EINVAL; > } > > -static const struct file_operations bpf_map_fops = { > +const struct file_operations bpf_map_fops = { > #ifdef CONFIG_PROC_FS > .show_fdinfo = bpf_map_show_fdinfo, > #endif > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file > *m, struct file *filp) > } > #endif > > -static const struct file_operations bpf_prog_fops = { > +const struct file_operations bpf_prog_fops = { > #ifdef CONFIG_PROC_FS > .show_fdinfo = bpf_prog_show_fdinfo, > #endif > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 41aba4e3d57c..381474ce3216 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1847,6 +1847,7 @@ 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); > > @@ -2142,6 +2143,10 @@ static int > selinux_binder_transfer_binder(struct task_struct *from, > NULL); > } > > +#ifdef CONFIG_BPF_SYSCALL > +static int bpf_fd_pass(struct file *file, u32 sid); > +#endif > + > static int selinux_binder_transfer_file(struct task_struct *from, > struct task_struct *to, > struct file *file) > @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct > task_struct *from, > return rc; > } > > +#ifdef CONFIG_BPF_SYSCALL > + rc = bpf_fd_pass(file, sid); > + if (rc) > + return rc; > +#endif > + > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > > @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct > task_struct *tsk, > static int selinux_file_receive(struct file *file) > { > const struct cred *cred = current_cred(); > + int rc; > + > + rc = file_has_perm(cred, file, file_to_av(file)); > + if (rc) > + goto out; > + > +#ifdef CONFIG_BPF_SYSCALL > + rc = bpf_fd_pass(file, cred_sid(sid)); > +#endif > > - return file_has_perm(cred, file, file_to_av(file)); > +out: > + return rc; > } > > static int selinux_file_open(struct file *file, const struct cred > *cred) > @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode) > return av; > } > > +/* This function will check the file pass through unix socket or > binder to see > + * if it is a bpf related object. And apply correspinding checks on > the bpf > + * object based on the type. The bpf maps and programs, not like > other files and > + * socket, are using a shared anonymous inode inside the kernel as > their inode. > + * So checking that inode cannot identify if the process have > privilege to > + * access the bpf object and that's why we have to add this > additional check in > + * selinux_file_receive and selinux_binder_transfer_files. > + */ > +static int bpf_fd_pass(struct file *file, u32 sid) > +{ > + struct bpf_security_struct *bpfsec; > + u32 sid = cred_sid(cred); > + struct bpf_prog *prog; > + struct bpf_map *map; > + int ret; > + > + if (file->f_op == &bpf_map_fops) { > + map = file->private_data; > + bpfsec = map->security; > + ret = avc_has_perm(sid, bpfsec->sid, > SECCLASS_BPF_MAP, > + bpf_map_fmode_to_av(file- > >f_mode), NULL); > + if (ret) > + return ret; > + } else if (file->f_op == &bpf_prog_fops) { > + prog = file->private_data; > + bpfsec = prog->aux->security; > + ret = avc_has_perm(sid, bpfsec->sid, > SECCLASS_BPF_PROG, > + BPF_PROG__USE, NULL); > + if (ret) > + return ret; > + } > + return 0; > +} When the struct file is allocated for the bpf map and/or prog, you could call a hook at that time passing both, and note the fact that it is a bpf map/prog in the file_security_struct. Then, on file_receive/binder_transfer_file, you could apply the appropriate checking. Further, if we know that the file is always allocated at the same point as the bpf map/prog, then they should have the same SID (i.e fsec->sid should be the same as bpfsec->sid), so we shouldn't even need to dereference the bpf map/prog. Unless I'm missing something. Also, are we concerned about doing the same in flush_unauthorized_files(), for inheriting descriptors across a context-changing execve? Should this checking actually go into file_has_perm() itself so it is always applied on any use of the struct file? Lastly, do we need/want these checks if sid == bpfsec->sid? We skip FD__USE in the case where sid == fsec->sid, for example. > + > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) > { > u32 sid = current_sid(); -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote: > On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote: > > From: Chenbo Feng <fengc@google.com> > > > > Introduce a bpf object related check when sending and receiving > > files > > through unix domain socket as well as binder. It checks if the > > receiving > > process have privilege to read/write the bpf map or use the bpf > > program. > > This check is necessary because the bpf maps and programs are using > > a > > anonymous inode as their shared inode so the normal way of checking > > the > > files and sockets when passing between processes cannot work > > properly > > on > > eBPF object. This check only works when the BPF_SYSCALL is > > configured. > > > > Signed-off-by: Chenbo Feng <fengc@google.com> > > --- > > include/linux/bpf.h | 3 +++ > > kernel/bpf/syscall.c | 4 ++-- > > security/selinux/hooks.c | 57 > > +++++++++++++++++++++++++++++++++++++++++++++++- > > 3 files changed, 61 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index d757ea3f2228..ac8428a36d56 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog > > *prog, > > const union bpf_attr *kattr, > > #ifdef CONFIG_BPF_SYSCALL > > DECLARE_PER_CPU(int, bpf_prog_active); > > > > +extern const struct file_operations bpf_map_fops; > > +extern const struct file_operations bpf_prog_fops; > > + > > #define BPF_PROG_TYPE(_id, _ops) \ > > extern const struct bpf_verifier_ops _ops; > > #define BPF_MAP_TYPE(_id, _ops) \ > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 58ff769d58ab..5789a5359f0a 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file > > *filp, > > const char __user *buf, > > return -EINVAL; > > } > > > > -static const struct file_operations bpf_map_fops = { > > +const struct file_operations bpf_map_fops = { > > #ifdef CONFIG_PROC_FS > > .show_fdinfo = bpf_map_show_fdinfo, > > #endif > > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct > > seq_file > > *m, struct file *filp) > > } > > #endif > > > > -static const struct file_operations bpf_prog_fops = { > > +const struct file_operations bpf_prog_fops = { > > #ifdef CONFIG_PROC_FS > > .show_fdinfo = bpf_prog_show_fdinfo, > > #endif > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 41aba4e3d57c..381474ce3216 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -1847,6 +1847,7 @@ 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); > > > > @@ -2142,6 +2143,10 @@ static int > > selinux_binder_transfer_binder(struct task_struct *from, > > NULL); > > } > > > > +#ifdef CONFIG_BPF_SYSCALL > > +static int bpf_fd_pass(struct file *file, u32 sid); > > +#endif > > + > > static int selinux_binder_transfer_file(struct task_struct *from, > > struct task_struct *to, > > struct file *file) > > @@ -2165,6 +2170,12 @@ static int > > selinux_binder_transfer_file(struct > > task_struct *from, > > return rc; > > } > > > > +#ifdef CONFIG_BPF_SYSCALL > > + rc = bpf_fd_pass(file, sid); > > + if (rc) > > + return rc; > > +#endif > > + > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > return 0; > > > > @@ -3735,8 +3746,18 @@ static int > > selinux_file_send_sigiotask(struct > > task_struct *tsk, > > static int selinux_file_receive(struct file *file) > > { > > const struct cred *cred = current_cred(); > > + int rc; > > + > > + rc = file_has_perm(cred, file, file_to_av(file)); > > + if (rc) > > + goto out; > > + > > +#ifdef CONFIG_BPF_SYSCALL > > + rc = bpf_fd_pass(file, cred_sid(sid)); > > +#endif > > > > - return file_has_perm(cred, file, file_to_av(file)); > > +out: > > + return rc; > > } > > > > static int selinux_file_open(struct file *file, const struct cred > > *cred) > > @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t > > fmode) > > return av; > > } > > > > +/* This function will check the file pass through unix socket or > > binder to see > > + * if it is a bpf related object. And apply correspinding checks > > on > > the bpf > > + * object based on the type. The bpf maps and programs, not like > > other files and > > + * socket, are using a shared anonymous inode inside the kernel as > > their inode. > > + * So checking that inode cannot identify if the process have > > privilege to > > + * access the bpf object and that's why we have to add this > > additional check in > > + * selinux_file_receive and selinux_binder_transfer_files. > > + */ > > +static int bpf_fd_pass(struct file *file, u32 sid) > > +{ > > + struct bpf_security_struct *bpfsec; > > + u32 sid = cred_sid(cred); > > + struct bpf_prog *prog; > > + struct bpf_map *map; > > + int ret; > > + > > + if (file->f_op == &bpf_map_fops) { > > + map = file->private_data; > > + bpfsec = map->security; > > + ret = avc_has_perm(sid, bpfsec->sid, > > SECCLASS_BPF_MAP, > > + bpf_map_fmode_to_av(file- > > > f_mode), NULL); > > > > + if (ret) > > + return ret; > > + } else if (file->f_op == &bpf_prog_fops) { > > + prog = file->private_data; > > + bpfsec = prog->aux->security; > > + ret = avc_has_perm(sid, bpfsec->sid, > > SECCLASS_BPF_PROG, > > + BPF_PROG__USE, NULL); > > + if (ret) > > + return ret; > > + } > > + return 0; > > +} > > When the struct file is allocated for the bpf map and/or prog, you > could call a hook at that time passing both, and note the fact that > it > is a bpf map/prog in the file_security_struct. Then, on > file_receive/binder_transfer_file, you could apply the appropriate > checking. Further, if we know that the file is always allocated at > the > same point as the bpf map/prog, then they should have the same SID > (i.e > fsec->sid should be the same as bpfsec->sid), so we shouldn't even > need > to dereference the bpf map/prog. Unless I'm missing something. > > Also, are we concerned about doing the same in > flush_unauthorized_files(), for inheriting descriptors across a > context-changing execve? Should this checking actually go into > file_has_perm() itself so it is always applied on any use of the > struct > file? > > Lastly, do we need/want these checks if sid == bpfsec->sid? We skip > FD__USE in the case where sid == fsec->sid, for example. BTW, the prog use check seems slightly redundant in that we will already check fd use permission. So we only really need it if you want to allow fd use but deny prog use. The map read/write checks are more granular than fd use, so I guess we can't get rid of those. > > > + > > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) > > { > > u32 sid = current_sid(); -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 5, 2017 at 6:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote: >> From: Chenbo Feng <fengc@google.com> >> >> Introduce a bpf object related check when sending and receiving files >> through unix domain socket as well as binder. It checks if the >> receiving >> process have privilege to read/write the bpf map or use the bpf >> program. >> This check is necessary because the bpf maps and programs are using a >> anonymous inode as their shared inode so the normal way of checking >> the >> files and sockets when passing between processes cannot work properly >> on >> eBPF object. This check only works when the BPF_SYSCALL is >> configured. >> >> Signed-off-by: Chenbo Feng <fengc@google.com> >> --- >> include/linux/bpf.h | 3 +++ >> kernel/bpf/syscall.c | 4 ++-- >> security/selinux/hooks.c | 57 >> +++++++++++++++++++++++++++++++++++++++++++++++- >> 3 files changed, 61 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index d757ea3f2228..ac8428a36d56 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, >> const union bpf_attr *kattr, >> #ifdef CONFIG_BPF_SYSCALL >> DECLARE_PER_CPU(int, bpf_prog_active); >> >> +extern const struct file_operations bpf_map_fops; >> +extern const struct file_operations bpf_prog_fops; >> + >> #define BPF_PROG_TYPE(_id, _ops) \ >> extern const struct bpf_verifier_ops _ops; >> #define BPF_MAP_TYPE(_id, _ops) \ >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >> index 58ff769d58ab..5789a5359f0a 100644 >> --- a/kernel/bpf/syscall.c >> +++ b/kernel/bpf/syscall.c >> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp, >> const char __user *buf, >> return -EINVAL; >> } >> >> -static const struct file_operations bpf_map_fops = { >> +const struct file_operations bpf_map_fops = { >> #ifdef CONFIG_PROC_FS >> .show_fdinfo = bpf_map_show_fdinfo, >> #endif >> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file >> *m, struct file *filp) >> } >> #endif >> >> -static const struct file_operations bpf_prog_fops = { >> +const struct file_operations bpf_prog_fops = { >> #ifdef CONFIG_PROC_FS >> .show_fdinfo = bpf_prog_show_fdinfo, >> #endif >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 41aba4e3d57c..381474ce3216 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -1847,6 +1847,7 @@ 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); >> >> @@ -2142,6 +2143,10 @@ static int >> selinux_binder_transfer_binder(struct task_struct *from, >> NULL); >> } >> >> +#ifdef CONFIG_BPF_SYSCALL >> +static int bpf_fd_pass(struct file *file, u32 sid); >> +#endif >> + >> static int selinux_binder_transfer_file(struct task_struct *from, >> struct task_struct *to, >> struct file *file) >> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct >> task_struct *from, >> return rc; >> } >> >> +#ifdef CONFIG_BPF_SYSCALL >> + rc = bpf_fd_pass(file, sid); >> + if (rc) >> + return rc; >> +#endif >> + >> if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) >> return 0; >> >> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct >> task_struct *tsk, >> static int selinux_file_receive(struct file *file) >> { >> const struct cred *cred = current_cred(); >> + int rc; >> + >> + rc = file_has_perm(cred, file, file_to_av(file)); >> + if (rc) >> + goto out; >> + >> +#ifdef CONFIG_BPF_SYSCALL >> + rc = bpf_fd_pass(file, cred_sid(sid)); >> +#endif >> >> - return file_has_perm(cred, file, file_to_av(file)); >> +out: >> + return rc; >> } >> >> static int selinux_file_open(struct file *file, const struct cred >> *cred) >> @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode) >> return av; >> } >> >> +/* This function will check the file pass through unix socket or >> binder to see >> + * if it is a bpf related object. And apply correspinding checks on >> the bpf >> + * object based on the type. The bpf maps and programs, not like >> other files and >> + * socket, are using a shared anonymous inode inside the kernel as >> their inode. >> + * So checking that inode cannot identify if the process have >> privilege to >> + * access the bpf object and that's why we have to add this >> additional check in >> + * selinux_file_receive and selinux_binder_transfer_files. >> + */ >> +static int bpf_fd_pass(struct file *file, u32 sid) >> +{ >> + struct bpf_security_struct *bpfsec; >> + u32 sid = cred_sid(cred); >> + struct bpf_prog *prog; >> + struct bpf_map *map; >> + int ret; >> + >> + if (file->f_op == &bpf_map_fops) { >> + map = file->private_data; >> + bpfsec = map->security; >> + ret = avc_has_perm(sid, bpfsec->sid, >> SECCLASS_BPF_MAP, >> + bpf_map_fmode_to_av(file- >> >f_mode), NULL); >> + if (ret) >> + return ret; >> + } else if (file->f_op == &bpf_prog_fops) { >> + prog = file->private_data; >> + bpfsec = prog->aux->security; >> + ret = avc_has_perm(sid, bpfsec->sid, >> SECCLASS_BPF_PROG, >> + BPF_PROG__USE, NULL); >> + if (ret) >> + return ret; >> + } >> + return 0; >> +} > > When the struct file is allocated for the bpf map and/or prog, you > could call a hook at that time passing both, and note the fact that it > is a bpf map/prog in the file_security_struct. Then, on > file_receive/binder_transfer_file, you could apply the appropriate > checking. Further, if we know that the file is always allocated at the > same point as the bpf map/prog, then they should have the same SID (i.e > fsec->sid should be the same as bpfsec->sid), so we shouldn't even need > to dereference the bpf map/prog. Unless I'm missing something. > Thanks for the feedback, but I am a little confused about the proposed implementation. Do we need to add an additional field inside file_security_struct to identify a file as bpf map/prog? Or we just copy the sid stored inside bpf map/prog security field into file_security_struct when we allocate the file and do checks like following: if (file->f_op == &bpf_map_fops) { ret = avc_has_perm(sid, fsec->sid, SECCLASS_BPF, bpf_map_fmode_to_av(file->f_mode), NULL); } else if (file->f_op == &bpf_prog_fops) { ret = avc_has_perm(sid, fsec->sid, SECCLASS_BPF, BPF_PROG__USE, NULL); } > Also, are we concerned about doing the same in > flush_unauthorized_files(), for inheriting descriptors across a > context-changing execve? Should this checking actually go into > file_has_perm() itself so it is always applied on any use of the struct > file? > I agree moving this into file_has_perm might be a better solution. > Lastly, do we need/want these checks if sid == bpfsec->sid? We skip > FD__USE in the case where sid == fsec->sid, for example. > In this case we still have to check if the process have the right privilege to access the map. The creator of the bpf map/prog doesn't necessarily have all privileges to the object. >> + >> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) >> { >> u32 sid = current_sid(); -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 5, 2017 at 11:26 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote: >> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote: >> > From: Chenbo Feng <fengc@google.com> >> > >> > Introduce a bpf object related check when sending and receiving >> > files >> > through unix domain socket as well as binder. It checks if the >> > receiving >> > process have privilege to read/write the bpf map or use the bpf >> > program. >> > This check is necessary because the bpf maps and programs are using >> > a >> > anonymous inode as their shared inode so the normal way of checking >> > the >> > files and sockets when passing between processes cannot work >> > properly >> > on >> > eBPF object. This check only works when the BPF_SYSCALL is >> > configured. >> > >> > Signed-off-by: Chenbo Feng <fengc@google.com> >> > --- >> > include/linux/bpf.h | 3 +++ >> > kernel/bpf/syscall.c | 4 ++-- >> > security/selinux/hooks.c | 57 >> > +++++++++++++++++++++++++++++++++++++++++++++++- >> > 3 files changed, 61 insertions(+), 3 deletions(-) >> > >> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> > index d757ea3f2228..ac8428a36d56 100644 >> > --- a/include/linux/bpf.h >> > +++ b/include/linux/bpf.h >> > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog >> > *prog, >> > const union bpf_attr *kattr, >> > #ifdef CONFIG_BPF_SYSCALL >> > DECLARE_PER_CPU(int, bpf_prog_active); >> > >> > +extern const struct file_operations bpf_map_fops; >> > +extern const struct file_operations bpf_prog_fops; >> > + >> > #define BPF_PROG_TYPE(_id, _ops) \ >> > extern const struct bpf_verifier_ops _ops; >> > #define BPF_MAP_TYPE(_id, _ops) \ >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >> > index 58ff769d58ab..5789a5359f0a 100644 >> > --- a/kernel/bpf/syscall.c >> > +++ b/kernel/bpf/syscall.c >> > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file >> > *filp, >> > const char __user *buf, >> > return -EINVAL; >> > } >> > >> > -static const struct file_operations bpf_map_fops = { >> > +const struct file_operations bpf_map_fops = { >> > #ifdef CONFIG_PROC_FS >> > .show_fdinfo = bpf_map_show_fdinfo, >> > #endif >> > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct >> > seq_file >> > *m, struct file *filp) >> > } >> > #endif >> > >> > -static const struct file_operations bpf_prog_fops = { >> > +const struct file_operations bpf_prog_fops = { >> > #ifdef CONFIG_PROC_FS >> > .show_fdinfo = bpf_prog_show_fdinfo, >> > #endif >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> > index 41aba4e3d57c..381474ce3216 100644 >> > --- a/security/selinux/hooks.c >> > +++ b/security/selinux/hooks.c >> > @@ -1847,6 +1847,7 @@ 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); >> > >> > @@ -2142,6 +2143,10 @@ static int >> > selinux_binder_transfer_binder(struct task_struct *from, >> > NULL); >> > } >> > >> > +#ifdef CONFIG_BPF_SYSCALL >> > +static int bpf_fd_pass(struct file *file, u32 sid); >> > +#endif >> > + >> > static int selinux_binder_transfer_file(struct task_struct *from, >> > struct task_struct *to, >> > struct file *file) >> > @@ -2165,6 +2170,12 @@ static int >> > selinux_binder_transfer_file(struct >> > task_struct *from, >> > return rc; >> > } >> > >> > +#ifdef CONFIG_BPF_SYSCALL >> > + rc = bpf_fd_pass(file, sid); >> > + if (rc) >> > + return rc; >> > +#endif >> > + >> > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) >> > return 0; >> > >> > @@ -3735,8 +3746,18 @@ static int >> > selinux_file_send_sigiotask(struct >> > task_struct *tsk, >> > static int selinux_file_receive(struct file *file) >> > { >> > const struct cred *cred = current_cred(); >> > + int rc; >> > + >> > + rc = file_has_perm(cred, file, file_to_av(file)); >> > + if (rc) >> > + goto out; >> > + >> > +#ifdef CONFIG_BPF_SYSCALL >> > + rc = bpf_fd_pass(file, cred_sid(sid)); >> > +#endif >> > >> > - return file_has_perm(cred, file, file_to_av(file)); >> > +out: >> > + return rc; >> > } >> > >> > static int selinux_file_open(struct file *file, const struct cred >> > *cred) >> > @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t >> > fmode) >> > return av; >> > } >> > >> > +/* This function will check the file pass through unix socket or >> > binder to see >> > + * if it is a bpf related object. And apply correspinding checks >> > on >> > the bpf >> > + * object based on the type. The bpf maps and programs, not like >> > other files and >> > + * socket, are using a shared anonymous inode inside the kernel as >> > their inode. >> > + * So checking that inode cannot identify if the process have >> > privilege to >> > + * access the bpf object and that's why we have to add this >> > additional check in >> > + * selinux_file_receive and selinux_binder_transfer_files. >> > + */ >> > +static int bpf_fd_pass(struct file *file, u32 sid) >> > +{ >> > + struct bpf_security_struct *bpfsec; >> > + u32 sid = cred_sid(cred); >> > + struct bpf_prog *prog; >> > + struct bpf_map *map; >> > + int ret; >> > + >> > + if (file->f_op == &bpf_map_fops) { >> > + map = file->private_data; >> > + bpfsec = map->security; >> > + ret = avc_has_perm(sid, bpfsec->sid, >> > SECCLASS_BPF_MAP, >> > + bpf_map_fmode_to_av(file- >> > > f_mode), NULL); >> > >> > + if (ret) >> > + return ret; >> > + } else if (file->f_op == &bpf_prog_fops) { >> > + prog = file->private_data; >> > + bpfsec = prog->aux->security; >> > + ret = avc_has_perm(sid, bpfsec->sid, >> > SECCLASS_BPF_PROG, >> > + BPF_PROG__USE, NULL); >> > + if (ret) >> > + return ret; >> > + } >> > + return 0; >> > +} >> >> When the struct file is allocated for the bpf map and/or prog, you >> could call a hook at that time passing both, and note the fact that >> it >> is a bpf map/prog in the file_security_struct. Then, on >> file_receive/binder_transfer_file, you could apply the appropriate >> checking. Further, if we know that the file is always allocated at >> the >> same point as the bpf map/prog, then they should have the same SID >> (i.e >> fsec->sid should be the same as bpfsec->sid), so we shouldn't even >> need >> to dereference the bpf map/prog. Unless I'm missing something. >> >> Also, are we concerned about doing the same in >> flush_unauthorized_files(), for inheriting descriptors across a >> context-changing execve? Should this checking actually go into >> file_has_perm() itself so it is always applied on any use of the >> struct >> file? >> >> Lastly, do we need/want these checks if sid == bpfsec->sid? We skip >> FD__USE in the case where sid == fsec->sid, for example. > > BTW, the prog use check seems slightly redundant in that we will > already check fd use permission. So we only really need it if you want > to allow fd use but deny prog use. The map read/write checks are more > granular than fd use, so I guess we can't get rid of those. > But it seems fd use doesn't check what object is behind that fd. So if we don't want a process to run the eBPF program, then we still need a additional check for it. Maybe use prog_run instead of prog_use should be more appropriate. >> >> > + >> > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) >> > { >> > u32 sid = current_sid(); -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index d757ea3f2228..ac8428a36d56 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, #ifdef CONFIG_BPF_SYSCALL DECLARE_PER_CPU(int, bpf_prog_active); +extern const struct file_operations bpf_map_fops; +extern const struct file_operations bpf_prog_fops; + #define BPF_PROG_TYPE(_id, _ops) \ extern const struct bpf_verifier_ops _ops; #define BPF_MAP_TYPE(_id, _ops) \ diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 58ff769d58ab..5789a5359f0a 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf, return -EINVAL; } -static const struct file_operations bpf_map_fops = { +const struct file_operations bpf_map_fops = { #ifdef CONFIG_PROC_FS .show_fdinfo = bpf_map_show_fdinfo, #endif @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp) } #endif -static const struct file_operations bpf_prog_fops = { +const struct file_operations bpf_prog_fops = { #ifdef CONFIG_PROC_FS .show_fdinfo = bpf_prog_show_fdinfo, #endif diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 41aba4e3d57c..381474ce3216 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1847,6 +1847,7 @@ 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); @@ -2142,6 +2143,10 @@ static int selinux_binder_transfer_binder(struct task_struct *from, NULL); } +#ifdef CONFIG_BPF_SYSCALL +static int bpf_fd_pass(struct file *file, u32 sid); +#endif + static int selinux_binder_transfer_file(struct task_struct *from, struct task_struct *to, struct file *file) @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct task_struct *from, return rc; } +#ifdef CONFIG_BPF_SYSCALL + rc = bpf_fd_pass(file, sid); + if (rc) + return rc; +#endif + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0; @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk, static int selinux_file_receive(struct file *file) { const struct cred *cred = current_cred(); + int rc; + + rc = file_has_perm(cred, file, file_to_av(file)); + if (rc) + goto out; + +#ifdef CONFIG_BPF_SYSCALL + rc = bpf_fd_pass(file, cred_sid(sid)); +#endif - return file_has_perm(cred, file, file_to_av(file)); +out: + return rc; } static int selinux_file_open(struct file *file, const struct cred *cred) @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode) return av; } +/* This function will check the file pass through unix socket or binder to see + * if it is a bpf related object. And apply correspinding checks on the bpf + * object based on the type. The bpf maps and programs, not like other files and + * socket, are using a shared anonymous inode inside the kernel as their inode. + * So checking that inode cannot identify if the process have privilege to + * access the bpf object and that's why we have to add this additional check in + * selinux_file_receive and selinux_binder_transfer_files. + */ +static int bpf_fd_pass(struct file *file, u32 sid) +{ + struct bpf_security_struct *bpfsec; + u32 sid = cred_sid(cred); + struct bpf_prog *prog; + struct bpf_map *map; + int ret; + + if (file->f_op == &bpf_map_fops) { + map = file->private_data; + bpfsec = map->security; + ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP, + bpf_map_fmode_to_av(file->f_mode), NULL); + if (ret) + return ret; + } else if (file->f_op == &bpf_prog_fops) { + prog = file->private_data; + bpfsec = prog->aux->security; + ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG, + BPF_PROG__USE, NULL); + if (ret) + return ret; + } + return 0; +} + static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) { u32 sid = current_sid();