netdev - Re: [bpf-next PATCH 1/2] bpf: allow sk_msg programs to read sock fields

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]

Date:   Thu, 17 May 2018 11:17:38 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     John Fastabend <john.fastabend@...il.com>
CC:     <ast@...nel.org>, <daniel@...earbox.net>, <netdev@...r.kernel.org>
Subject: Re: [bpf-next PATCH 1/2] bpf: allow sk_msg programs to read sock
 fields

On Thu, May 17, 2018 at 08:54:04AM -0700, John Fastabend wrote:
> Currently sk_msg programs only have access to the raw data. However,
> it is often useful when building policies to have the policies specific
> to the socket endpoint. This allows using the socket tuple as input
> into filters, etc.
> 
> This patch adds ctx access to the sock fields.
> 
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---
>  include/linux/filter.h   |    1 
>  include/uapi/linux/bpf.h |    8 +++
>  kernel/bpf/sockmap.c     |    1 
>  net/core/filter.c        |  114 +++++++++++++++++++++++++++++++++++++++++++++-
It is indeed a lot of dup lines with sock_ops_convert_ctx_access()
as you mentioned in the cover.

Other than that, LGTM.

Acked-by: Martin KaFai Lau <kafafi@...com>

>  4 files changed, 121 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 9dbcb9d..d358d18 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -517,6 +517,7 @@ struct sk_msg_buff {
>  	bool sg_copy[MAX_SKB_FRAGS];
>  	__u32 flags;
>  	struct sock *sk_redir;
> +	struct sock *sk;
>  	struct sk_buff *skb;
>  	struct list_head list;
>  };
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d94d333..97446bb 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2176,6 +2176,14 @@ enum sk_action {
>  struct sk_msg_md {
>  	void *data;
>  	void *data_end;
> +
> +	__u32 family;
> +	__u32 remote_ip4;	/* Stored in network byte order */
> +	__u32 local_ip4;	/* Stored in network byte order */
> +	__u32 remote_ip6[4];	/* Stored in network byte order */
> +	__u32 local_ip6[4];	/* Stored in network byte order */
> +	__u32 remote_port;	/* Stored in network byte order */
> +	__u32 local_port;	/* stored in host byte order */
This ordering inconsistency could be a trap to write bpf_prog
but I guess it is too late to change now considering
bpf_sock_ops is also using this convention.

Just curious, we cannot always assume inet_sk and then uses
its inet_sport?