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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Jan 2022 14:54:31 +0800 From: Hangyu Hua <hbh25y@...il.com> To: Eric Dumazet <eric.dumazet@...il.com>, jreuter@...na.de, ralf@...ux-mips.org, davem@...emloft.net, kuba@...nel.org Cc: linux-hams@...r.kernel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH net] ax25: use after free in ax25_connect Any suggestions for this patch ? Guys. I think putting sk_to_ax25 after lock_sock(sk) here will avoid any possilbe race conditions like other functions in ax25_proto_ops. On 2022/1/12 下午7:11, Hangyu Hua wrote: > 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@...il.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) {
Powered by blists - more mailing lists