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: <CAHC9VhQ0trrgHyJxQOqAQAeN2bCsCx0JeXQgj_xeQbcckCbdZg@mail.gmail.com>
Date: Tue, 17 Jun 2025 18:08:15 -0400
From: Paul Moore <paul@...l-moore.com>
To: Kuniyuki Iwashima <kuni1840@...il.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, 
	horms@...nel.org, huw@...eweavers.com, john.cs.hey@...il.com, kuba@...nel.org, 
	kuniyu@...gle.com, linux-security-module@...r.kernel.org, 
	netdev@...r.kernel.org, pabeni@...hat.com, syzkaller@...glegroups.com
Subject: Re: [PATCH v1 net] calipso: Fix null-ptr-deref in calipso_req_{set,del}attr().

On Tue, Jun 17, 2025 at 5:23 PM Kuniyuki Iwashima <kuni1840@...il.com> wrote:
> From: Paul Moore <paul@...l-moore.com>
> Date: Tue, 17 Jun 2025 17:04:18 -0400
> > 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 */" ?
>
> tcp_syncookies=1 enables SYN cookie and =2 forces it for every request.
> I just used =2 to reproduce the issue without SYN flooding, so it would
> be /* sk is NULL for SYN+ACK w/ SYN Cookie */

Sure, that sounds good.

> But I think no one will hit it (at least so for 9 years) and wonder why
> because SYN could be dropped randomly under such a event.

Yes, you are probably correct, but that doesn't mean a brief comment
as described above isn't a good idea.  If you add the comment and
you've got my ACK.

Acked-by: Paul Moore <paul@...l-moore.com>

-- 
paul-moore.com