| Message ID | 20220111042048.43532-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ax25: use after free in ax25_connect | expand |
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) {
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). 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) {
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) {
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) {
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@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) {
On 1/13/22 22:54, Hangyu Hua wrote: > 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. CTING) { > As explained, your patch is not needed. You failed to describe how a race was possible. Just moving code around wont help. How about providing a stack trace or some syzbot repro ?
I get it. Thanks. On 2022/1/14 下午11:19, Eric Dumazet wrote: > > On 1/13/22 22:54, Hangyu Hua wrote: >> 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. CTING) { >> > > As explained, your patch is not needed. > > You failed to describe how a race was possible. > > Just moving code around wont help. > > How about providing a stack trace or some syzbot repro ? >
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) {
sk_to_ax25(sk) needs to be called after lock_sock(sk) to avoid UAF caused by a race condition. Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- net/ax25/af_ax25.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)