netdev - Re: [PATCH v2] net/tcp: Fix a NULL pointer dereference when using TCP-AO with TCP

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] [day] [month] [year] [list]

Message-ID: <CAPhRvkzeYFbbZwr52kSzXMOX9Z=2ab6Y4NPUBYhWrTKJRFVO-w@mail.gmail.com>
Date: Thu, 11 Sep 2025 20:25:40 -0300
From: Anderson Nascimento <anderson@...elesecurity.com>
To: Dmitry Safonov <0x7f454c46@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, ncardwell@...gle.com, kuniyu@...gle.com, 
	davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net/tcp: Fix a NULL pointer dereference when using
 TCP-AO with TCP_REPAIR.

On Thu, Sep 11, 2025 at 11:23 AM Dmitry Safonov <0x7f454c46@...il.com> wrote:
>
> Thanks for copying on the thread,
>
> On Thu, 11 Sept 2025 at 14:15, Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Wed, Sep 10, 2025 at 8:49 PM Anderson Nascimento
> > <anderson@...elesecurity.com> wrote:
> > >
> > > A NULL pointer dereference can occur in tcp_ao_finish_connect() during a
> > > connect() system call on a socket with a TCP-AO key added and TCP_REPAIR
> > > enabled.
> > >
> > > The function is called with skb being NULL and attempts to dereference it
> > > on tcp_hdr(skb)->seq without a prior skb validation.
> > >
> > > Fix this by checking if skb is NULL before dereferencing it. If skb is
> > > not NULL, the ao->risn is set to tcp_hdr(skb)->seq. If skb is NULL,
> > > ao->risn is set to 0 to keep compatibility with calls made from
> > > tcp_rcv_synsent_state_process().
> > >
> > > int main(void){
> > >         struct sockaddr_in sockaddr;
> > >         struct tcp_ao_add tcp_ao;
> > >         int sk;
> > >         int one = 1;
> > >
> > >         memset(&sockaddr,'\0',sizeof(sockaddr));
> > >         memset(&tcp_ao,'\0',sizeof(tcp_ao));
> > >
> > >         sk = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > >
> > >         sockaddr.sin_family = AF_INET;
> > >
> > >         memcpy(tcp_ao.alg_name,"cmac(aes128)",12);
> > >         memcpy(tcp_ao.key,"ABCDEFGHABCDEFGH",16);
> > >         tcp_ao.keylen = 16;
> > >
> > >         memcpy(&tcp_ao.addr,&sockaddr,sizeof(sockaddr));
> > >
> > >         setsockopt(sk, IPPROTO_TCP, TCP_AO_ADD_KEY, &tcp_ao,
> > >         sizeof(tcp_ao));
> > >         setsockopt(sk, IPPROTO_TCP, TCP_REPAIR, &one, sizeof(one));
> > >
> > >         sockaddr.sin_family = AF_INET;
> > >         sockaddr.sin_port = htobe16(123);
> > >
> > >         inet_aton("127.0.0.1", &sockaddr.sin_addr);
> > >
> > >         connect(sk,(struct sockaddr *)&sockaddr,sizeof(sockaddr));
> > >
> > > return 0;
> > > }
> > >
> > > $ gcc tcp-ao-nullptr.c -o tcp-ao-nullptr -Wall
> > > $ unshare -Urn
> > > # ip addr add 127.0.0.1 dev lo
> > > # ./tcp-ao-nullptr
> > >
> > > BUG: kernel NULL pointer dereference, address: 00000000000000b6
> > >
> >
> > CC Dmitry Safonov <0x7f454c46@...il.com>
> >
> > <cut many useless details>
> >
> > Really I do not think you need to include the crash in the changelog.
> >
> > Just mentioning a possible NULL deref should be enough, it seems
> > obvious skb can be NULL here
> > now you mention it.
>
> > -       WRITE_ONCE(ao->risn, tcp_hdr(skb)->seq);
> > +       /* sk with TCP_REPAIR_ON does not have skb in tcp_finish_connect */
> > +       if (skb)
> > +               WRITE_ONCE(ao->risn, tcp_hdr(skb)->seq);
> > +       else
> > +               WRITE_ONCE(ao->risn, 0);
>
> I think we don't need 'else' here with 0-write. It may overwrite a
> value previously set. The allocation of ao_info is kzalloc() so even
> if it wasn't - we don't leak kernel memory contents here.
>
> > Real question is : can a TCP-AO socket be fully checkpointed/restored
> > with TCP_REPAIR ?
>
> Yeah, I've missed that skb can be 0 for TCP-REPAIR on connect().
> I have selftests to checkpoint/restore a socket, but they all
> checkpoint/dump an existing tcp-ao established connection, rather than
> set TCP-REPAIR before connect().
>
> > If not, we should just reject the attempt much earlier, and add needed
> > socket options to support it in the future if there is interest.
>
> The restoration of TCP-AO state is done with TCP_AO_REPAIR
> setsockopt(), the corresponding Initial Sequence Numbers (ISNs) should
> be set by tcp_ao_repair::{snt_isn,recv_isn}.
> So, I think at this moment it's fully possible to checkpoint/restore
> AO sockets (albeit, I haven't yet spent time to add support in CRIU).
> The only thing I've missed was connect() under repair-enabled.
>
> Thanks,
>              Dmitry

Thanks Eric and Dmitry. I've just sent a v3.

https://lore.kernel.org/all/20250911230743.2551-3-anderson@allelesecurity.com/

Regards,

-- 
Anderson Nascimento
Allele Security Intelligence
https://www.allelesecurity.com