All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maxim Mikityanskiy <maximmi@nvidia.com>
To: Lorenz Bauer <lmb@cloudflare.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Tariq Toukan <tariqt@nvidia.com>,
	"Martin KaFai Lau" <kafai@fb.com>,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Petar Penkov <ppenkov@google.com>,
	Eric Dumazet <edumazet@google.com>
Subject: Re: [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie
Date: Mon, 31 Jan 2022 15:37:49 +0200	[thread overview]
Message-ID: <5090da78-305c-dc42-65a6-ef0b2927db51@nvidia.com> (raw)
In-Reply-To: <CACAyw9_5-T5Y9AQpAmCe=aj9A0Q=SMyx1cMz6TRQvnW=NU9ygA@mail.gmail.com>

On 2022-01-26 11:45, Lorenz Bauer wrote:
> On Mon, 24 Jan 2022 at 15:13, Maxim Mikityanskiy <maximmi@nvidia.com> wrote:
>>
>> bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie expect the full length
>> of the TCP header (with all extensions). Fix the documentation that says
>> it should be sizeof(struct tcphdr).
> 
> I don't understand this change, sorry. Are you referring to the fact
> that the check is len < sizeof(*th) instead of len != sizeof(*th)?
> 
> Your commit message makes me think that the helpers will access data
> in the extension headers, which isn't true as far as I can tell.

Yes, they will. See bpf_tcp_gen_syncookie -> tcp_v4_get_syncookie -> 
tcp_get_syncookie_mss -> tcp_parse_mss_option, which iterates over the 
TCP options ("extensions" wasn't the best word I used here). Moreover, 
bpf_tcp_gen_syncookie even checks that th_len == th->doff * 4.

Although bpf_tcp_check_syncookie doesn't need the TCP options and 
doesn't enforce them to be passed, it's still allowed.

> That
> would be a problem in fact, since it could be used to read memory that
> the verifier hasn't deemed safe.

It's safe, because bpf_tcp_gen_syncookie reads up to th_len, which is 
ARG_CONST_SIZE for the TCP header.

  reply	other threads:[~2022-01-31 13:38 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-24 15:11 [PATCH bpf v2 0/4] Bugfixes for syncookie BPF helpers Maxim Mikityanskiy
2022-01-24 15:11 ` [PATCH bpf v2 1/4] bpf: Use ipv6_only_sock in bpf_tcp_gen_syncookie Maxim Mikityanskiy
2022-01-25  6:44   ` John Fastabend
2022-01-26  9:46   ` Lorenz Bauer
2022-01-27 21:33     ` Petar Penkov
2022-01-24 15:11 ` [PATCH bpf v2 2/4] bpf: Support dual-stack sockets in bpf_tcp_check_syncookie Maxim Mikityanskiy
2022-01-25  7:04   ` John Fastabend
2022-01-26  9:49   ` Lorenz Bauer
2022-01-31 13:38     ` Maxim Mikityanskiy
2022-01-24 15:11 ` [PATCH bpf v2 3/4] bpf: Use EOPNOTSUPP " Maxim Mikityanskiy
2022-01-25  7:06   ` John Fastabend
2022-01-31 13:37     ` Maxim Mikityanskiy
2022-01-31 20:55       ` John Fastabend
2022-01-24 15:11 ` [PATCH bpf v2 4/4] bpf: Fix documentation of th_len in bpf_tcp_{gen,check}_syncookie Maxim Mikityanskiy
2022-01-25  7:09   ` John Fastabend
2022-01-26  9:45   ` Lorenz Bauer
2022-01-31 13:37     ` Maxim Mikityanskiy [this message]
2022-02-01 17:02       ` Lorenz Bauer
2022-01-25  7:12 ` [PATCH bpf v2 0/4] Bugfixes for syncookie BPF helpers John Fastabend

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5090da78-305c-dc42-65a6-ef0b2927db51@nvidia.com \
    --to=maximmi@nvidia.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=lmb@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=ppenkov@google.com \
    --cc=songliubraving@fb.com \
    --cc=tariqt@nvidia.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.