| Message ID | bbfd69226fd74391045bafc695bba9a46cacca85.1482545792.git.luto@kernel.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 12/24/2016 03:22 AM, Andy Lutomirski wrote: > BPF digests are intended to be used to avoid reloading programs that > are already loaded. For use cases (CRIU?) where untrusted programs > are involved, intentional hash collisions could cause the wrong BPF > program to execute. Additionally, if BPF digests are ever used > in-kernel to skip verification, a hash collision could give privilege > escalation directly. Just for the record, digests will never ever be used to skip the verification step, so I don't know why this idea even comes up here (?) or is part of the changelog? As this will never be done anyway, rather drop that part so we can avoid confusion on this? Wrt untrusted programs, I don't see much of a use on this facility in general for them. Something like a tail call map would quite likely only be private to the application. And again, I really doubt we'll have something like user namespace support in the foreseeable future. Anyway, that said, I don't really have a big issue if you want to switch to sha256, though. > SHA1 is no longer considered adequately collision-resistant (see, for > example, all the major browsers dropping support for SHA1 > certificates). Use SHA256 instead. > > I moved the digest field to keep all of the bpf program metadata in > the same cache line. > > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
On Sat, Dec 24, 2016 at 08:59:53PM +0100, Daniel Borkmann wrote: > On 12/24/2016 03:22 AM, Andy Lutomirski wrote: > >BPF digests are intended to be used to avoid reloading programs that > >are already loaded. For use cases (CRIU?) where untrusted programs > >are involved, intentional hash collisions could cause the wrong BPF > >program to execute. Additionally, if BPF digests are ever used > >in-kernel to skip verification, a hash collision could give privilege > >escalation directly. > > Just for the record, digests will never ever be used to skip the > verification step, so I don't know why this idea even comes up > here (?) or is part of the changelog? As this will never be done > anyway, rather drop that part so we can avoid confusion on this? +1 to what Daniel said above. For the others let me explain what this patch set is actually trying to accomplish. Andy had an idea that sha256 of the program can somehow be used to bypass kernel verifier during program loading. Furthemore he thinks that such 'bypass' would be useful for criu of bpf programs, hence see vigorously attacking existing prog_digest (sha1) because it's not as secure as sha256 and hence cannot be used for such 'bypass'. The problem with criu of bpf programs is same as criu of kernel modules. For the main tracing and networking use cases, we cannot stop the kernel, so criu is out of question already. Even if we could stop all the events that trigger bpf program execution, the sha256 or memcmp() of the full program is not enough to guarantee that two programs are the same. Ex. bpf_map_lookup() may be safe for one program and not for another depending on how map was created. Two programs of different types are not comparable either. Etc, etc. Therefore the idea of using sha256 for such purpose is bogus, and I have an obvious NACK for bpf related patches 3,4,5,6. For the questions raised in other threads: I'm not ok with making BPF depend on CRYPTO, since it's the same as requiring CRYPTO to select BPF for no good reason. And 0/6 commit log: > Since there are plenty of uses for the new-in-4.10 BPF digest feature > that would be problematic if malicious users could produce collisions, > the BPF digest should be collision-resistant. This statement is also bogus. The only reason we added prog_digest is to improve debuggability and introspection of bpf programs. As I said in the previous thread "collisions are ok" and we could have used jhash here to avoid patches like this ever appearing and wasting everyones time. sha1 is 20 bytes which is already a bit long to print and copy paste by humans. whereas 4 byte jhash is a bit too short, since collisions are not that rare and may lead to incorrect assumptions from the users that develop the programs. I would prefer something in 6-10 byte range that prevents collisions most of the time and short to print as hex, but I don't know of anything like this in the existing kernel and inventing bpf specific hash is not great. Another requirement for debugging (and prog_digest) that user space should be able to produce the same hash without asking kernel, so sha1 fits that as well, since it's well known and easy to put into library. sha256 doesn't fit either of these requirements. 32-bytes are too long to print and when we use it as a substitue for the prog name for jited ksym, looking at long function names will screw up all tools like perf, which we don't want. sha256 is equally not easy for user space app like iproute2, so not an acceptable choice from that pov either.
On Mon, Dec 26, 2016 at 5:36 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Sat, Dec 24, 2016 at 08:59:53PM +0100, Daniel Borkmann wrote: >> On 12/24/2016 03:22 AM, Andy Lutomirski wrote: >> >BPF digests are intended to be used to avoid reloading programs that >> >are already loaded. For use cases (CRIU?) where untrusted programs >> >are involved, intentional hash collisions could cause the wrong BPF >> >program to execute. Additionally, if BPF digests are ever used >> >in-kernel to skip verification, a hash collision could give privilege >> >escalation directly. >> >> Just for the record, digests will never ever be used to skip the >> verification step, so I don't know why this idea even comes up >> here (?) or is part of the changelog? As this will never be done >> anyway, rather drop that part so we can avoid confusion on this? > > +1 to what Daniel said above. > > For the others let me explain what this patch set is actually > trying to accomplish. The patch: a) cleans up the code and b) uses a cryptographic hash that is actually believed to satisfy the definition of a cryptographic hash. There's no excuse for not doing b. > and I have an obvious NACK for bpf related patches 3,4,5,6. Did you *read* the ones that were pure cleanups? > > sha1 is 20 bytes which is already a bit long to print and copy paste by humans. > whereas 4 byte jhash is a bit too short, since collisions are not that rare > and may lead to incorrect assumptions from the users that develop the programs. > I would prefer something in 6-10 byte range that prevents collisions most of > the time and short to print as hex, but I don't know of anything like this > in the existing kernel and inventing bpf specific hash is not great. > Another requirement for debugging (and prog_digest) that user space > should be able to produce the same hash without asking kernel, so > sha1 fits that as well, since it's well known and easy to put into library. Then truncate them in user space.
diff --git a/include/linux/filter.h b/include/linux/filter.h index 702314253797..23df2574e30c 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -14,7 +14,8 @@ #include <linux/workqueue.h> #include <linux/sched.h> #include <linux/capability.h> -#include <linux/cryptohash.h> + +#include <crypto/sha.h> #include <net/sch_generic.h> @@ -408,11 +409,11 @@ struct bpf_prog { kmemcheck_bitfield_end(meta); enum bpf_prog_type type; /* Type of BPF program */ u32 len; /* Number of filter blocks */ - u32 digest[SHA_DIGEST_WORDS]; /* Program digest */ struct bpf_prog_aux *aux; /* Auxiliary fields */ struct sock_fprog_kern *orig_prog; /* Original BPF program */ unsigned int (*bpf_func)(const void *ctx, const struct bpf_insn *insn); + u8 digest[SHA256_DIGEST_SIZE]; /* Program digest */ /* Instructions for interpreter */ union { struct sock_filter insns[0]; @@ -519,12 +520,6 @@ static inline u32 bpf_prog_insn_size(const struct bpf_prog *prog) return prog->len * sizeof(struct bpf_insn); } -static inline u32 bpf_prog_digest_scratch_size(const struct bpf_prog *prog) -{ - return round_up(bpf_prog_insn_size(prog) + - sizeof(__be64) + 1, SHA_MESSAGE_BYTES); -} - static inline unsigned int bpf_prog_size(unsigned int proglen) { return max(sizeof(struct bpf_prog), diff --git a/init/Kconfig b/init/Kconfig index 223b734abccd..5a4e2d99cc38 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1634,6 +1634,7 @@ config BPF_SYSCALL bool "Enable bpf() system call" select ANON_INODES select BPF + select CRYPTO_SHA256_LIB default n help Enable the bpf() system call that allows to manipulate eBPF diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 1eb4f1303756..911993863799 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -148,22 +148,18 @@ void __bpf_prog_free(struct bpf_prog *fp) int bpf_prog_calc_digest(struct bpf_prog *fp) { - const u32 bits_offset = SHA_MESSAGE_BYTES - sizeof(__be64); - u32 raw_size = bpf_prog_digest_scratch_size(fp); - u32 ws[SHA_WORKSPACE_WORDS]; - u32 i, bsize, psize, blocks; + struct sha256_state sha; + u32 i, psize; struct bpf_insn *dst; bool was_ld_map; - u8 *raw, *todo; - __be32 *result; - __be64 *bits; + u8 *raw; - raw = vmalloc(raw_size); + psize = bpf_prog_insn_size(fp); + raw = vmalloc(psize); if (!raw) return -ENOMEM; - sha_init(fp->digest); - memset(ws, 0, sizeof(ws)); + sha256_init(&sha); /* We need to take out the map fd for the digest calculation * since they are unstable from user space side. @@ -188,30 +184,7 @@ int bpf_prog_calc_digest(struct bpf_prog *fp) } } - psize = bpf_prog_insn_size(fp); - memset(&raw[psize], 0, raw_size - psize); - raw[psize++] = 0x80; - - bsize = round_up(psize, SHA_MESSAGE_BYTES); - blocks = bsize / SHA_MESSAGE_BYTES; - todo = raw; - if (bsize - psize >= sizeof(__be64)) { - bits = (__be64 *)(todo + bsize - sizeof(__be64)); - } else { - bits = (__be64 *)(todo + bsize + bits_offset); - blocks++; - } - *bits = cpu_to_be64((psize - 1) << 3); - - while (blocks--) { - sha_transform(fp->digest, todo, ws); - todo += SHA_MESSAGE_BYTES; - } - - result = (__force __be32 *)fp->digest; - for (i = 0; i < SHA_DIGEST_WORDS; i++) - result[i] = cpu_to_be32(fp->digest[i]); - + sha256_finup(&sha, raw, psize, fp->digest); vfree(raw); return 0; }
BPF digests are intended to be used to avoid reloading programs that are already loaded. For use cases (CRIU?) where untrusted programs are involved, intentional hash collisions could cause the wrong BPF program to execute. Additionally, if BPF digests are ever used in-kernel to skip verification, a hash collision could give privilege escalation directly. SHA1 is no longer considered adequately collision-resistant (see, for example, all the major browsers dropping support for SHA1 certificates). Use SHA256 instead. I moved the digest field to keep all of the bpf program metadata in the same cache line. Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- include/linux/filter.h | 11 +++-------- init/Kconfig | 1 + kernel/bpf/core.c | 41 +++++++---------------------------------- 3 files changed, 11 insertions(+), 42 deletions(-)