netdev - Re: Re: [PATCH] net: do not release sk in sk_wait

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] [day] [month] [year] [list]

Message-Id: <20240827085932.905688-1-sunyiqixm@gmail.com>
Date: Tue, 27 Aug 2024 16:59:32 +0800
From: sunyiqi <sunyiqixm@...il.com>
To: kerneljasonxing@...il.com
Cc: davem@...emloft.net,
	edumazet@...gle.com,
	kuba@...nel.org,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	sunyiqixm@...il.com
Subject: Re: Re: [PATCH] net: do not release sk in sk_wait_event

On Thu, 15 Aug 2024 19:14:12 Jason Xing <kerneljasonxing@...il.com> wrote:
> On Thu, Aug 15, 2024 at 5:50 PM sunyiqi <sunyiqixm@...il.com> wrote:
> >
> > When investigating the kcm socket UAF which is also found by syzbot,
> > I found that the root cause of this problem is actually in
> > sk_wait_event.
> >
> > In sk_wait_event, sk is released and relocked and called by
> > sk_stream_wait_memory. Protocols like tcp, kcm, etc., called it in some
> > ops function like *sendmsg which will lock the sk at the beginning.
> > But sk_stream_wait_memory releases sk unexpectedly and destroy
> > the thread safety. Finally it causes the kcm sk UAF.
> >
> > If at the time when a thread(thread A) calls sk_stream_wait_memory
> > and the other thread(thread B) is waiting for lock in lock_sock,
> > thread B will successfully get the sk lock as thread A release sk lock
> > in sk_wait_event.
> >
> > The thread B may change the sk which is not thread A expecting.
> >
> > As a result, it will lead kernel to the unexpected behavior. Just like
> > the kcm sk UAF, which is actually cause by sk_wait_event in
> > sk_stream_wait_memory.
> >
> > Previous commit d9dc8b0f8b4e ("net: fix sleeping for sk_wait_event()")
> > in 2016 seems do not solved this problem. Is it necessary to release
> > sock in sk_wait_event? Or just delete it to make the protocol ops
> > thread-secure.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Link: https://syzkaller.appspot.com/bug?extid=b72d86aa5df17ce74c60
> > Signed-off-by: sunyiqi <sunyiqixm@...il.com>
> > ---
> >  include/net/sock.h | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index cce23ac4d514..08d3b204b019 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1145,7 +1145,6 @@ static inline void sock_rps_reset_rxhash(struct sock *sk)
> >
> >  #define sk_wait_event(__sk, __timeo, __condition, __wait)              \
> >         ({      int __rc, __dis = __sk->sk_disconnects;                 \
> > -               release_sock(__sk);                                     \
> >                 __rc = __condition;                                     \
> >                 if (!__rc) {                                            \
> >                         *(__timeo) = wait_woken(__wait,                 \
> > @@ -1153,7 +1152,6 @@ static inline void sock_rps_reset_rxhash(struct sock *sk)
> >                                                 *(__timeo));            \
> >                 }                                                       \
> >                 sched_annotate_sleep();                                 \
> > -               lock_sock(__sk);                                        \
> 
> Are you sure that you want the socket lock to be held possibly for a
> really long time even if it has to wait for the available memory,
> which means, during this period, other threads trying to access the
> lock will be blocked?
> 
> Thanks,
> Jason

You're correct. Sorry for my poor knowledge of kernel. But I think kcm socket
UAF vulnerability is not the last one which use kernel API correctly(?) but
result in vulnerability.

Though the kcm developer can fix(and already fixed) this issue by adding more
lock in kcm sock, but, just the same question, what's the root cause of this
issue? Is kcm socket code or sk_wait_event release the sock?

Thanks,
Yiqi Sun