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: <20240816034646.18670-1-kuniyu@amazon.com>
Date: Thu, 15 Aug 2024 20:46:46 -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 11:36:35 +0800
> On Fri, Aug 16, 2024 at 11:05 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> >
> > 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<---
> 
> 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().