netdev - Re: [PATCH v5 bpf-next 6/6] selftest: bpf: Test bpf_sk_assign_tcp

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: Wed, 13 Dec 2023 12:44:42 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: Kuniyuki Iwashima <kuni1840@...il.com>, bpf@...r.kernel.org,
 netdev@...r.kernel.org, Daniel Xu <dxu@...uu.xyz>,
 Eric Dumazet <edumazet@...gle.com>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH v5 bpf-next 6/6] selftest: bpf: Test
 bpf_sk_assign_tcp_reqsk().

On 12/10/23 11:36 PM, Kuniyuki Iwashima wrote:
> This commit adds a sample selftest to demonstrate how we can use
> bpf_sk_assign_tcp_reqsk() as the backend of SYN Proxy.
> 
> The test creates IPv4/IPv6 x TCP/MPTCP connections and transfer
> messages over them on lo with BPF tc prog attached.
> 
> The tc prog will process SYN and returns SYN+ACK with the following
> ISN and TS.  In a real use case, this part will be done by other
> hosts.
> 
>          MSB                                   LSB
>    ISN:  | 31 ... 8 | 7 6 |   5 |    4 | 3 2 1 0 |
>          |   Hash_1 | MSS | ECN | SACK |  WScale |
> 
>    TS:   | 31 ... 8 |          7 ... 0           |
>          |   Random |           Hash_2           |
> 
>    WScale in SYN is reused in SYN+ACK.
> 
> The client returns ACK, and tc prog will recalculate ISN and TS
> from ACK and validate SYN Cookie.
> 
> If it's valid, the prog calls kfunc to allocate a reqsk for skb and
> configure the reqsk based on the argument created from SYN Cookie.
> 
> Later, the reqsk will be processed in cookie_v[46]_check() to create
> a connection.

The patch set looks good.

One thing I just noticed is about writing/reading bits into/from "struct 
tcp_options_received". More on this below.

[ ... ]

> +void test_tcp_custom_syncookie(void)
> +{
> +	struct test_tcp_custom_syncookie *skel;
> +	int i;
> +
> +	if (setup_netns())
> +		return;
> +
> +	skel = test_tcp_custom_syncookie__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +
> +	if (setup_tc(skel))
> +		goto destroy_skel;
> +
> +	for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
> +		skel->bss->handled_syn = false;
> +		skel->bss->handled_ack = false;
> +
> +		test__start_subtest(test_cases[i].name);


This should be tested with:

	if (!test__start_subtest(test_cases[i].name))
		continue;

to skip the create_connection(). Probably do it at the beginning of the for loop.

> +		create_connection(&test_cases[i]);
> +
> +		ASSERT_EQ(skel->bss->handled_syn, true, "SYN is not handled at tc.");
> +		ASSERT_EQ(skel->bss->handled_ack, true, "ACK is not handled at tc");
> +	}
> +
> +destroy_skel:
> +	system("tc qdisc del dev lo clsact");
> +
> +	test_tcp_custom_syncookie__destroy(skel);
> +}

[ ... ]

> +static int tcp_parse_option(__u32 index, struct tcp_syncookie *ctx)
> +{
> +	struct tcp_options_received *tcp_opt = &ctx->attr.tcp_opt;
> +	char opcode, opsize;
> +
> +	if (ctx->ptr + 1 > ctx->data_end)
> +		goto stop;
> +
> +	opcode = *ctx->ptr++;
> +
> +	if (opcode == TCPOPT_EOL)
> +		goto stop;
> +
> +	if (opcode == TCPOPT_NOP)
> +		goto next;
> +
> +	if (ctx->ptr + 1 > ctx->data_end)
> +		goto stop;
> +
> +	opsize = *ctx->ptr++;
> +
> +	if (opsize < 2)
> +		goto stop;
> +
> +	switch (opcode) {
> +	case TCPOPT_MSS:
> +		if (opsize == TCPOLEN_MSS && ctx->tcp->syn &&
> +		    ctx->ptr + (TCPOLEN_MSS - 2) < ctx->data_end)
> +			tcp_opt->mss_clamp = get_unaligned_be16(ctx->ptr);
> +		break;
> +	case TCPOPT_WINDOW:
> +		if (opsize == TCPOLEN_WINDOW && ctx->tcp->syn &&
> +		    ctx->ptr + (TCPOLEN_WINDOW - 2) < ctx->data_end) {
> +			tcp_opt->wscale_ok = 1;
> +			tcp_opt->snd_wscale = *ctx->ptr;

When writing to a bitfield of "struct tcp_options_received" which is a kernel 
struct, it needs to use the CO-RE api. The BPF_CORE_WRITE_BITFIELD has not been 
landed yet: 
https://lore.kernel.org/bpf/4d3dd215a4fd57d980733886f9c11a45e1a9adf3.1702325874.git.dxu@dxuuu.xyz/

The same for reading bitfield but BPF_CORE_READ_BITFIELD() has already been 
implemented in bpf_core_read.h

Once the BPF_CORE_WRITE_BITFIELD is landed, this test needs to be changed to use 
the BPF_CORE_{READ,WRITE}_BITFIELD.

> +		}
> +		break;
> +	case TCPOPT_TIMESTAMP:
> +		if (opsize == TCPOLEN_TIMESTAMP &&
> +		    ctx->ptr + (TCPOLEN_TIMESTAMP - 2) < ctx->data_end) {
> +			tcp_opt->saw_tstamp = 1;
> +			tcp_opt->rcv_tsval = get_unaligned_be32(ctx->ptr);
> +			tcp_opt->rcv_tsecr = get_unaligned_be32(ctx->ptr + 4);
> +
> +			if (ctx->tcp->syn && tcp_opt->rcv_tsecr)
> +				tcp_opt->tstamp_ok = 0;
> +			else
> +				tcp_opt->tstamp_ok = 1;
> +		}
> +		break;
> +	case TCPOPT_SACK_PERM:
> +		if (opsize == TCPOLEN_SACK_PERM && ctx->tcp->syn &&
> +		    ctx->ptr + (TCPOLEN_SACK_PERM - 2) < ctx->data_end)
> +			tcp_opt->sack_ok = 1;
> +		break;
> +	}
> +
> +	ctx->ptr += opsize - 2;
> +next:
> +	return 0;
> +stop:
> +	return 1;
> +}