lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <91a6d5a7-7b18-48a2-9a74-7c00509467f8@linux.dev> Date: Wed, 1 Nov 2023 22:50:54 +0000 From: Vadim Fedorenko <vadim.fedorenko@...ux.dev> To: Martin KaFai Lau <martin.lau@...ux.dev>, "David S. Miller" <davem@...emloft.net>, Herbert Xu <herbert@...dor.apana.org.au> Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, linux-crypto@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>, Andrii Nakryiko <andrii@...nel.org>, Alexei Starovoitov <ast@...nel.org>, Mykola Lysenko <mykolal@...com>, Vadim Fedorenko <vadfed@...a.com> Subject: Re: [PATCH bpf-next v3 1/2] bpf: add skcipher API support to TC/XDP programs On 01/11/2023 21:49, Martin KaFai Lau wrote: > On 10/31/23 6:48 AM, Vadim Fedorenko wrote: >> --- /dev/null >> +++ b/kernel/bpf/crypto.c >> @@ -0,0 +1,280 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright (c) 2023 Meta, Inc */ >> +#include <linux/bpf.h> >> +#include <linux/bpf_mem_alloc.h> >> +#include <linux/btf.h> >> +#include <linux/btf_ids.h> >> +#include <linux/filter.h> >> +#include <linux/scatterlist.h> >> +#include <linux/skbuff.h> >> +#include <crypto/skcipher.h> >> + >> +/** >> + * struct bpf_crypto_skcipher_ctx - refcounted BPF sync skcipher >> context structure >> + * @tfm: The pointer to crypto_sync_skcipher struct. >> + * @rcu: The RCU head used to free the crypto context with RCU >> safety. >> + * @usage: Object reference counter. When the refcount goes to 0, the >> + * memory is released back to the BPF allocator, which provides >> + * RCU safety. >> + */ >> +struct bpf_crypto_skcipher_ctx { >> + struct crypto_sync_skcipher *tfm; >> + struct rcu_head rcu; >> + refcount_t usage; >> +}; >> + >> +__diag_push(); >> +__diag_ignore_all("-Wmissing-prototypes", >> + "Global kfuncs as their definitions will be in BTF"); >> + >> +static void *__bpf_dynptr_data_ptr(const struct bpf_dynptr_kern *ptr) >> +{ >> + enum bpf_dynptr_type type; >> + >> + if (!ptr->data) >> + return NULL; >> + >> + type = bpf_dynptr_get_type(ptr); >> + >> + switch (type) { >> + case BPF_DYNPTR_TYPE_LOCAL: >> + case BPF_DYNPTR_TYPE_RINGBUF: >> + return ptr->data + ptr->offset; >> + case BPF_DYNPTR_TYPE_SKB: >> + return skb_pointer_if_linear(ptr->data, ptr->offset, >> __bpf_dynptr_size(ptr)); >> + case BPF_DYNPTR_TYPE_XDP: >> + { >> + void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset, >> __bpf_dynptr_size(ptr)); > > I suspect what it is doing here (for skb and xdp in particular) is very > similar to bpf_dynptr_slice. Please check if bpf_dynptr_slice(ptr, 0, > NULL, sz) will work. > Well, yes, it's simplified version of bpf_dynptr_slice. The problem is that bpf_dynptr_slice bpf_kfunc which cannot be used in another bpf_kfunc. Should I refactor the code to use it in both places? Like create __bpf_dynptr_slice() which will be internal part of bpf_kfunc? > >> + if (!IS_ERR_OR_NULL(xdp_ptr)) >> + return xdp_ptr; >> + >> + return NULL; >> + } >> + default: >> + WARN_ONCE(true, "unknown dynptr type %d\n", type); >> + return NULL; >> + } >> +} >> + >> +/** >> + * bpf_crypto_skcipher_ctx_create() - Create a mutable BPF crypto >> context. >> + * >> + * Allocates a crypto context that can be used, acquired, and >> released by >> + * a BPF program. The crypto context returned by this function must >> either >> + * be embedded in a map as a kptr, or freed with >> bpf_crypto_skcipher_ctx_release(). >> + * >> + * bpf_crypto_skcipher_ctx_create() allocates memory using the BPF >> memory >> + * allocator, and will not block. It may return NULL if no memory is >> available. >> + * @algo: bpf_dynptr which holds string representation of algorithm. >> + * @key: bpf_dynptr which holds cipher key to do crypto. >> + */ >> +__bpf_kfunc struct bpf_crypto_skcipher_ctx * >> +bpf_crypto_skcipher_ctx_create(const struct bpf_dynptr_kern *palgo, > > Song's patch on __const_str should help on the palgo (which is a string) > also: > https://lore.kernel.org/bpf/20231024235551.2769174-4-song@kernel.org/ > Got it, I'll update it. >> + const struct bpf_dynptr_kern *pkey, int *err) >> +{ >> + struct bpf_crypto_skcipher_ctx *ctx; >> + char *algo; >> + >> + if (__bpf_dynptr_size(palgo) > CRYPTO_MAX_ALG_NAME) { >> + *err = -EINVAL; >> + return NULL; >> + } >> + >> + algo = __bpf_dynptr_data_ptr(palgo); >> + >> + if (!crypto_has_skcipher(algo, CRYPTO_ALG_TYPE_SKCIPHER, >> CRYPTO_ALG_TYPE_MASK)) { >> + *err = -EOPNOTSUPP; >> + return NULL; >> + } >> + >> + ctx = kmalloc(sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) { >> + *err = -ENOMEM; >> + return NULL; >> + } >> + >> + memset(ctx, 0, sizeof(*ctx)); > > nit. kzalloc() > >> + >> + ctx->tfm = crypto_alloc_sync_skcipher(algo, 0, 0); >> + if (IS_ERR(ctx->tfm)) { >> + *err = PTR_ERR(ctx->tfm); >> + ctx->tfm = NULL; >> + goto err; >> + } >> + >> + *err = crypto_sync_skcipher_setkey(ctx->tfm, >> __bpf_dynptr_data_ptr(pkey), >> + __bpf_dynptr_size(pkey)); >> + if (*err) >> + goto err; >> + >> + refcount_set(&ctx->usage, 1); >> + >> + return ctx; >> +err: >> + if (ctx->tfm) >> + crypto_free_sync_skcipher(ctx->tfm); >> + kfree(ctx); >> + >> + return NULL; >> +} > > [ ... ] > >> +static int bpf_crypto_skcipher_crypt(struct crypto_sync_skcipher *tfm, >> + const struct bpf_dynptr_kern *src, >> + struct bpf_dynptr_kern *dst, >> + const struct bpf_dynptr_kern *iv, >> + bool decrypt) >> +{ >> + struct skcipher_request *req = NULL; >> + struct scatterlist sgin, sgout; >> + int err; >> + >> + if (crypto_sync_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY) >> + return -EINVAL; >> + >> + if (__bpf_dynptr_is_rdonly(dst)) >> + return -EINVAL; >> + >> + if (!__bpf_dynptr_size(dst) || !__bpf_dynptr_size(src)) >> + return -EINVAL; >> + >> + if (__bpf_dynptr_size(iv) != crypto_sync_skcipher_ivsize(tfm)) >> + return -EINVAL; >> + >> + req = skcipher_request_alloc(&tfm->base, GFP_ATOMIC); > > Doing alloc per packet may kill performance. Is it possible to optimize > it somehow? What is the usual size of the req (e.g. the example in the > selftest)? > In ktls code aead_request is allocated every time encryption is invoked, see tls_decrypt_sg(), apparently per skb. Doesn't look like performance killer. For selftest it's only sizeof(struct skcipher_request). >> + if (!req) >> + return -ENOMEM; >> + >> + sg_init_one(&sgin, __bpf_dynptr_data_ptr(src), >> __bpf_dynptr_size(src)); >> + sg_init_one(&sgout, __bpf_dynptr_data_ptr(dst), >> __bpf_dynptr_size(dst)); >> + >> + skcipher_request_set_crypt(req, &sgin, &sgout, >> __bpf_dynptr_size(src), >> + __bpf_dynptr_data_ptr(iv)); >> + >> + err = decrypt ? crypto_skcipher_decrypt(req) : >> crypto_skcipher_encrypt(req); > > I am hitting this with the selftest in patch 2: > > [ 39.806675] ============================= > [ 39.807243] WARNING: suspicious RCU usage > [ 39.807825] 6.6.0-rc7-02091-g17e023652cc1 #336 Tainted: G O > [ 39.808798] ----------------------------- > [ 39.809368] kernel/sched/core.c:10149 Illegal context switch in > RCU-bh read-side critical section! > [ 39.810589] > [ 39.810589] other info that might help us debug this: > [ 39.810589] > [ 39.811696] > [ 39.811696] rcu_scheduler_active = 2, debug_locks = 1 > [ 39.812616] 2 locks held by test_progs/1769: > [ 39.813249] #0: ffffffff84dce140 (rcu_read_lock){....}-{1:3}, at: > ip6_finish_output2+0x625/0x1b70 > [ 39.814539] #1: ffffffff84dce1a0 (rcu_read_lock_bh){....}-{1:3}, at: > __dev_queue_xmit+0x1df/0x2c40 > [ 39.815813] > [ 39.815813] stack backtrace: > [ 39.816441] CPU: 1 PID: 1769 Comm: test_progs Tainted: G O > 6.6.0-rc7-02091-g17e023652cc1 #336 > [ 39.817774] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 > [ 39.819312] Call Trace: > [ 39.819655] <TASK> > [ 39.819967] dump_stack_lvl+0x130/0x1d0 > [ 39.822669] dump_stack+0x14/0x20 > [ 39.823136] lockdep_rcu_suspicious+0x220/0x350 > [ 39.823777] __might_resched+0xe0/0x660 > [ 39.827915] __might_sleep+0x89/0xf0 > [ 39.828423] skcipher_walk_virt+0x55/0x120 > [ 39.828990] crypto_ecb_decrypt+0x159/0x310 > [ 39.833846] crypto_skcipher_decrypt+0xa0/0xd0 > [ 39.834481] bpf_crypto_skcipher_crypt+0x29a/0x3c0 > [ 39.837100] bpf_crypto_skcipher_decrypt+0x56/0x70 > [ 39.837770] bpf_prog_fa576505ab32d186_decrypt_sanity+0x180/0x185 > [ 39.838627] cls_bpf_classify+0x3b6/0xf80 > [ 39.839807] tcf_classify+0x2f4/0x550 > That's odd. skcipher_walk_virt() checks for SKCIPHER_WALK_SLEEP which depends on CRYPTO_TFM_REQ_MAY_SLEEP of tfm, which shouldn't be set for crypto_alloc_sync_skcipher(). I think we need some crypto@ folks to jump in and explain. David, Herbert could you please take a look at the patchset to confirm that crypto API is used properly. >> + >> + skcipher_request_free(req); >> + >> + return err; >> +} >> + > >
Powered by blists - more mailing lists