| 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: <20231214031819.83105-1-kuniyu@amazon.com>
Date: Thu, 14 Dec 2023 12:18:19 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <martin.lau@...ux.dev>
CC: <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
<daniel@...earbox.net>, <dxu@...uu.xyz>, <edumazet@...gle.com>,
<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v5 bpf-next 6/6] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
From: Martin KaFai Lau <martin.lau@...ux.dev>
Date: Wed, 13 Dec 2023 12:44:42 -0800
> 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.
Thanks for catching this!
Will fix.
>
> > + 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.
IIUC, the CO-RE api assumes that the offset of bitfields could be changed.
If the size of struct tcp_cookie_attributes is changed, kfunc will not work
in this test. So, BPF_CORE_WRITE_BITFIELD() works only when the size of
tcp_cookie_attributes is unchanged but fields in tcp_options_received are
rearranged or expanded to use the unused@ bits ?
Also, do we need to use BPF_CORE_READ() for other non-bitfields in
strcut tcp_options_received (and ecn_ok in struct tcp_cookie_attributes
just in case other fields are added to tcp_cookie_attributes and ecn_ok
is rearranged) ?
Just trying to understand when to use CO-RE api.
Btw, thanks for merging BPF_CORE_WRITE_BITFIELD patches!
>
> > + }
> > + 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;
> > +}
Powered by blists - more mailing lists