Switch BPF's digest to SHA256
Subsystems (8)
v2 cover Switch BPF's digest to SHA256
I can imagine future uses for the new-in-4.10 BPF digest feature that would be problematic if malicious users could produce collisions, and SHA-1 is no longer consdiered to be collision-free. Even without needing collision resistance, SHA-1 is no longer recommended for new applications. Switch the BPF digest to SHA-256 instead.
The actual switchover is trivial. Most of this series consists of cleanups to the SHA256 code to make it usable as a standalone library (since BPF should not depend on crypto).
The cleaned up library is much more user-friendly than the SHA-1 code, so this also significantly tidies up the BPF digest code.
This is intended for 4.10. If this series misses 4.10 and nothing takes its place, then we'll have an unpleasant ABI stability situation.
NB: I would be happy to make parallel changes to the SHA-512 code if the crypto folks would like for me to do so. I haven't yet because I wanted to minimize churn. Also, the changes will be essentially identical to the SHA-256 changes and I want to get the latter reviewed first.
Andy Lutomirski (8):
crypto/sha256: Factor out the parts of base API that don't use
shash_desc
crypto/sha256: Export a sha256_{init,update,final}_direct() API crypto/sha256: Build the SHA256 core separately from the crypto module
bpf: Use SHA256 instead of SHA1 for bpf digests
bpf: Avoid copying the entire BPF program when hashing it
bpf: Rename fdinfo's prog_digest to prog_sha256
net: Rename TCA*BPF_DIGEST to ..._SHA256
crypto/testmgr: Allocate only the required output size for hash tests
crypto/Kconfig | 8 ++ crypto/Makefile | 1 + crypto/sha256_direct.c | 238 +++++++++++++++++++++++++++++++++++++ crypto/sha256_generic.c | 215 ++------------------------------- crypto/testmgr.c | 9 +- include/crypto/sha.h | 24 ++++ include/crypto/sha256_base.h | 58 +++++---- include/linux/filter.h | 11 +- include/uapi/linux/pkt_cls.h | 2 +- include/uapi/linux/tc_act/tc_bpf.h | 2 +- init/Kconfig | 1 + kernel/bpf/core.c | 63 +++------- kernel/bpf/syscall.c | 2 +- net/sched/act_bpf.c | 2 +- net/sched/cls_bpf.c | 2 +- 15 files changed, 343 insertions(+), 295 deletions(-) create mode 100644 crypto/sha256_direct.c
Signature · 1 line
2.9.3
PATCH v2 1/8 crypto/sha256: Factor out the parts of base API that don't use shash_desc
I want to expose a minimal SHA256 API that can be used without the depending on the crypto core. To prepare for this, factor out the meat of the sha256_base_*() helpers.
--- include/crypto/sha256_base.h | 53 ++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 17 deletions(-)
include/crypto/sha256_base.h +36 -17
@@ -18,10 +18,8 @@typedef void (sha256_block_fn )(struct sha256_state *sst,u8 const *src,int blocks); -static inline int sha224_base_init (struct shash_desc *desc) +static inline void sha224_init_direct (struct sha256_state *sctx) { -struct sha256_state *sctx =shash_desc_ctx (desc); - sctx->state[0 ] =SHA224_H0 ; sctx->state[1 ] =SHA224_H1 ; sctx->state[2 ] =SHA224_H2 ;
@@ -31,14 +29,16 @@ static inline int sha224_base_init(struct shash_desc *desc) sctx->state[6 ] =SHA224_H6 ; sctx->state[7 ] =SHA224_H7 ; sctx->count =0 ; +} +static inline int sha224_base_init (struct shash_desc *desc) +{ +sha224_init_direct (shash_desc_ctx (desc));return 0 ; } -static inline int sha256_base_init (struct shash_desc *desc) +static inline void sha256_init_direct (struct sha256_state *sctx) { -struct sha256_state *sctx =shash_desc_ctx (desc); - sctx->state[0 ] =SHA256_H0 ; sctx->state[1 ] =SHA256_H1 ; sctx->state[2 ] =SHA256_H2 ;
@@ -48,16 +48,19 @@ static inline int sha256_base_init(struct shash_desc *desc) sctx->state[6 ] =SHA256_H6 ; sctx->state[7 ] =SHA256_H7 ; sctx->count =0 ; +} +static inline int sha256_base_init (struct shash_desc *desc) +{ +sha256_init_direct (shash_desc_ctx (desc));return 0 ; } -static inline int sha256_base_do_update (struct shash_desc *desc, -const u8 *data, -unsigned int len, -sha256_block_fn *block_fn) +static inline void __sha256_base_do_update (struct sha256_state *sctx, +const u8 *data, +unsigned int len, +sha256_block_fn *block_fn) { -struct sha256_state *sctx =shash_desc_ctx (desc);unsigned int partial = sctx->count %SHA256_BLOCK_SIZE ; sctx->count += len;
@@ -86,15 +89,21 @@ static inline int sha256_base_do_update(struct shash_desc *desc, }if (len)memcpy (sctx->buf + partial, data, len); +} +static inline int sha256_base_do_update (struct shash_desc *desc, +const u8 *data, +unsigned int len, +sha256_block_fn *block_fn) +{ +__sha256_base_do_update (shash_desc_ctx (desc), data, len, block_fn);return 0 ; } -static inline int sha256_base_do_finalize (struct shash_desc *desc, -sha256_block_fn *block_fn) +static inline void sha256_do_finalize_direct (struct sha256_state *sctx, +sha256_block_fn *block_fn) {const int bit_offset =SHA256_BLOCK_SIZE -sizeof (__be64); -struct sha256_state *sctx =shash_desc_ctx (desc);__be64 *bits = (__be64 *)(sctx->buf + bit_offset);unsigned int partial = sctx->count %SHA256_BLOCK_SIZE ;
@@ -109,14 +118,18 @@ static inline int sha256_base_do_finalize(struct shash_desc *desc,memset (sctx->buf + partial,0x0 , bit_offset - partial); *bits =cpu_to_be64 (sctx->count <<3 );block_fn (sctx, sctx->buf,1 ); +} +static inline int sha256_base_do_finalize (struct shash_desc *desc, +sha256_block_fn *block_fn) +{ +sha256_do_finalize_direct (shash_desc_ctx (desc), block_fn);return 0 ; } -static inline int sha256_base_finish (struct shash_desc *desc,u8 *out) +static inline void __sha256_base_finish (struct sha256_state *sctx, +unsigned int digest_size,u8 *out) { -unsigned int digest_size =crypto_shash_digestsize (desc->tfm); -struct sha256_state *sctx =shash_desc_ctx (desc);__be32 *digest = (__be32 *)out;int i;
@@ -124,5 +137,11 @@ static inline int sha256_base_finish(struct shash_desc *desc, u8 *out)put_unaligned_be32 (sctx->state[i], digest++); *sctx = (struct sha256_state ){}; +} + +static inline int sha256_base_finish (struct shash_desc *desc,u8 *out) +{ +__sha256_base_finish (shash_desc_ctx (desc), +crypto_shash_digestsize (desc->tfm), out);return 0 ; }
Signature · 1 line
2.9.3
PATCH v2 2/8 crypto/sha256: Export a sha256_{init,update,final}_direct() API
In reply to Andy LutomirskiThis provides a very simple interface for kernel code to use to do synchronous, unaccelerated, virtual-address-based SHA256 hashing without needing to create a crypto context.
Subsequent patches will make this work without building the crypto core and will use to avoid making BPF-based tracing depend on crypto.
--- crypto/sha256_generic.c | 31 ++++++++++++++++++++++++++----- include/crypto/sha.h | 24 ++++++++++++++++++++++++ include/crypto/sha256_base.h | 13 ------------- 3 files changed, 50 insertions(+), 18 deletions(-)
crypto/sha256_generic.c +26 -5
@@ -240,24 +240,45 @@ static void sha256_generic_block_fn(struct sha256_state *sst, u8 const *src, } } +void sha256_update_direct (struct sha256_state *sctx,const u8 *data, +unsigned int len) +{ +__sha256_base_do_update (sctx, data, len, sha256_generic_block_fn); +} +EXPORT_SYMBOL (sha256_update_direct); +int crypto_sha256_update (struct shash_desc *desc,const u8 *data,unsigned int len) { -return sha256_base_do_update (desc, data, len, sha256_generic_block_fn); +sha256_update_direct (shash_desc_ctx (desc), data, len); +return 0 ; }EXPORT_SYMBOL (crypto_sha256_update);static int sha256_final (struct shash_desc *desc,u8 *out) { -sha256_base_do_finalize (desc, sha256_generic_block_fn); -return sha256_base_finish (desc, out); +__sha256_final_direct (shash_desc_ctx (desc), +crypto_shash_digestsize (desc->tfm), out); +return 0 ; } +void __sha256_final_direct (struct sha256_state *sctx,unsigned int digest_size, +u8 *out) +{ +sha256_do_finalize_direct (sctx, sha256_generic_block_fn); +__sha256_base_finish (sctx, digest_size, out); +} +EXPORT_SYMBOL (sha256_final_direct); +int crypto_sha256_finup (struct shash_desc *desc,const u8 *data,unsigned int len,u8 *hash) { -sha256_base_do_update (desc, data, len, sha256_generic_block_fn); -return sha256_final (desc, hash); +struct sha256_state *sctx =shash_desc_ctx (desc); +unsigned int digest_size =crypto_shash_digestsize (desc->tfm); + +sha256_update_direct (sctx, data, len); +__sha256_final_direct (sctx, digest_size, hash); +return 0 ; }EXPORT_SYMBOL (crypto_sha256_finup);
include/crypto/sha.h +24 -0
@@ -88,6 +88,30 @@ struct sha512_state {u8 buf[SHA512_BLOCK_SIZE ]; }; +static inline void sha256_init_direct (struct sha256_state *sctx) +{ + sctx->state[0 ] =SHA256_H0 ; + sctx->state[1 ] =SHA256_H1 ; + sctx->state[2 ] =SHA256_H2 ; + sctx->state[3 ] =SHA256_H3 ; + sctx->state[4 ] =SHA256_H4 ; + sctx->state[5 ] =SHA256_H5 ; + sctx->state[6 ] =SHA256_H6 ; + sctx->state[7 ] =SHA256_H7 ; + sctx->count =0 ; +} + +extern void sha256_update_direct (struct sha256_state *sctx,const u8 *data, +unsigned int len); + +extern void __sha256_final_direct (struct sha256_state *sctx, +unsigned int digest_size,u8 *out); + +static inline void sha256_final_direct (struct sha256_state *sctx,u8 *out) +{ +__sha256_final_direct (sctx,SHA256_DIGEST_SIZE , out); +} +struct shash_desc ; extern int crypto_sha1_update(struct shash_desc *desc, const u8 *data,
include/crypto/sha256_base.h +0 -13
@@ -37,19 +37,6 @@ static inline int sha224_base_init(struct shash_desc *desc)return 0 ; } -static inline void sha256_init_direct (struct sha256_state *sctx) -{ - sctx->state[0 ] =SHA256_H0 ; - sctx->state[1 ] =SHA256_H1 ; - sctx->state[2 ] =SHA256_H2 ; - sctx->state[3 ] =SHA256_H3 ; - sctx->state[4 ] =SHA256_H4 ; - sctx->state[5 ] =SHA256_H5 ; - sctx->state[6 ] =SHA256_H6 ; - sctx->state[7 ] =SHA256_H7 ; - sctx->count =0 ; -} -static inline int sha256_base_init (struct shash_desc *desc) {sha256_init_direct (shash_desc_ctx (desc));
Signature · 1 line
2.9.3
PATCH v2 3/8 crypto/sha256: Build the SHA256 core separately from the crypto module
In reply to Andy LutomirskiThis just moves code around -- no code changes in this patch. This wil let BPF-based tracing link against the SHA256 core code without depending on the crypto core.
--- crypto/Kconfig | 8 ++ crypto/Makefile | 1 + crypto/sha256_direct.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++++ crypto/sha256_generic.c | 214 ------------------------------------------- 4 files changed, 247 insertions(+), 214 deletions(-) create mode 100644 crypto/sha256_direct.c
crypto/Kconfig +8 -0
@@ -10,6 +10,13 @@ config XOR_BLOCKS source "crypto/async_tx/Kconfig" # +# Cryptographic algorithms that are usable without the Crypto API. +# None of these should have visible config options. +# +config CRYPTO_SHA256_DIRECT + bool + +# # Cryptographic API Configuration # menuconfig CRYPTO
@@ -763,6 +770,7 @@ config CRYPTO_SHA512_MB config CRYPTO_SHA256 tristate "SHA224 and SHA256 digest algorithm" + select CRYPTO_SHA256_DIRECT select CRYPTO_HASH help SHA256 secure hash standard (DFIPS 180-2).
crypto/Makefile +1 -0
@@ -71,6 +71,7 @@ obj-$(CONFIG_CRYPTO_RMD160) += rmd160.o obj-$(CONFIG_CRYPTO_RMD256) += rmd256.o obj-$(CONFIG_CRYPTO_RMD320) += rmd320.o obj-$(CONFIG_CRYPTO_SHA1) += sha1_generic.o +obj-$(CONFIG_CRYPTO_SHA256_DIRECT) += sha256_direct.o obj-$(CONFIG_CRYPTO_SHA256) += sha256_generic.o obj-$(CONFIG_CRYPTO_SHA512) += sha512_generic.o obj-$(CONFIG_CRYPTO_SHA3) += sha3_generic.o
crypto/sha256_direct.c new file +238 -0
238 lines not on this page · raw message
crypto/sha256_generic.c +0 -214
232 lines not on this page · raw message
Signature · 1 line
2.9.3
PATCH v2 4/8 bpf: Use SHA256 instead of SHA1 for bpf digests
In reply to Andy LutomirskiSHA1 is considered obsolete. It is no longer considered to be collision resistant and, in general, it should not be used for new applications. Change the new-in-4.10 BPF digest to SHA-256. This means that potential future applications of the digest that need collision resistance will be able to use the BPF digest. Applications that just want a short identifier for a BPF program are welcome to truncate the digest.
This is also a cleanup IMO -- the new sha256_*_direct() API is much nicer than the old SHA1 library helpers. It will also enable incremental hashing so the BPF code can avoid calling vmalloc().
I moved the digest field to keep all of the bpf program metadata in the same cache line.
--- include/linux/filter.h | 11 +++-------- init/Kconfig | 1 + kernel/bpf/core.c | 42 ++++++++---------------------------------- 3 files changed, 12 insertions(+), 42 deletions(-)
include/linux/filter.h +3 -8
@@ -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),
init/Kconfig +1 -0
@@ -1389,6 +1389,7 @@ config HAVE_PCSPKR_PLATFORM # interpreter that classic socket filters depend on config BPF bool + select CRYPTO_SHA256_DIRECT menuconfig EXPERT bool "Configure standard kernel features (expert users)"
kernel/bpf/core.c +8 -34
60 lines not on this page · raw message
Signature · 1 line
2.9.3
PATCH v2 5/8 bpf: Avoid copying the entire BPF program when hashing it
In reply to Andy LutomirskiThe sha256 helpers can consume a message incrementally, so there's no need to allocate a buffer to store the whole blob to be hashed.
This may be a slight slowdown for very long messages because gcc can't inline the sha256_update() calls. For reasonably-sized programs, however, this should be a considerable speedup as vmalloc() is quite slow.
--- kernel/bpf/core.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)
kernel/bpf/core.c +13 -20
@@ -149,44 +149,37 @@ void __bpf_prog_free(struct bpf_prog *fp)int bpf_prog_calc_digest (struct bpf_prog *fp) {struct sha256_state sha; -u32 i, psize; -struct bpf_insn *dst; +u32 i;bool was_ld_map; -u8 *raw; - - psize =bpf_prog_insn_size (fp); - raw =vmalloc (psize); -if (!raw) -return -ENOMEM ;sha256_init_direct (&sha);/* We need to take out the map fd for the digest calculation * since they are unstable from user space side. */ - dst = (void *)raw;for (i =0 , was_ld_map = false; i < fp->len; i++) { - dst[i] = fp->insnsi[i]; +struct bpf_insn insn = fp->insnsi[i]; +if (!was_ld_map && - dst[i].code == (BPF_LD |BPF_IMM |BPF_DW ) && - dst[i].src_reg ==BPF_PSEUDO_MAP_FD ) { + insn.code == (BPF_LD |BPF_IMM |BPF_DW ) && + insn.src_reg ==BPF_PSEUDO_MAP_FD ) { was_ld_map = true; - dst[i].imm =0 ; + insn.imm =0 ; }else if (was_ld_map && - dst[i].code ==0 && - dst[i].dst_reg ==0 && - dst[i].src_reg ==0 && - dst[i].off ==0 ) { + insn.code ==0 && + insn.dst_reg ==0 && + insn.src_reg ==0 && + insn.off ==0 ) { was_ld_map = false; - dst[i].imm =0 ; + insn.imm =0 ; }else { was_ld_map = false; } + +sha256_update_direct (&sha, (const u8 *)&insn,sizeof (insn)); } -sha256_update_direct (&sha, raw, psize);sha256_final_direct (&sha, fp->digest); -vfree (raw);return 0 ; }
Signature · 1 line
2.9.3
PATCH v2 6/8 bpf: Rename fdinfo's prog_digest to prog_sha256
In reply to Andy LutomirskiThis makes it easier to add another digest algorithm down the road if needed. It also serves to force any programs that might have been written against a kernel that had 'prog_digest' to be updated.
This shouldn't violate any stable API policies, as no released kernel has ever had 'prog_digest'.
--- kernel/bpf/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/bpf/syscall.c +1 -1
@@ -694,7 +694,7 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp) seq_printf(m, "prog_type:\t%u\n" "prog_jited:\t%u\n" - "prog_digest:\t%s\n" + "prog_sha256:\t%s\n" "memlock:\t%llu\n", prog->type, prog->jited,
Signature · 1 line
2.9.3
PATCH v2 7/8 net: Rename TCA*BPF_DIGEST to ..._SHA256
In reply to Andy LutomirskiThis makes it easier to add another digest algorithm down the road if needed. It also serves to force any programs that might have been written against a kernel that had the old field name to notice the change and make any necessary changes.
This shouldn't violate any stable API policies, as no released kernel has ever had TCA*BPF_DIGEST.
--- include/uapi/linux/pkt_cls.h | 2 +- include/uapi/linux/tc_act/tc_bpf.h | 2 +- net/sched/act_bpf.c | 2 +- net/sched/cls_bpf.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
include/uapi/linux/pkt_cls.h +1 -1
@@ -397,7 +397,7 @@ enum {TCA_BPF_NAME ,TCA_BPF_FLAGS ,TCA_BPF_FLAGS_GEN , -TCA_BPF_DIGEST , +TCA_BPF_SHA256 , __TCA_BPF_MAX, };
include/uapi/linux/tc_act/tc_bpf.h +1 -1
@@ -27,7 +27,7 @@ enum {TCA_ACT_BPF_FD ,TCA_ACT_BPF_NAME ,TCA_ACT_BPF_PAD , -TCA_ACT_BPF_DIGEST , +TCA_ACT_BPF_SHA256 , __TCA_ACT_BPF_MAX, };#define TCA_ACT_BPF_MAX (__TCA_ACT_BPF_MAX - 1)
net/sched/act_bpf.c +1 -1
@@ -123,7 +123,7 @@ static int tcf_bpf_dump_ebpf_info(const struct tcf_bpf *prog, nla_put_string(skb, TCA_ACT_BPF_NAME, prog->bpf_name))return -EMSGSIZE ; - nla =nla_reserve (skb,TCA_ACT_BPF_DIGEST , + nla =nla_reserve (skb,TCA_ACT_BPF_SHA256 ,sizeof (prog->filter->digest));if (nla ==NULL )return -EMSGSIZE ;
net/sched/cls_bpf.c +1 -1
@@ -555,7 +555,7 @@ static int cls_bpf_dump_ebpf_info(const struct cls_bpf_prog *prog, nla_put_string(skb, TCA_BPF_NAME, prog->bpf_name))return -EMSGSIZE ; - nla =nla_reserve (skb,TCA_BPF_DIGEST ,sizeof (prog->filter->digest)); + nla =nla_reserve (skb,TCA_BPF_SHA256 ,sizeof (prog->filter->digest));if (nla ==NULL )return -EMSGSIZE ;
Signature · 1 line
2.9.3
Daniel Borkmann
maintainerRe: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256
Quoted 7 lines · Andy Lutomirski
This makes it easier to add another digest algorithm down the road if needed. It also serves to force any programs that might have been written against a kernel that had the old field name to notice the change and make any necessary changes.
This shouldn't violate any stable API policies, as no released kernel has ever had TCA*BPF_DIGEST.
Imho, this and patch 6/8 is not really needed. Should there ever another digest alg be used (doubt it), then you'd need a new nl attribute and fdinfo line anyway to keep existing stuff intact. Nobody made the claim that you can just change this underneath and not respecting abi for existing applications when I read from above that such apps now will get "forced" to notice a change.
Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256
Quoted 11 lines · Daniel Borkmann
Quoted 8 lines · Andy Lutomirski
This makes it easier to add another digest algorithm down the road if needed. It also serves to force any programs that might have been written against a kernel that had the old field name to notice the change and make any necessary changes.
This shouldn't violate any stable API policies, as no released kernel has ever had TCA*BPF_DIGEST.
Quoted 6 lines
Imho, this and patch 6/8 is not really needed. Should there ever another digest alg be used (doubt it), then you'd need a new nl attribute and fdinfo line anyway to keep existing stuff intact. Nobody made the claim that you can just change this underneath and not respecting abi for existing applications when I read from above that such apps now will get "forced" to notice a change.
Fair enough. I was more concerned about prerelease iproute2 versions, but maybe that's a nonissue. I'll drop these two patches.
--Andy
Daniel Borkmann
maintainerRe: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256
Hi Andy,
Quoted 18 lines · Andy Lutomirski
Quoted 10 lines · Daniel Borkmann
Quoted 8 lines · Andy Lutomirski
This makes it easier to add another digest algorithm down the road if needed. It also serves to force any programs that might have been written against a kernel that had the old field name to notice the change and make any necessary changes.
This shouldn't violate any stable API policies, as no released kernel has ever had TCA*BPF_DIGEST.
Quoted 6 lines
Imho, this and patch 6/8 is not really needed. Should there ever another digest alg be used (doubt it), then you'd need a new nl attribute and fdinfo line anyway to keep existing stuff intact. Nobody made the claim that you can just change this underneath and not respecting abi for existing applications when I read from above that such apps now will get "forced" to notice a change.
Quoted 2 lines
Fair enough. I was more concerned about prerelease iproute2 versions, but maybe that's a nonissue. I'll drop these two patches.
Ok. Sleeping over this a bit, how about a general rename into "prog_tag" for fdinfo and TCA_BPF_TAG resp. TCA_ACT_BPF_TAG for the netlink attributes, fwiw, it might reduce any assumptions on this being made? If this would be preferable, I could cook that patch against -net for renaming it?
Thanks,
Daniel
Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256
Quoted 31 lines · Daniel Borkmann
Hi Andy,
Quoted 24 lines · Andy Lutomirski
Quoted 13 lines · Daniel Borkmann
Quoted 9 lines · Andy Lutomirski
This makes it easier to add another digest algorithm down the road if needed. It also serves to force any programs that might have been written against a kernel that had the old field name to notice the change and make any necessary changes.
This shouldn't violate any stable API policies, as no released kernel has ever had TCA*BPF_DIGEST.
Quoted 6 lines
Imho, this and patch 6/8 is not really needed. Should there ever another digest alg be used (doubt it), then you'd need a new nl attribute and fdinfo line anyway to keep existing stuff intact. Nobody made the claim that you can just change this underneath and not respecting abi for existing applications when I read from above that such apps now will get "forced" to notice a change.
Quoted 2 lines
Fair enough. I was more concerned about prerelease iproute2 versions, but maybe that's a nonissue. I'll drop these two patches.
Quoted 5 lines
Ok. Sleeping over this a bit, how about a general rename into "
prog_tag" for fdinfo andTCA_BPF_TAGresp.TCA_ACT_BPF_TAGfor the netlink attributes, fwiw, it might reduce any assumptions on this being made? If this would be preferable, I could cook that patch against -net for renaming it?
That would be fine with me.
I think there are two reasonable approaches to computing the actual tag.
1. Use a standard, modern cryptographic hash. SHA-256, SHA-512, Blake2b, whatever. SHA-1 is a bad choice in part because it's partly broken and in part because the implementation in lib/ is a real mess to use (as you noticed while writing the code).
2. Use whatever algorithm you like but make the tag so short that it's obviously not collision-free. 48 or 64 bits is probably reasonable.
The intermediate versions are just asking for trouble. Alexei wants to make the tag shorter, but I admit I still don't understand why he prefers that over using a better crypto hash and letting user code truncate the tag if it wants.
Daniel Borkmann
maintainerRe: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256
Quoted 1 line · Andy Lutomirski
On Wed, Jan 11, 2017 at 1:09 AM, Daniel Borkmann <[email protected]> wrote:
[...]
Quoted 15 lines
Quoted 5 lines
Ok. Sleeping over this a bit, how about a general rename into "
prog_tag" for fdinfo andTCA_BPF_TAGresp.TCA_ACT_BPF_TAGfor the netlink attributes, fwiw, it might reduce any assumptions on this being made? If this would be preferable, I could cook that patch against -net for renaming it?That would be fine with me.
I think there are two reasonable approaches to computing the actual tag.
1. Use a standard, modern cryptographic hash. SHA-256, SHA-512, Blake2b, whatever. SHA-1 is a bad choice in part because it's partly broken and in part because the implementation in
lib/is a real mess to use (as you noticed while writing the code).
Quoted 4 lines
2. Use whatever algorithm you like but make the tag so short that it's obviously not collision-free. 48 or 64 bits is probably reasonable.
The intermediate versions are just asking for trouble.
Yeah agree, I've just sent a patch to rework this a bit and it got also reasonably small for net. Cleanups, if needed, can be done in net-next once that's pulled into it.
Thanks,
Daniel
PATCH v2 8/8 crypto/testmgr: Allocate only the required output size for hash tests
In reply to Andy LutomirskiThere are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer.
Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun.
Tested by intentionally breaking sha224 to output all 256 internally-generated bits while running on KASAN.
--- crypto/testmgr.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
crypto/testmgr.c +5 -4
@@ -265,6 +265,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, const int align_offset) {const char *algo =crypto_tfm_alg_driver_name (crypto_ahash_tfm (tfm)); +size_t digest_size =crypto_ahash_digestsize (tfm);unsigned int i, j, k, temp;struct scatterlist sg[8 ];char *result;
@@ -275,7 +276,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,char *xbuf[XBUFSIZE ];int ret = -ENOMEM ; - result =kmalloc (MAX_DIGEST_SIZE ,GFP_KERNEL ); + result =kmalloc (digest_size,GFP_KERNEL );if (!result)return ret; key =kmalloc (MAX_KEYLEN ,GFP_KERNEL );
@@ -305,7 +306,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, goto out; j++; -memset (result,0 ,MAX_DIGEST_SIZE ); +memset (result,0 , digest_size); hash_buff = xbuf[0 ]; hash_buff += align_offset;
@@ -380,7 +381,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,continue ; j++; -memset (result,0 ,MAX_DIGEST_SIZE ); +memset (result,0 , digest_size); temp =0 ;sg_init_table (sg, template[i].np);
@@ -458,7 +459,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,continue ; j++; -memset (result,0 ,MAX_DIGEST_SIZE ); +memset (result,0 , digest_size); ret = -EINVAL ; hash_buff = xbuf[0 ];
Signature · 1 line
2.9.3
RE: [PATCH v2 8/8] crypto/testmgr: Allocate only the required output size for hash tests
From: Andy Lutomirski
Quoted 9 lines
Sent: 10 January 2017 23:25
There are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer.Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun.
Might be better to test this by allocating an overlong buffer and then explicitly checking that the output hasn't overrun the allowed space.
If nothing else the error message will be clearer.
David
Re: [PATCH v2 8/8] crypto/testmgr: Allocate only the required output size for hash tests
Quoted 11 lines · David Laight
From: Andy Lutomirski
Quoted 9 lines
Sent: 10 January 2017 23:25
There are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer.Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun.
Quoted 5 lines
Might be better to test this by allocating an overlong buffer and then explicitly checking that the output hasn't overrun the allowed space.
If nothing else the error message will be clearer.
I thought about that, but then I'd have to figure out what poison value to use. Both KASAN and the usual slab debugging are quite good at this kind of checking, and KASAN will even point you to the problematic code directly.
--Andy
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Herbert Xu
maintainerRe: [PATCH v2 8/8] crypto/testmgr: Allocate only the required output size for hash tests
In reply to Andy LutomirskiQuoted 9 lines · Andy Lutomirski
There are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer.
Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun.
Quoted 6 lines
Tested by intentionally breaking sha224 to output all 256 internally-generated bits while running on KASAN.
Cc Ard Biesheuvel <[email protected]>Cc Herbert Xu <[email protected]>Signed-off-by Andy Lutomirski <[email protected]>
This patch doesn't seem to depend on anything else in the series. Do you want me to take it separately?
Cheers,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 8/8] crypto/testmgr: Allocate only the required output size for hash tests
Quoted 13 lines · Herbert Xu
Quoted 12 lines · Andy Lutomirski
There are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer.
Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun.
Tested by intentionally breaking sha224 to output all 256 internally-generated bits while running on KASAN.
Quoted 6 lines
Quoted 3 lines
Cc Ard Biesheuvel <[email protected]>Cc Herbert Xu <[email protected]>Signed-off-by Andy Lutomirski <[email protected]>This patch doesn't seem to depend on anything else in the series. Do you want me to take it separately?
Yes, please. Its only relation to the rest of the series is that I wanted to make sure that I didn't mess up sha224's finalization code.
--Andy
Herbert Xu
maintainerRe: [PATCH v2 8/8] crypto/testmgr: Allocate only the required output size for hash tests
In reply to Andy LutomirskiQuoted 9 lines · Andy Lutomirski
There are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer.
Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun.
Quoted 6 lines
Tested by intentionally breaking sha224 to output all 256 internally-generated bits while running on KASAN.
Cc Ard Biesheuvel <[email protected]>Cc Herbert Xu <[email protected]>Signed-off-by Andy Lutomirski <[email protected]>
Patch applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 5 lines · Andy Lutomirski
I can imagine future uses for the new-in-4.10 BPF digest feature that would be problematic if malicious users could produce collisions, and SHA-1 is no longer consdiered to be collision-free. Even without needing collision resistance, SHA-1 is no longer recommended for new applications. Switch the BPF digest to SHA-256 instead.
Using this reasoning we must immediately change 'git' to use sha256 as well. malicious users are coming!
Seriously, NACK for bpf bits again, since you somehow missed the reasons I gave earlier, here they are again:
.......
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.
...........
tldr: I see only Cons for sha1->sha256 switch. Not a single Pro, hence nack for bpf patches 4+
The patches 1-3 are nice and useful on their own.