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: <CAJwJo6astnZxqkpikp4OtatHLMTq8-j35nRWYcq3HjX4eH0VuA@mail.gmail.com>
Date: Thu, 11 Sep 2025 15:23:28 +0100
From: Dmitry Safonov <0x7f454c46@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, Anderson Nascimento <anderson@...elesecurity.com>
Cc: 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.

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