* Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON [not found] <20201028203853.2412751-1-dan@kernelim.com> @ 2020-10-28 21:56 ` Andrii Nakryiko 2020-10-28 22:30 ` Alon, Liran 0 siblings, 1 reply; 5+ messages in thread From: Andrii Nakryiko @ 2020-10-28 21:56 UTC (permalink / raw) To: Dan Aloni, bpf Cc: security, Liran Alon, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh + bpf@vger.kernel.org On Wed, Oct 28, 2020 at 1:40 PM Dan Aloni <dan@kernelim.com> wrote: > > Commit 341dfcf8d78e ("btf: expose BTF info through sysfs") added a sysfs > file that exposes to userspace kernel BTF information which allows > userspace to deduce the structure layout of all kernel internal > structures. > > This file is currently accessible to unprivileged users, without > requiring any special capability. Given that knowledge on kernel > structure layout is useful for dynamically building local privilege > escalation exploit in userspace, access to this file should be > restricted. So is /proc/config.gz, which is also very helpful in understanding what exactly is there in the kernel. So seems to be /boot/vmlinux-$(uname -r), which has exactly the same BTF data and more. Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot of users relying on BTF availability to build their BPF applications. We shouldn't expect developers to build their applications under root. But that's what this patch is trying to do. > > Fixes: 341dfcf8d78e ("btf: expose BTF info through sysfs") > Co-developed-by: Liran Alon <liran@amazon.com> > Signed-off-by: Liran Alon <liran@amazon.com> > Signed-off-by: Dan Aloni <dan@kernelim.com> > --- > kernel/bpf/sysfs_btf.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/bpf/sysfs_btf.c b/kernel/bpf/sysfs_btf.c > index 11b3380887fa..c985d42dfa49 100644 > --- a/kernel/bpf/sysfs_btf.c > +++ b/kernel/bpf/sysfs_btf.c > @@ -7,6 +7,7 @@ > #include <linux/kobject.h> > #include <linux/init.h> > #include <linux/sysfs.h> > +#include <linux/capability.h> > > /* See scripts/link-vmlinux.sh, gen_btf() func for details */ > extern char __weak __start_BTF[]; > @@ -17,6 +18,9 @@ btf_vmlinux_read(struct file *file, struct kobject *kobj, > struct bin_attribute *bin_attr, > char *buf, loff_t off, size_t len) > { > + if (!perfmon_capable()) > + return -EACCES; > + > memcpy(buf, __start_BTF + off, len); > return len; > } > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON 2020-10-28 21:56 ` [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON Andrii Nakryiko @ 2020-10-28 22:30 ` Alon, Liran 2020-10-28 23:06 ` Alexei Starovoitov 0 siblings, 1 reply; 5+ messages in thread From: Alon, Liran @ 2020-10-28 22:30 UTC (permalink / raw) To: Andrii Nakryiko, Dan Aloni, bpf Cc: security, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh On 28/10/2020 23:56, Andrii Nakryiko wrote: > + bpf@vger.kernel.org You shouldn't Cc public email lists for a patch submitted to security@kernel.org. > > On Wed, Oct 28, 2020 at 1:40 PM Dan Aloni <dan@kernelim.com> wrote: >> Commit 341dfcf8d78e ("btf: expose BTF info through sysfs") added a sysfs >> file that exposes to userspace kernel BTF information which allows >> userspace to deduce the structure layout of all kernel internal >> structures. >> >> This file is currently accessible to unprivileged users, without >> requiring any special capability. Given that knowledge on kernel >> structure layout is useful for dynamically building local privilege >> escalation exploit in userspace, access to this file should be >> restricted. > So is /proc/config.gz, which is also very helpful in understanding > what exactly is there in the kernel. Viewing kernel build config is more like querying supported kernel features. I don't consider it as a meaningful information disclosure, as I see disclosing the kernel internal struct layout. > So seems to be > /boot/vmlinux-$(uname -r), which has exactly the same BTF data and > more. I agree. True. Good enough argument for dropping this patch. > > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot > of users relying on BTF availability to build their BPF applications. True. If this patch is applied, would need to at least be behind an optin knob. Similar to dmesg_restrict. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON 2020-10-28 22:30 ` Alon, Liran @ 2020-10-28 23:06 ` Alexei Starovoitov 2020-10-29 4:15 ` Willy Tarreau 0 siblings, 1 reply; 5+ messages in thread From: Alexei Starovoitov @ 2020-10-28 23:06 UTC (permalink / raw) To: Alon, Liran Cc: Andrii Nakryiko, Dan Aloni, bpf, security, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh On Thu, Oct 29, 2020 at 12:30:49AM +0200, Alon, Liran wrote: > > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot > > of users relying on BTF availability to build their BPF applications. > True. If this patch is applied, would need to at least be behind an optin > knob. Similar to dmesg_restrict. It's not going to be applied. If a file shouldn't be read by a user it should have appropriate file permissions instead of 444. Checking capable() in read() is very non-unix way to deal with permissions. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON 2020-10-28 23:06 ` Alexei Starovoitov @ 2020-10-29 4:15 ` Willy Tarreau 2020-10-30 11:23 ` Michael Ellerman 0 siblings, 1 reply; 5+ messages in thread From: Willy Tarreau @ 2020-10-29 4:15 UTC (permalink / raw) To: Alexei Starovoitov Cc: Alon, Liran, Andrii Nakryiko, Dan Aloni, bpf, security, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh On Wed, Oct 28, 2020 at 04:06:02PM -0700, Alexei Starovoitov wrote: > On Thu, Oct 29, 2020 at 12:30:49AM +0200, Alon, Liran wrote: > > > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot > > > of users relying on BTF availability to build their BPF applications. > > True. If this patch is applied, would need to at least be behind an optin > > knob. Similar to dmesg_restrict. > > It's not going to be applied. If a file shouldn't be read by a user > it should have appropriate file permissions instead of 444. > Checking capable() in read() is very non-unix way to deal with permissions. Not only it's a non-unix way, both don't achieve the same goals at all! One checks for permissions at open() time and may for example allow a process to drop its uid after opening, while the other one allows to filter who can really read it, particularly in case the FD is inherited between processes. With this said, I don't see why there would be a special case for this one, it should definitely stick to file permissions only. Willy ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON 2020-10-29 4:15 ` Willy Tarreau @ 2020-10-30 11:23 ` Michael Ellerman 0 siblings, 0 replies; 5+ messages in thread From: Michael Ellerman @ 2020-10-30 11:23 UTC (permalink / raw) To: Willy Tarreau, Alexei Starovoitov Cc: Alon, Liran, Andrii Nakryiko, Dan Aloni, bpf, security, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh Willy Tarreau <w@1wt.eu> writes: > On Wed, Oct 28, 2020 at 04:06:02PM -0700, Alexei Starovoitov wrote: >> On Thu, Oct 29, 2020 at 12:30:49AM +0200, Alon, Liran wrote: >> > > Guarding /sys/kernel/bpf/vmlinux behind CAP_PERFMON would break a lot >> > > of users relying on BTF availability to build their BPF applications. >> > True. If this patch is applied, would need to at least be behind an optin >> > knob. Similar to dmesg_restrict. >> >> It's not going to be applied. If a file shouldn't be read by a user >> it should have appropriate file permissions instead of 444. >> Checking capable() in read() is very non-unix way to deal with permissions. > > Not only it's a non-unix way, both don't achieve the same goals at all! > > One checks for permissions at open() time and may for example allow a > process to drop its uid after opening, while the other one allows to > filter who can really read it, particularly in case the FD is inherited > between processes. With this said, I don't see why there would be a > special case for this one, it should definitely stick to file permissions > only. From include/linux/bpf.h: static inline bool bpf_allow_ptr_leaks(void) { return perfmon_capable(); } static inline bool bpf_allow_ptr_to_map_access(void) { return perfmon_capable(); } static inline bool bpf_bypass_spec_v1(void) { return perfmon_capable(); } static inline bool bpf_bypass_spec_v4(void) { return perfmon_capable(); } There's also several cases in bpf_base_func_proto(). So it seems entirely reasonable to suggest that perfmon_capable() is the right check here. cheers ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-30 11:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20201028203853.2412751-1-dan@kernelim.com>
2020-10-28 21:56 ` [PATCH] btf: Expose kernel BTF only to tasks with CAP_PERFMON Andrii Nakryiko
2020-10-28 22:30 ` Alon, Liran
2020-10-28 23:06 ` Alexei Starovoitov
2020-10-29 4:15 ` Willy Tarreau
2020-10-30 11:23 ` Michael Ellerman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.