[PATCH bpf-next] bpf: allow loading instructions from a fd

[PATCH bpf-next] bpf: allow loading instructions from a fd

[Matteo Croce]

From: Matteo Croce <mcroce@microsoft.com>

Allow to load the BPF instructons from a file descriptor,
other than a pointer.

This is required by the Integrity Subsystem to validate the source of
the instructions.

In bpf_attr replace 'insns', which is an u64, to a union containing also
the file descriptor as int.
A new BPF_F_LOAD_BY_FD flag tells bpf_prog_load() to load
the instructions from file descriptor and ignore the pointer.

As BPF files usually are regular ELF files, start reading from the
current file position, so the userspace can skip the ELF header and jump
to the right section.

Signed-off-by: Matteo Croce <mcroce@microsoft.com>
---
 include/uapi/linux/bpf.h | 10 +++++-
 kernel/bpf/syscall.c     | 69 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 8bd33050b7bb..4ef75198db21 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -332,6 +332,11 @@ enum bpf_link_type {
 /* The verifier internal test flag. Behavior is undefined */
 #define BPF_F_TEST_STATE_FREQ	(1U << 3)
 
+/* The BPF is loaded by the file descriptor in `prog_bpf_fd`
+ * instead of the buffer pointed by `insns`.
+ */
+#define BPF_F_LOAD_BY_FD	(1U << 4)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * two extensions:
  *
@@ -482,7 +487,10 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_PROG_LOAD command */
 		__u32		prog_type;	/* one of enum bpf_prog_type */
 		__u32		insn_cnt;
-		__aligned_u64	insns;
+		union {
+			__aligned_u64	insns;		/* BPF instructions */
+			__u32		prog_bpf_fd;	/* fd pointing to BPF program */
+		};
 		__aligned_u64	license;
 		__u32		log_level;	/* verbosity level of verifier */
 		__u32		log_size;	/* size of user buffer */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0fd80ac81f70..b6b1ce34a72b 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -24,6 +24,7 @@
 #include <linux/ctype.h>
 #include <linux/nospec.h>
 #include <linux/audit.h>
+#include <linux/stat.h>
 #include <uapi/linux/btf.h>
 #include <linux/pgtable.h>
 #include <linux/bpf_lsm.h>
@@ -2082,6 +2083,55 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 /* last field in 'union bpf_attr' used by this command */
 #define	BPF_PROG_LOAD_LAST_FIELD attach_prog_fd
 
+static int bpf_load_from_fd(u32 fd, void *buf, loff_t insn_cnt)
+{
+	ssize_t bytes, total = 0;
+	struct fd f = fdget(fd);
+	int ret = 0;
+	loff_t pos;
+
+	if (!f.file)
+		return -EBADF;
+
+	if (!S_ISREG(file_inode(f.file)->i_mode)) {
+		ret = -EINVAL;
+		goto out_fd;
+	}
+
+	ret = deny_write_access(f.file);
+	if (ret)
+		goto out_fd;
+
+	ret = security_kernel_read_file(f.file, READING_UNKNOWN);
+	if (ret)
+		goto out;
+
+	pos = f.file->f_pos;
+
+	while (total < insn_cnt) {
+		bytes = kernel_read(f.file, buf + total, insn_cnt - total, &pos);
+		if (bytes < 0) {
+			ret = bytes;
+			goto out;
+		}
+
+		if (bytes == 0)
+			break;
+
+		total += bytes;
+		pos += bytes;
+	}
+
+	if (total != insn_cnt)
+		ret = -EIO;
+
+out:
+	allow_write_access(f.file);
+out_fd:
+	fdput(f);
+	return ret;
+}
+
 static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 {
 	enum bpf_prog_type type = attr->prog_type;
@@ -2096,7 +2146,8 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 	if (attr->prog_flags & ~(BPF_F_STRICT_ALIGNMENT |
 				 BPF_F_ANY_ALIGNMENT |
 				 BPF_F_TEST_STATE_FREQ |
-				 BPF_F_TEST_RND_HI32))
+				 BPF_F_TEST_RND_HI32 |
+				 BPF_F_LOAD_BY_FD))
 		return -EINVAL;
 
 	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&
@@ -2162,10 +2213,18 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 
 	prog->len = attr->insn_cnt;
 
-	err = -EFAULT;
-	if (copy_from_user(prog->insns, u64_to_user_ptr(attr->insns),
-			   bpf_prog_insn_size(prog)) != 0)
-		goto free_prog;
+	if (attr->prog_flags & BPF_F_LOAD_BY_FD) {
+		err = bpf_load_from_fd(attr->prog_bpf_fd, (void *)prog->insns,
+				       bpf_prog_insn_size(prog));
+		if (err)
+			goto free_prog;
+	} else {
+		if (copy_from_user(prog->insns, u64_to_user_ptr(attr->insns),
+				   bpf_prog_insn_size(prog)) != 0) {
+			err = -EFAULT;
+			goto free_prog;
+		}
+	}
 
 	prog->orig_prog = NULL;
 	prog->jited = 0;
-- 
2.26.2

Re: [PATCH bpf-next] bpf: allow loading instructions from a fd

[Alexei Starovoitov]

On Mon, Jul 13, 2020 at 03:05:11PM +0200, Matteo Croce wrote:
> From: Matteo Croce <mcroce@microsoft.com>
> 
> Allow to load the BPF instructons from a file descriptor,
> other than a pointer.
> 
> This is required by the Integrity Subsystem to validate the source of
> the instructions.
> 
> In bpf_attr replace 'insns', which is an u64, to a union containing also
> the file descriptor as int.
> A new BPF_F_LOAD_BY_FD flag tells bpf_prog_load() to load
> the instructions from file descriptor and ignore the pointer.
> 
> As BPF files usually are regular ELF files, start reading from the
> current file position, so the userspace can skip the ELF header and jump
> to the right section.

That is not the case at all.
Have you looked at amount of work libbpf is doing with elf file before
raw instructions become suitable to be loaded by the kernel?

Re: [PATCH bpf-next] bpf: allow loading instructions from a fd

[Matteo Croce]

On Tue, Jul 14, 2020 at 7:31 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jul 13, 2020 at 03:05:11PM +0200, Matteo Croce wrote:
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > Allow to load the BPF instructons from a file descriptor,
> > other than a pointer.
> >
> > This is required by the Integrity Subsystem to validate the source of
> > the instructions.
> >
> > In bpf_attr replace 'insns', which is an u64, to a union containing also
> > the file descriptor as int.
> > A new BPF_F_LOAD_BY_FD flag tells bpf_prog_load() to load
> > the instructions from file descriptor and ignore the pointer.
> >
> > As BPF files usually are regular ELF files, start reading from the
> > current file position, so the userspace can skip the ELF header and jump
> > to the right section.
>
> That is not the case at all.
> Have you looked at amount of work libbpf is doing with elf file before
> raw instructions become suitable to be loaded by the kernel?

I see now what bpf_object__relocate() and all the *reloc* functions
do, so it can't be done this way, I see.

A malicious BPF file can be as bad as a malicious binary. Let's say I
want to assert code integrity for BPF files, what could be a viable
option?
Perhaps a signature in the object file as we do with modules?

Regards,
-- 
per aspera ad upstream

Re: [PATCH bpf-next] bpf: allow loading instructions from a fd

[Alexei Starovoitov]

On Thu, Jul 16, 2020 at 08:47:36PM +0200, Matteo Croce wrote:
> On Tue, Jul 14, 2020 at 7:31 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 03:05:11PM +0200, Matteo Croce wrote:
> > > From: Matteo Croce <mcroce@microsoft.com>
> > >
> > > Allow to load the BPF instructons from a file descriptor,
> > > other than a pointer.
> > >
> > > This is required by the Integrity Subsystem to validate the source of
> > > the instructions.
> > >
> > > In bpf_attr replace 'insns', which is an u64, to a union containing also
> > > the file descriptor as int.
> > > A new BPF_F_LOAD_BY_FD flag tells bpf_prog_load() to load
> > > the instructions from file descriptor and ignore the pointer.
> > >
> > > As BPF files usually are regular ELF files, start reading from the
> > > current file position, so the userspace can skip the ELF header and jump
> > > to the right section.
> >
> > That is not the case at all.
> > Have you looked at amount of work libbpf is doing with elf file before
> > raw instructions become suitable to be loaded by the kernel?
> 
> I see now what bpf_object__relocate() and all the *reloc* functions
> do, so it can't be done this way, I see.
> 
> A malicious BPF file can be as bad as a malicious binary. Let's say I
> want to assert code integrity for BPF files, what could be a viable
> option?
> Perhaps a signature in the object file as we do with modules?

It's a hard problem to solve.
Signing bpf programs was proposed in the past. It may work, but challenging
to implement, since even simplest programs are being modified by the user
space loader before kernel sees them. The signature would have to skip
all such instructions which makes the signature verification 'best effort'.
Some instructions won't be covered by signature (like ld_imm64 that points
to a map).