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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Fri, 16 Aug 2024 10:56:19 +0800
> Hello Kuniyuki,
> 
> On Fri, Aug 16, 2024 at 6:05 AM Kuniyuki Iwashima <kuniyu@amazon.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<---