* [PATCH] ipv4: ping: Fix potential use-after-free bug
@ 2022-09-16 10:07 Liang He
2022-09-18 15:30 ` David Ahern
0 siblings, 1 reply; 3+ messages in thread
From: Liang He @ 2022-09-16 10:07 UTC (permalink / raw)
To: davem, yoshfuji, dsahern, edumazet, kuba, pabeni, netdev; +Cc: windhl
In ping_unhash(), we should move sock_put(sk) after any possible
access point as the put function may free the object.
Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
Signed-off-by: Liang He <windhl@126.com>
---
I have found other places containing similar code patterns.
net/ipv4/ping.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index b83c2bd9d722..f90c86d37ffc 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -157,10 +157,10 @@ void ping_unhash(struct sock *sk)
spin_lock(&ping_table.lock);
if (sk_hashed(sk)) {
hlist_nulls_del_init_rcu(&sk->sk_nulls_node);
- sock_put(sk);
isk->inet_num = 0;
isk->inet_sport = 0;
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+ sock_put(sk);
}
spin_unlock(&ping_table.lock);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ipv4: ping: Fix potential use-after-free bug
2022-09-16 10:07 [PATCH] ipv4: ping: Fix potential use-after-free bug Liang He
@ 2022-09-18 15:30 ` David Ahern
2022-09-19 3:00 ` Liang He
0 siblings, 1 reply; 3+ messages in thread
From: David Ahern @ 2022-09-18 15:30 UTC (permalink / raw)
To: Liang He, davem, yoshfuji, edumazet, kuba, pabeni, netdev
On 9/16/22 4:07 AM, Liang He wrote:
> In ping_unhash(), we should move sock_put(sk) after any possible
> access point as the put function may free the object.
unhash handlers are called from sk_common_release which still has a
reference on the sock, so not really going to hit a UAF.
I do agree that it does not read correctly to 'put' a reference then
continue using the object. ie., the put should be moved to the end like
you have here. This is more of a tidiness exercise than a need to
backport to stable kernels.
>
> Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
> Signed-off-by: Liang He <windhl@126.com>
> ---
>
> I have found other places containing similar code patterns.
>
> net/ipv4/ping.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index b83c2bd9d722..f90c86d37ffc 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -157,10 +157,10 @@ void ping_unhash(struct sock *sk)
> spin_lock(&ping_table.lock);
> if (sk_hashed(sk)) {
> hlist_nulls_del_init_rcu(&sk->sk_nulls_node);
> - sock_put(sk);
> isk->inet_num = 0;
> isk->inet_sport = 0;
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> + sock_put(sk);
> }
> spin_unlock(&ping_table.lock);
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re:Re: [PATCH] ipv4: ping: Fix potential use-after-free bug
2022-09-18 15:30 ` David Ahern
@ 2022-09-19 3:00 ` Liang He
0 siblings, 0 replies; 3+ messages in thread
From: Liang He @ 2022-09-19 3:00 UTC (permalink / raw)
To: David Ahern; +Cc: davem, yoshfuji, edumazet, kuba, pabeni, netdev
At 2022-09-18 23:30:21, "David Ahern" <dsahern@kernel.org> wrote:
>On 9/16/22 4:07 AM, Liang He wrote:
>> In ping_unhash(), we should move sock_put(sk) after any possible
>> access point as the put function may free the object.
>
>unhash handlers are called from sk_common_release which still has a
>reference on the sock, so not really going to hit a UAF.
>
Thanks for this valuable lesson.
>I do agree that it does not read correctly to 'put' a reference then
>continue using the object. ie., the put should be moved to the end like
>you have here. This is more of a tidiness exercise than a need to
>backport to stable kernels.
>
OK, thanks.
>>
>> Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>
>> I have found other places containing similar code patterns.
>>
>> net/ipv4/ping.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
>> index b83c2bd9d722..f90c86d37ffc 100644
>> --- a/net/ipv4/ping.c
>> +++ b/net/ipv4/ping.c
>> @@ -157,10 +157,10 @@ void ping_unhash(struct sock *sk)
>> spin_lock(&ping_table.lock);
>> if (sk_hashed(sk)) {
>> hlist_nulls_del_init_rcu(&sk->sk_nulls_node);
>> - sock_put(sk);
>> isk->inet_num = 0;
>> isk->inet_sport = 0;
>> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>> + sock_put(sk);
>> }
>> spin_unlock(&ping_table.lock);
>> }
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-19 3:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-16 10:07 [PATCH] ipv4: ping: Fix potential use-after-free bug Liang He
2022-09-18 15:30 ` David Ahern
2022-09-19 3:00 ` Liang He
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.