netdev - Re: [PATCH bpf-next v3] bpf: bpf_{g,s}etsockopt for struct bpf_sock

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: <20200501215202.GA72448@rdna-mbp.dhcp.thefacebook.com>
Date:   Fri, 1 May 2020 14:52:02 -0700
From:   Andrey Ignatov <rdna@...com>
To:     Stanislav Fomichev <sdf@...gle.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <ast@...nel.org>, <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v3] bpf: bpf_{g,s}etsockopt for struct
 bpf_sock_addr

Stanislav Fomichev <sdf@...gle.com> [Thu, 2020-04-30 16:32 -0700]:
> Currently, bpf_getsockopt and bpf_setsockopt helpers operate on the
> 'struct bpf_sock_ops' context in BPF_PROG_TYPE_SOCK_OPS program.
> Let's generalize them and make them available for 'struct bpf_sock_addr'.
> That way, in the future, we can allow those helpers in more places.
> 
> As an example, let's expose those 'struct bpf_sock_addr' based helpers to
> BPF_CGROUP_INET{4,6}_CONNECT hooks. That way we can override CC before the
> connection is made.
> 
> v3:
> * Expose custom helpers for bpf_sock_addr context instead of doing
>   generic bpf_sock argument (as suggested by Daniel). Even with
>   try_socket_lock that doesn't sleep we have a problem where context sk
>   is already locked and socket lock is non-nestable.
> 
> v2:
> * s/BPF_PROG_TYPE_CGROUP_SOCKOPT/BPF_PROG_TYPE_SOCK_OPS/
> 
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: Martin KaFai Lau <kafai@...com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>

...

>  SEC("cgroup/connect4")
>  int connect_v4_prog(struct bpf_sock_addr *ctx)
>  {
> @@ -66,6 +108,10 @@ int connect_v4_prog(struct bpf_sock_addr *ctx)
>  
>  	bpf_sk_release(sk);
>  
> +	/* Rewrite congestion control. */
> +	if (ctx->type == SOCK_STREAM && set_cc(ctx))
> +		return 0;

Hi Stas,

This new check breaks one of tests in test_sock_addr:

	root@...h-fb-vm1:/home/rdna/bpf-next/tools/testing/selftests/bpf ./test_sock_addr.sh
	...
	(test_sock_addr.c:1199: errno: Operation not permitted) Fail to connect to server
	Test case: connect4: rewrite IP & TCP port .. [FAIL]
	...
	Summary: 34 PASSED, 1 FAILED

What the test does is it sets up TCPv4 server:

	[pid   386] socket(PF_INET, SOCK_STREAM, IPPROTO_IP) = 6
	[pid   386] bind(6, {sa_family=AF_INET, sin_port=htons(4444), sin_addr=inet_addr("127.0.0.1")}, 128) = 0
	[pid   386] listen(6, 128)              = 0

Then tries to connect to a fake IPv4:port and this connect4 program
should redirect it to that TCP server, but only if every field in
context has expected value.

But after that commit program started denying the connect:

	[pid   386] socket(PF_INET, SOCK_STREAM, IPPROTO_IP) = 7
	[pid   386] connect(7, {sa_family=AF_INET, sin_port=htons(4040), sin_addr=inet_addr("192.168.1.254")}, 128) = -1 EPERM (Operation not permitted)
	(test_sock_addr.c:1201: errno: Operation not permitted) Fail to connect to server
	Test case: connect4: rewrite IP & TCP port .. [FAIL] 

I verified that commenting out this new `if` fixes the problem, but
haven't spent time root-causing it. Could you please look at it?

Thanks.


-- 
Andrey Ignatov