Messages in this thread | | | Subject | Re: [PATCH net] ax25: use after free in ax25_connect | From | Hangyu Hua <> | Date | Wed, 12 Jan 2022 19:11:05 +0800 |
| |
Yes.
And there are two ways to release ax25, ax25_release and time expiry. I tested that ax25_release will not be invoked before ax25_connect is done by closing fd from user space. I think the reason is that __sys_connect use fdget() to protect fd. But i can't test if a function like ax25_std_heartbeat_expiry will release ax25 between sk_to_ax25(sk) and lock_sock(sk).
So i think it's better to protect sk_to_ax25(sk) by a lock. Beacause functions like ax25_release use sk_to_ax25 after a lock.
On 2022/1/12 下午5:59, Eric Dumazet wrote: > > On 1/11/22 18:13, Hangyu Hua wrote: >> I try to use ax25_release to trigger this bug like this: >> ax25_release ax25_connect >> lock_sock(sk); >> -----------------------------sk = sock->sk; >> -----------------------------ax25 = sk_to_ax25(sk); >> ax25_destroy_socket(ax25); >> release_sock(sk); >> -----------------------------lock_sock(sk); >> -----------------------------use ax25 again >> >> But i failed beacause their have large speed difference. And i >> don't have a physical device to test other function in ax25. >> Anyway, i still think there will have a function to trigger this >> race condition like ax25_destroy_timer. Beacause Any ohter >> functions in ax25_proto_ops like ax25_bind protect ax25_sock by >> lock_sock(sk). > > > For a given sk pointer, sk_to_ax25(sk) is always returning the same value, > > regardless of sk lock being held or not. > > ax25_sk(sk)->cb is set only from ax25_create() or ax25_make_new() > > ax25_connect can not be called until these operations have completed ? > > > >> >> Thanks. >> >> >> >> >> On 2022/1/12 上午4:56, Eric Dumazet wrote: >>> >>> On 1/10/22 20:20, Hangyu Hua wrote: >>>> sk_to_ax25(sk) needs to be called after lock_sock(sk) to avoid UAF >>>> caused by a race condition. >>> >>> Can you describe what race condition you have found exactly ? >>> >>> sk pointer can not change. >>> >>> >>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >>>> --- >>>> net/ax25/af_ax25.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c >>>> index cfca99e295b8..c5d62420a2a8 100644 >>>> --- a/net/ax25/af_ax25.c >>>> +++ b/net/ax25/af_ax25.c >>>> @@ -1127,7 +1127,7 @@ static int __must_check ax25_connect(struct >>>> socket *sock, >>>> struct sockaddr *uaddr, int addr_len, int flags) >>>> { >>>> struct sock *sk = sock->sk; >>>> - ax25_cb *ax25 = sk_to_ax25(sk), *ax25t; >>>> + ax25_cb *ax25, *ax25t; >>>> struct full_sockaddr_ax25 *fsa = (struct full_sockaddr_ax25 >>>> *)uaddr; >>>> ax25_digi *digi = NULL; >>>> int ct = 0, err = 0; >>>> @@ -1155,6 +1155,8 @@ static int __must_check ax25_connect(struct >>>> socket *sock, >>>> lock_sock(sk); >>>> + ax25 = sk_to_ax25(sk); >>>> + >>>> /* deal with restarts */ >>>> if (sock->state == SS_CONNECTING) { >>>> switch (sk->sk_state) {
| |