netdev - RE: [PATCH net 2/2] sock: redo the psock vs ULP protection check

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: <62b3206a15bc4_3937b2085a@john.notmuch>
Date:   Wed, 22 Jun 2022 07:00:10 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        Jakub Kicinski <kuba@...nel.org>, john.fastabend@...il.com,
        jakub@...udflare.com, yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        kafai@...com, songliubraving@...com, yhs@...com,
        kpsingh@...nel.org, borisp@...dia.com, cong.wang@...edance.com,
        bpf@...r.kernel.org
Subject: RE: [PATCH net 2/2] sock: redo the psock vs ULP protection check

Jakub Kicinski wrote:
> Commit 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
> has moved the inet_csk_has_ulp(sk) check from sk_psock_init() to
> the new tcp_bpf_update_proto() function. I'm guessing that this
> was done to allow creating psocks for non-inet sockets.
> 
> Unfortunately the destruction path for psock includes the ULP
> unwind, so we need to fail the sk_psock_init() itself.
> Otherwise if ULP is already present we'll notice that later,
> and call tcp_update_ulp() with the sk_proto of the ULP
> itself, which will most likely result in the ULP looping
> its callbacks.
> 
> Fixes: 8a59f9d1e3d4 ("sock: Introduce sk->sk_prot->psock_update_sk_prot()")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: john.fastabend@...il.com
> CC: jakub@...udflare.com
> CC: yoshfuji@...ux-ipv6.org
> CC: dsahern@...nel.org
> CC: ast@...nel.org
> CC: daniel@...earbox.net
> CC: andrii@...nel.org
> CC: kafai@...com
> CC: songliubraving@...com
> CC: yhs@...com
> CC: kpsingh@...nel.org
> CC: borisp@...dia.com
> CC: cong.wang@...edance.com
> CC: bpf@...r.kernel.org
> ---
>  include/net/inet_sock.h | 5 +++++
>  net/core/skmsg.c        | 5 +++++
>  net/ipv4/tcp_bpf.c      | 3 ---
>  net/tls/tls_main.c      | 2 ++
>  4 files changed, 12 insertions(+), 3 deletions(-)

Thanks Jakub this looks correct to me. I can't remember at the moment
or dig up in the email or git why it was originally moved into the
update logic.  Maybe Cong can comment seeing it was his original patch?
I seemed to have missed the error path on original review :(

>From my side though,

Reviewed-by: John Fastabend <john.fastabend@...il.com>