Re: [PATCH] net: do not release sk in sk_wait_event

[Jason Xing]

On Thu, Aug 15, 2024 at 5:50 PM sunyiqi <sunyiqixm@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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