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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoA+5nQqdJAUYXoa=Y7KJX8LDRWQP8sBrOUfb4LMwkHrCg@mail.gmail.com>
Date: Fri, 16 Aug 2024 14:57:28 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	kuni1840@...il.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.

On Fri, Aug 16, 2024 at 11:47 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> 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().

It makes sense. I have to say that it was my concern.

After digging into this part, sorry, I can't find a easy way to
prevent double-free issues because:
initially I was trying using seq_skb (something like that) as an
indicator to reflect whether we are allowed to kfree the skb, but it
doesn't work for all the cases. Supposing there are three threads,
each of them can call kcm_sendmsg() and wait, which makes it more
complicated. Let alone more threads access this function and try to
grab the lock nearly concurrently.

Making the whole process serialised can make life easier. For sure,
introducing mutex locks can solve the issue.

Thanks,
Jason