netdev - Re: [PATCH v1 net] kcm: Serialise kcm_sendmsg() for the same socket.

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: <20240816030543.15051-1-kuniyu@amazon.com>
Date: Thu, 15 Aug 2024 20:05:43 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kerneljasonxing@...il.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <syzbot+b72d86aa5df17ce74c60@...kaller.appspotmail.com>,
	<tom@...bertland.com>
Subject: Re: [PATCH v1 net] kcm: Serialise kcm_sendmsg() for the same socket.

From: Jason Xing <kerneljasonxing@...il.com>
Date: Fri, 16 Aug 2024 10:56:19 +0800
> Hello Kuniyuki,
> 
> On Fri, Aug 16, 2024 at 6:05 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> >
> > syzkaller reported UAF in kcm_release(). [0]
> >
> > The scenario is
> >
> >   1. Thread A builds a skb with MSG_MORE and sets kcm->seq_skb.
> >
> >   2. Thread A resumes building skb from kcm->seq_skb but is blocked
> >      by sk_stream_wait_memory()
> >
> >   3. Thread B calls sendmsg() concurrently, finishes building kcm->seq_skb
> >      and puts the skb to the write queue
> >
> >   4. Thread A faces an error and finally frees skb that is already in the
> >      write queue
> >
> >   5. kcm_release() does double-free the skb in the write queue
> >
> > When a thread is building a MSG_MORE skb, another thread must not touch it.
> 
> Thanks for the analysis.
> 
> Since the empty skb (without payload) could cause such race and
> double-free issue, I wonder if we can clear the empty skb before
> waiting for memory,

kcm->seq_skb is set when a part of data is copied to skb, so it's not
empty.  Also, seq_skb is cleared when queued to the write queue.

The problem is one thread referencing kcm->seq_skb goes to sleep and
another thread queues the skb to the write queue.

---8<---
        if (eor) {
                bool not_busy = skb_queue_empty(&sk->sk_write_queue);

                if (head) {
                        /* Message complete, queue it on send buffer */
                        __skb_queue_tail(&sk->sk_write_queue, head);
                        kcm->seq_skb = NULL;
                        KCM_STATS_INCR(kcm->stats.tx_msgs);
                }
...
        } else {
                /* Message not complete, save state */
partial_message:
                if (head) {
                        kcm->seq_skb = head;
                        kcm_tx_msg(head)->last_skb = skb;
                }
---8<---