| 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: <Y08KUZbmHdvtE3Ih@google.com>
Date: Tue, 18 Oct 2022 13:19:29 -0700
From: sdf@...gle.com
To: Kees Cook <keescook@...omium.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Jesper Dangaard Brouer <hawk@...nel.org>,
bpf@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] bpf, test_run: Track allocation size of data
On 10/18, Kees Cook wrote:
> In preparation for requiring that build_skb() have a non-zero size
> argument, track the data allocation size explicitly and pass it into
> build_skb(). To retain the original result of using the ksize()
> side-effect on the skb size, explicitly round up the size during
> allocation.
Can you share more on the end goal? Is the plan to remove ksize(data)
from build_skb and pass it via size argument?
> Cc: Alexei Starovoitov <ast@...nel.org>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: Andrii Nakryiko <andrii@...nel.org>
> Cc: Martin KaFai Lau <martin.lau@...ux.dev>
> Cc: Song Liu <song@...nel.org>
> Cc: Yonghong Song <yhs@...com>
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: KP Singh <kpsingh@...nel.org>
> Cc: Stanislav Fomichev <sdf@...gle.com>
> Cc: Hao Luo <haoluo@...gle.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: Jesper Dangaard Brouer <hawk@...nel.org>
> Cc: bpf@...r.kernel.org
> Cc: netdev@...r.kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> net/bpf/test_run.c | 84 +++++++++++++++++++++++++---------------------
> 1 file changed, 46 insertions(+), 38 deletions(-)
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 13d578ce2a09..299ff102f516 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -762,28 +762,38 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref,
> KF_TRUSTED_ARGS)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
> BTF_SET8_END(test_sk_check_kfunc_ids)
> -static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> - u32 size, u32 headroom, u32 tailroom)
> +struct bpfalloc {
> + size_t len;
> + void *data;
> +};
> +
> +static int bpf_test_init(struct bpfalloc *alloc,
> + const union bpf_attr *kattr, u32 user_size,
> + u32 size, u32 headroom, u32 tailroom)
> {
> void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> - void *data;
> if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
> if (user_size > size)
> - return ERR_PTR(-EMSGSIZE);
> + return -EMSGSIZE;
> - data = kzalloc(size + headroom + tailroom, GFP_USER);
> - if (!data)
> - return ERR_PTR(-ENOMEM);
[..]
> + alloc->len = kmalloc_size_roundup(size + headroom + tailroom);
> + alloc->data = kzalloc(alloc->len, GFP_USER);
I still probably miss something. Here, why do we need to do
kmalloc_size_roundup+kzalloc vs doing kzalloc+ksize?
data = bpf_test_init(kattr, kattr->test.data_size_in,
size, NET_SKB_PAD + NET_IP_ALIGN,
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
skb = build_skb(data, ksize(data));
> + if (!alloc->data) {
> + alloc->len = 0;
> + return -ENOMEM;
> + }
> - if (copy_from_user(data + headroom, data_in, user_size)) {
> - kfree(data);
> - return ERR_PTR(-EFAULT);
> + if (copy_from_user(alloc->data + headroom, data_in, user_size)) {
> + kfree(alloc->data);
> + alloc->data = NULL;
> + alloc->len = 0;
> + return -EFAULT;
> }
> - return data;
> + return 0;
> }
> int bpf_prog_test_run_tracing(struct bpf_prog *prog,
> @@ -1086,25 +1096,25 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> u32 size = kattr->test.data_size_in;
> u32 repeat = kattr->test.repeat;
> struct __sk_buff *ctx = NULL;
> + struct bpfalloc alloc = { };
> u32 retval, duration;
> int hh_len = ETH_HLEN;
> struct sk_buff *skb;
> struct sock *sk;
> - void *data;
> int ret;
> if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
> return -EINVAL;
> - data = bpf_test_init(kattr, kattr->test.data_size_in,
> - size, NET_SKB_PAD + NET_IP_ALIGN,
> - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> - if (IS_ERR(data))
> - return PTR_ERR(data);
> + ret = bpf_test_init(&alloc, kattr, kattr->test.data_size_in,
> + size, NET_SKB_PAD + NET_IP_ALIGN,
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> + if (ret)
> + return ret;
> ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> if (IS_ERR(ctx)) {
> - kfree(data);
> + kfree(alloc.data);
> return PTR_ERR(ctx);
> }
> @@ -1124,15 +1134,15 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> sk = sk_alloc(net, AF_UNSPEC, GFP_USER, &bpf_dummy_proto, 1);
> if (!sk) {
> - kfree(data);
> + kfree(alloc.data);
> kfree(ctx);
> return -ENOMEM;
> }
> sock_init_data(NULL, sk);
> - skb = build_skb(data, 0);
> + skb = build_skb(alloc.data, alloc.len);
> if (!skb) {
> - kfree(data);
> + kfree(alloc.data);
> kfree(ctx);
> sk_free(sk);
> return -ENOMEM;
> @@ -1283,10 +1293,10 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> u32 repeat = kattr->test.repeat;
> struct netdev_rx_queue *rxqueue;
> struct skb_shared_info *sinfo;
> + struct bpfalloc alloc = {};
> struct xdp_buff xdp = {};
> int i, ret = -EINVAL;
> struct xdp_md *ctx;
> - void *data;
> if (prog->expected_attach_type == BPF_XDP_DEVMAP ||
> prog->expected_attach_type == BPF_XDP_CPUMAP)
> @@ -1329,16 +1339,14 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> size = max_data_sz;
> }
> - data = bpf_test_init(kattr, size, max_data_sz, headroom, tailroom);
> - if (IS_ERR(data)) {
> - ret = PTR_ERR(data);
> + ret = bpf_test_init(&alloc, kattr, size, max_data_sz, headroom,
> tailroom);
> + if (ret)
> goto free_ctx;
> - }
> rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev,
> 0);
> rxqueue->xdp_rxq.frag_size = headroom + max_data_sz + tailroom;
> xdp_init_buff(&xdp, rxqueue->xdp_rxq.frag_size, &rxqueue->xdp_rxq);
> - xdp_prepare_buff(&xdp, data, headroom, size, true);
> + xdp_prepare_buff(&xdp, alloc.data, headroom, size, true);
> sinfo = xdp_get_shared_info_from_buff(&xdp);
> ret = xdp_convert_md_to_buff(ctx, &xdp);
> @@ -1410,7 +1418,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> free_data:
> for (i = 0; i < sinfo->nr_frags; i++)
> __free_page(skb_frag_page(&sinfo->frags[i]));
> - kfree(data);
> + kfree(alloc.data);
> free_ctx:
> kfree(ctx);
> return ret;
> @@ -1441,10 +1449,10 @@ int bpf_prog_test_run_flow_dissector(struct
> bpf_prog *prog,
> u32 repeat = kattr->test.repeat;
> struct bpf_flow_keys *user_ctx;
> struct bpf_flow_keys flow_keys;
> + struct bpfalloc alloc = {};
> const struct ethhdr *eth;
> unsigned int flags = 0;
> u32 retval, duration;
> - void *data;
> int ret;
> if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
> @@ -1453,18 +1461,18 @@ int bpf_prog_test_run_flow_dissector(struct
> bpf_prog *prog,
> if (size < ETH_HLEN)
> return -EINVAL;
> - data = bpf_test_init(kattr, kattr->test.data_size_in, size, 0, 0);
> - if (IS_ERR(data))
> - return PTR_ERR(data);
> + ret = bpf_test_init(&alloc, kattr, kattr->test.data_size_in, size, 0,
> 0);
> + if (ret)
> + return ret;
> - eth = (struct ethhdr *)data;
> + eth = (struct ethhdr *)alloc.data;
> if (!repeat)
> repeat = 1;
> user_ctx = bpf_ctx_init(kattr, sizeof(struct bpf_flow_keys));
> if (IS_ERR(user_ctx)) {
> - kfree(data);
> + kfree(alloc.data);
> return PTR_ERR(user_ctx);
> }
> if (user_ctx) {
> @@ -1475,8 +1483,8 @@ int bpf_prog_test_run_flow_dissector(struct
> bpf_prog *prog,
> }
> ctx.flow_keys = &flow_keys;
> - ctx.data = data;
> - ctx.data_end = (__u8 *)data + size;
> + ctx.data = alloc.data;
> + ctx.data_end = (__u8 *)alloc.data + size;
> bpf_test_timer_enter(&t);
> do {
> @@ -1496,7 +1504,7 @@ int bpf_prog_test_run_flow_dissector(struct
> bpf_prog *prog,
> out:
> kfree(user_ctx);
> - kfree(data);
> + kfree(alloc.data);
> return ret;
> }
> --
> 2.34.1
Powered by blists - more mailing lists