netdev - Re: [RFC v2 PATCH 2/4] ebpf: Add sg_filter

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b19a9d21-f1a6-466b-0e97-884967b62155@iogearbox.net>
Date:   Fri, 29 Jun 2018 10:18:59 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Tushar Dave <tushar.n.dave@...cle.com>, ast@...nel.org,
        davem@...emloft.net, jakub.kicinski@...ronome.com,
        quentin.monnet@...ronome.com, jiong.wang@...ronome.com,
        guro@...com, sandipan@...ux.vnet.ibm.com, john.fastabend@...il.com,
        kafai@...com, rdna@...com, brakmo@...com, netdev@...r.kernel.org,
        acme@...hat.com, sowmini.varadhan@...cle.com
Subject: Re: [RFC v2 PATCH 2/4] ebpf: Add sg_filter_run and sg helper

On 06/19/2018 08:00 PM, Tushar Dave wrote:
> When sg_filter_run() is invoked it runs the attached eBPF
> SOCKET_SG_FILTER program which deals with struct scatterlist.
> 
> In addition, this patch also adds bpf_sg_next helper function that
> allows users to retrieve the next sg element from sg list.
> 
> Signed-off-by: Tushar Dave <tushar.n.dave@...cle.com>
> Acked-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
> ---
>  include/linux/filter.h                    |  2 +
>  include/uapi/linux/bpf.h                  | 10 ++++-
>  net/core/filter.c                         | 72 +++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h            | 10 ++++-
>  tools/testing/selftests/bpf/bpf_helpers.h |  3 ++
>  5 files changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 71618b1..d176402 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1072,4 +1072,6 @@ struct bpf_sock_ops_kern {
>  					 */
>  };
>  
> +int sg_filter_run(struct sock *sk, struct scatterlist *sg);
> +
>  #endif /* __LINUX_FILTER_H__ */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index ef0a7b6..036432b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2076,6 +2076,13 @@ struct bpf_stack_build_id {
>   * 	Return
>   * 		A 64-bit integer containing the current cgroup id based
>   * 		on the cgroup within which the current task is running.
> + *
> + * int bpf_sg_next(struct bpf_scatterlist *sg)
> + *	Description
> + *		This helper allows user to retrieve next sg element from
> + *		sg list.
> + *	Return
> + *		Returns 0 on success, or a negative error in case of failure.
>   */
>  #define __BPF_FUNC_MAPPER(FN)		\
>  	FN(unspec),			\
> @@ -2158,7 +2165,8 @@ struct bpf_stack_build_id {
>  	FN(rc_repeat),			\
>  	FN(rc_keydown),			\
>  	FN(skb_cgroup_id),		\
> -	FN(get_current_cgroup_id),
> +	FN(get_current_cgroup_id),	\
> +	FN(sg_next),
>  
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>   * function eBPF program intends to call
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8f67942..702ff5b 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -121,6 +121,53 @@ int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap)
>  }
>  EXPORT_SYMBOL(sk_filter_trim_cap);
>  
> +int sg_filter_run(struct sock *sk, struct scatterlist *sg)
> +{
> +	struct sk_filter *filter;
> +	int err;
> +
> +	rcu_read_lock();
> +	filter = rcu_dereference(sk->sk_filter);
> +	if (filter) {
> +		struct bpf_scatterlist bpfsg;
> +		int num_sg;
> +
> +		if (!sg) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		num_sg = sg_nents(sg);
> +		if (num_sg <= 0) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		/* We store a reference  to the sg list so it can later used by
> +		 * eBPF helpers to retrieve the next sg element.
> +		 */
> +		bpfsg.num_sg = num_sg;
> +		bpfsg.cur_sg = 0;
> +		bpfsg.sg = sg;
> +
> +		/* For the first sg element, we store the pkt access pointers
> +		 * into start and end so eBPF program can have pkt access using
> +		 * data and data_end. The pkt access for subsequent element of
> +		 * sg list is possible when eBPF program invokes bpf_sg_next
> +		 * which takes care of setting start and end to the correct sg
> +		 * element.
> +		 */
> +		bpfsg.start = sg_virt(sg);
> +		bpfsg.end = bpfsg.start + sg->length;
> +		BPF_PROG_RUN(filter->prog, &bpfsg);
> +	}
> +out:
> +	rcu_read_unlock();
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(sg_filter_run);
> +
>  BPF_CALL_1(bpf_skb_get_pay_offset, struct sk_buff *, skb)
>  {
>  	return skb_get_poff(skb);
> @@ -3753,6 +3800,29 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>  	.arg1_type      = ARG_PTR_TO_CTX,
>  };
>  
> +BPF_CALL_1(bpf_sg_next, struct bpf_scatterlist *, bpfsg)
> +{
> +	struct scatterlist *sg = bpfsg->sg;
> +	int cur_sg = bpfsg->cur_sg;
> +
> +	cur_sg++;
> +	if (cur_sg >= bpfsg->num_sg)
> +		return -ENODATA;
> +
> +	bpfsg->cur_sg = cur_sg;
> +	bpfsg->start = sg_virt(&sg[cur_sg]);
> +	bpfsg->end = bpfsg->start + sg[cur_sg].length;
> +
> +	return 0;
> +}
> +
> +static const struct bpf_func_proto bpf_sg_next_proto = {
> +	.func		= bpf_sg_next,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_CTX,
> +};

Should be added to bpf_helper_changes_pkt_data() in order to enforce a reload
of all pkt pointers. Otherwise this is buggy in the sense that someone could only
reload pkt_end pointer in the prog while old pkt_start still points to previous
sg entry, so you would be able to access out of bounds.

Thanks,
Daniel