netdev - Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk

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: <87wnhq6htx.fsf@cloudflare.com>
Date:   Sat, 19 Feb 2022 15:37:01 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Ilya Leoshkevich <iii@...ux.ibm.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        kernel-team <kernel-team@...udflare.com>,
        Yonghong Song <yhs@...com>
Subject: Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from
 remote_port in bpf_sk_lookup

On Thu, Feb 17, 2022 at 05:11 PM +01, Jakub Sitnicki wrote:
> On Thu, Feb 17, 2022 at 03:18 PM +01, Ilya Leoshkevich wrote:
>> On Wed, 2022-02-16 at 13:44 -0800, Andrii Nakryiko wrote:
>>> On Wed, Feb 9, 2022 at 10:43 AM Jakub Sitnicki <jakub@...udflare.com>
>>> wrote:
>
> [...]
>
>>> > +       /* Load from remote_port field with zero padding (backward
>>> > compatibility) */
>>> > +       val_u32 = *(__u32 *)&ctx->remote_port;
>>> > +       if (val_u32 != bpf_htonl(bpf_ntohs(SRC_PORT) << 16))
>>> > +               return SK_DROP;
>>> > +
>>> 
>>> Jakub, can you please double check that your patch set doesn't break
>>> big-endian architectures? I've noticed that our s390x test runner is
>>> now failing in the sk_lookup selftest. See [0]. Also CC'ing Ilya.
>>
>> I agree that this looks like an endianness issue. The new check seems
>> to make little sense on big-endian to me, so I would just #ifdef it
>> out.
>
> We have a very similar check for a load from context in
> progs/test_sock_fields.c, which is not causing problems:
>
> static __noinline bool sk_dst_port__load_word(struct bpf_sock *sk)
> {
> 	__u32 *word = (__u32 *)&sk->dst_port;
> 	return word[0] == bpf_htonl(0xcafe0000);
> }
>
> So I think I just messed something up here. Will dig into it.

Pretty sure the source of the problem here is undefined behaviour. Can't
legally shift u16 by 16 bits like I did in the `bpf_ntohs(SRC_PORT) <<
16` expression. Will fix.