netdev - Re: [PATCH v1 net] calipso: Fix null-ptr-deref in calipso_req

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: <CAHC9VhTPymjNwkz9FHFHQbbRMgjMQT80zj1aT+3CFDVY=Eo5wg@mail.gmail.com>
Date: Tue, 17 Jun 2025 17:04:18 -0400
From: Paul Moore <paul@...l-moore.com>
To: Kuniyuki Iwashima <kuni1840@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, Huw Davies <huw@...eweavers.com>, 
	Kuniyuki Iwashima <kuniyu@...gle.com>, netdev@...r.kernel.org, 
	linux-security-module@...r.kernel.org, syzkaller <syzkaller@...glegroups.com>, 
	John Cheung <john.cs.hey@...il.com>
Subject: Re: [PATCH v1 net] calipso: Fix null-ptr-deref in calipso_req_{set,del}attr().

On Mon, Jun 16, 2025 at 1:26 PM Kuniyuki Iwashima <kuni1840@...il.com> wrote:
>
> From: Kuniyuki Iwashima <kuniyu@...gle.com>
>
> syzkaller reported a null-ptr-deref in sock_omalloc() while allocating
> a CALIPSO option.  [0]
>
> The NULL is of struct sock, which was fetched by sk_to_full_sk() in
> calipso_req_setattr().
>
> Since commit a1a5344ddbe8 ("tcp: avoid two atomic ops for syncookies"),
> reqsk->rsk_listener could be NULL when SYN Cookie is returned to its
> client, as hinted by the leading SYN Cookie log.
>
> Here are 3 options to fix the bug:
>
>   1) Return 0 in calipso_req_setattr()
>   2) Return an error in calipso_req_setattr()
>   3) Alaways set rsk_listener
>
> 1) is no go as it bypasses LSM, but 2) effectively disables SYN Cookie
> for CALIPSO.  3) is also no go as there have been many efforts to reduce
> atomic ops and make TCP robust against DDoS.  See also commit 3b24d854cb35
> ("tcp/dccp: do not touch listener sk_refcnt under synflood").
>
> As of the blamed commit, SYN Cookie already did not need refcounting,
> and no one has stumbled on the bug for 9 years, so no CALIPSO user will
> care about SYN Cookie.
>
> Let's return an error in calipso_req_setattr() and calipso_req_delattr()
> in the SYN Cookie case.

I think that's reasonable, but I think it would be nice to have a
quick comment right before the '!sk' checks to help people who may hit
the CALIPSO/SYN-cookie issue in the future.  Maybe "/*
tcp_syncookies=2 can result in sk == NULL */" ?

> diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> index 62618a058b8f..e25ed02a54bf 100644
> --- a/net/ipv6/calipso.c
> +++ b/net/ipv6/calipso.c
> @@ -1207,6 +1207,9 @@ static int calipso_req_setattr(struct request_sock *req,
>         struct ipv6_opt_hdr *old, *new;
>         struct sock *sk = sk_to_full_sk(req_to_sk(req));
>
> +       if (!sk)
> +               return -ENOMEM;
> +
>         if (req_inet->ipv6_opt && req_inet->ipv6_opt->hopopt)
>                 old = req_inet->ipv6_opt->hopopt;
>         else
> @@ -1247,6 +1250,9 @@ static void calipso_req_delattr(struct request_sock *req)
>         struct ipv6_txoptions *txopts;
>         struct sock *sk = sk_to_full_sk(req_to_sk(req));
>
> +       if (!sk)
> +               return;
> +
>         if (!req_inet->ipv6_opt || !req_inet->ipv6_opt->hopopt)
>                 return;
>
> --
> 2.49.0

-- 
paul-moore.com