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 11:36:35 +0800
> On Fri, Aug 16, 2024 at 11:05 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > 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<---
> 
> Oh, I see the difference of handling error part after waiting for
> memory between tcp_sendmsg_locked and kcm_sendmsg:
> In kcm_sendmsg, it could kfree the skb which causes the issue while tcp doesn't.
> 
> But I cannot help asking if that lock is a little bit heavy, please
> don't get me wrong, I'm not against it. In the meantime, I decided to
> take a deep look at the 'out_error' label part.

I don't think the mutex is heavy because kcm_sendmsg() is already
serialised with lock_sock().