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

[Dmitry Safonov]

Thanks for copying on the thread,

On Thu, 11 Sept 2025 at 14:15, Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Wed, Sep 10, 2025 at 8:49 PM Anderson Nascimento
> <anderson@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>
> <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