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: <CAL+tcoCX2Si0q7HwvGspwqUeN8F1fPxocbb+BB8psQ++_2O_kg@mail.gmail.com>
Date: Fri, 16 Aug 2024 11:36:35 +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: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.

Thanks,
Jason