Re: [PATCH v1 net] calipso: Fix null-ptr-deref in calipso_req_{set,del}attr().

[Kuniyuki Iwashima <kuni1840@gmail.com>]

From: Paul Moore <paul@paul-moore.com>
Date: Tue, 17 Jun 2025 18:08:15 -0400
> On Tue, Jun 17, 2025 at 5:23 PM Kuniyuki Iwashima <kuni1840@gmail.com> wrote:
> > From: Paul Moore <paul@paul-moore.com>
> > Date: Tue, 17 Jun 2025 17:04:18 -0400
> > > On Mon, Jun 16, 2025 at 1:26 PM Kuniyuki Iwashima <kuni1840@gmail.com> wrote:
> > > >
> > > > From: Kuniyuki Iwashima <kuniyu@google.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.

Ok, will post v2 with comments and your tag.

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

Thanks!