If we failed to assign proto idx, we free the twsk_slab_name but forget to
free the twsk_slab. Add a helper function tw_prot_cleanup() to free these
together and also use this helper function in proto_unregister().
Fixes: b45ce32135d1 ("sock: fix potential memory leak in proto_register()")
Signed-off-by: Miaohe Lin <[email protected]>
---
net/core/sock.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c
index 49cd5ffe673e..c9083ad44ea1 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net, int val)
}
#endif
+static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot)
+{
+ if (!twsk_prot)
+ return;
+ kfree(twsk_prot->twsk_slab_name);
+ twsk_prot->twsk_slab_name = NULL;
+ kmem_cache_destroy(twsk_prot->twsk_slab);
+ twsk_prot->twsk_slab = NULL;
+}
+
static void req_prot_cleanup(struct request_sock_ops *rsk_prot)
{
if (!rsk_prot)
@@ -3476,7 +3486,7 @@ int proto_register(struct proto *prot, int alloc_slab)
prot->slab_flags,
NULL);
if (prot->twsk_prot->twsk_slab == NULL)
- goto out_free_timewait_sock_slab_name;
+ goto out_free_timewait_sock_slab;
}
}
@@ -3484,15 +3494,15 @@ int proto_register(struct proto *prot, int alloc_slab)
ret = assign_proto_idx(prot);
if (ret) {
mutex_unlock(&proto_list_mutex);
- goto out_free_timewait_sock_slab_name;
+ goto out_free_timewait_sock_slab;
}
list_add(&prot->node, &proto_list);
mutex_unlock(&proto_list_mutex);
return ret;
-out_free_timewait_sock_slab_name:
+out_free_timewait_sock_slab:
if (alloc_slab && prot->twsk_prot)
- kfree(prot->twsk_prot->twsk_slab_name);
+ tw_prot_cleanup(prot->twsk_prot);
out_free_request_sock_slab:
if (alloc_slab) {
req_prot_cleanup(prot->rsk_prot);
@@ -3516,12 +3526,7 @@ void proto_unregister(struct proto *prot)
prot->slab = NULL;
req_prot_cleanup(prot->rsk_prot);
-
- if (prot->twsk_prot != NULL && prot->twsk_prot->twsk_slab != NULL) {
- kmem_cache_destroy(prot->twsk_prot->twsk_slab);
- kfree(prot->twsk_prot->twsk_slab_name);
- prot->twsk_prot->twsk_slab = NULL;
- }
+ tw_prot_cleanup(prot->twsk_prot);
}
EXPORT_SYMBOL(proto_unregister);
--
2.19.1
From: Miaohe Lin <[email protected]>
Date: Mon, 10 Aug 2020 08:16:58 -0400
> If we failed to assign proto idx, we free the twsk_slab_name but forget to
> free the twsk_slab. Add a helper function tw_prot_cleanup() to free these
> together and also use this helper function in proto_unregister().
>
> Fixes: b45ce32135d1 ("sock: fix potential memory leak in proto_register()")
> Signed-off-by: Miaohe Lin <[email protected]>
Applied and queued up for -stable, thanks.
On Mon, Aug 10, 2020 at 5:19 AM Miaohe Lin <[email protected]> wrote:
>
> If we failed to assign proto idx, we free the twsk_slab_name but forget to
> free the twsk_slab. Add a helper function tw_prot_cleanup() to free these
> together and also use this helper function in proto_unregister().
>
> Fixes: b45ce32135d1 ("sock: fix potential memory leak in proto_register()")
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> net/core/sock.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 49cd5ffe673e..c9083ad44ea1 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net, int val)
> }
> #endif
>
> +static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot)
> +{
> + if (!twsk_prot)
> + return;
> + kfree(twsk_prot->twsk_slab_name);
> + twsk_prot->twsk_slab_name = NULL;
> + kmem_cache_destroy(twsk_prot->twsk_slab);
Hmm, are you sure you can free the kmem cache name before
kmem_cache_destroy()? To me, it seems kmem_cache_destroy()
frees the name via slab_kmem_cache_release() via kfree_const().
With your patch, we have a double-free on the name?
Or am I missing anything?
Thanks.
From: Cong Wang <[email protected]>
Date: Tue, 11 Aug 2020 16:02:51 -0700
>> @@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net, int val)
>> }
>> #endif
>>
>> +static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot)
>> +{
>> + if (!twsk_prot)
>> + return;
>> + kfree(twsk_prot->twsk_slab_name);
>> + twsk_prot->twsk_slab_name = NULL;
>> + kmem_cache_destroy(twsk_prot->twsk_slab);
>
> Hmm, are you sure you can free the kmem cache name before
> kmem_cache_destroy()? To me, it seems kmem_cache_destroy()
> frees the name via slab_kmem_cache_release() via kfree_const().
> With your patch, we have a double-free on the name?
>
> Or am I missing anything?
Yep, there is a double free here.
Please fix this.
Hi all:
David Miller <[email protected]> wrote:
>From: Cong Wang <[email protected]>
>Date: Tue, 11 Aug 2020 16:02:51 -0700
>
>>> @@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net,
>>> int val) } #endif
>>>
>>> +static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot) {
>>> + if (!twsk_prot)
>>> + return;
>>> + kfree(twsk_prot->twsk_slab_name);
>>> + twsk_prot->twsk_slab_name = NULL;
>>> + kmem_cache_destroy(twsk_prot->twsk_slab);
>>
>> Hmm, are you sure you can free the kmem cache name before
>> kmem_cache_destroy()? To me, it seems kmem_cache_destroy() frees the
>> name via slab_kmem_cache_release() via kfree_const().
>> With your patch, we have a double-free on the name?
>>
>> Or am I missing anything?
>
>Yep, there is a double free here.
>
>Please fix this.
Many thanks for both of you to point this issue out. But I'am not really understand, could you please explain it more?
As far as I can see, the double free path is:
1. kfree(twsk_prot->twsk_slab_name)
2. kmem_cache_destroy
--> shutdown_memcg_caches
--> shutdown_cache
--> slab_kmem_cache_release
--> kfree_const(s->name)
But twsk_prot->twsk_slab_name is allocated from kasprintf via kmalloc_track_caller while twsk_prot->twsk_slab->name is allocated
via kstrdup_const. So I think twsk_prot->twsk_slab_name and twsk_prot->twsk_slab->name point to different memory, and there is no double free.
Or am I missing anything?
By the way, req_prot_cleanup() do the same things, i.e. free the slab_name before involve kmem_cache_destroy(). If there is a double
free, so as here?
Thanks.
On Wed, Aug 12, 2020 at 2:21 AM linmiaohe <[email protected]> wrote:
>
> Hi all:
> David Miller <[email protected]> wrote:
> >From: Cong Wang <[email protected]>
> >Date: Tue, 11 Aug 2020 16:02:51 -0700
> >
> >>> @@ -3406,6 +3406,16 @@ static void sock_inuse_add(struct net *net,
> >>> int val) } #endif
> >>>
> >>> +static void tw_prot_cleanup(struct timewait_sock_ops *twsk_prot) {
> >>> + if (!twsk_prot)
> >>> + return;
> >>> + kfree(twsk_prot->twsk_slab_name);
> >>> + twsk_prot->twsk_slab_name = NULL;
> >>> + kmem_cache_destroy(twsk_prot->twsk_slab);
> >>
> >> Hmm, are you sure you can free the kmem cache name before
> >> kmem_cache_destroy()? To me, it seems kmem_cache_destroy() frees the
> >> name via slab_kmem_cache_release() via kfree_const().
> >> With your patch, we have a double-free on the name?
> >>
> >> Or am I missing anything?
> >
> >Yep, there is a double free here.
> >
> >Please fix this.
>
> Many thanks for both of you to point this issue out. But I'am not really understand, could you please explain it more?
> As far as I can see, the double free path is:
> 1. kfree(twsk_prot->twsk_slab_name)
> 2. kmem_cache_destroy
> --> shutdown_memcg_caches
> --> shutdown_cache
> --> slab_kmem_cache_release
> --> kfree_const(s->name)
> But twsk_prot->twsk_slab_name is allocated from kasprintf via kmalloc_track_caller while twsk_prot->twsk_slab->name is allocated
> via kstrdup_const. So I think twsk_prot->twsk_slab_name and twsk_prot->twsk_slab->name point to different memory, and there is no double free.
>
You are right. Since it is duplicated, then there is no need to keep
->twsk_slab_name, we can just use twsk_slab->name. I will send
a patch to get rid of it.
> Or am I missing anything?
>
> By the way, req_prot_cleanup() do the same things, i.e. free the slab_name before involve kmem_cache_destroy(). If there is a double
> free, so as here?
Ditto. Can be just removed.
Thanks.