* [PATCH] seccomp: Release fp pointer when leaving from seccomp_attach_filter().
@ 2014-04-14 16:02 Masami Ichikawa
2014-04-14 16:18 ` Alexei Starovoitov
0 siblings, 1 reply; 4+ messages in thread
From: Masami Ichikawa @ 2014-04-14 16:02 UTC (permalink / raw)
To: keescook, davem, josh, eparis, rashika.kheria, ast, linux-kernel, netdev
Cc: masami256
kmemleak reported some memory leak as below.
unreferenced object 0xffff8800d6ea4000 (size 512):
comm "sshd", pid 278, jiffies 4294898315 (age 46.653s)
hex dump (first 32 bytes):
21 00 00 00 04 00 00 00 15 00 01 00 3e 00 00 c0 !...........>...
06 00 00 00 00 00 00 00 21 00 00 00 00 00 00 00 ........!.......
backtrace:
[<ffffffff8151414e>] kmemleak_alloc+0x4e/0xb0
[<ffffffff811a3a40>] __kmalloc+0x280/0x320
[<ffffffff8110842e>] prctl_set_seccomp+0x11e/0x3b0
[<ffffffff8107bb6b>] SyS_prctl+0x3bb/0x4a0
[<ffffffff8152ef2d>] system_call_fastpath+0x1a/0x1f
[<ffffffffffffffff>] 0xffffffffffffffff
This memory leak happend in seccomp_attach_filter().
The fp pointer was allocated via kzalloc so that it needs to realase memory
when leaving from function.
This patch changed two things.
One is set -ENOMEM to ret, if fp is unable to get memory.
The other is removes "return 0" statement, and frees fp pointer before
leaving.
Signed-off-by: Masami Ichikawa <masami256@gmail.com>
---
kernel/seccomp.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index d8d046c..a9ce7a9 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -259,8 +259,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
filter = kzalloc(sizeof(struct seccomp_filter) +
sizeof(struct sock_filter_int) * new_len,
GFP_KERNEL|__GFP_NOWARN);
- if (!filter)
+ if (!filter) {
+ ret = -ENOMEM;
goto free_prog;
+ }
ret = sk_convert_filter(fp, fprog->len, filter->insnsi, &new_len);
if (ret)
@@ -275,10 +277,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
*/
filter->prev = current->seccomp.filter;
current->seccomp.filter = filter;
- return 0;
free_filter:
- kfree(filter);
+ if (ret)
+ kfree(filter);
free_prog:
kfree(fp);
return ret;
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] seccomp: Release fp pointer when leaving from seccomp_attach_filter().
2014-04-14 16:02 [PATCH] seccomp: Release fp pointer when leaving from seccomp_attach_filter() Masami Ichikawa
@ 2014-04-14 16:18 ` Alexei Starovoitov
2014-04-14 17:08 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Alexei Starovoitov @ 2014-04-14 16:18 UTC (permalink / raw)
To: Masami Ichikawa
Cc: Kees Cook, David S. Miller, josh, eparis, rashika.kheria, LKML,
Network Development
On Mon, Apr 14, 2014 at 9:02 AM, Masami Ichikawa <masami256@gmail.com> wrote:
> kmemleak reported some memory leak as below.
grrr. yes. sorry.
> unreferenced object 0xffff8800d6ea4000 (size 512):
> comm "sshd", pid 278, jiffies 4294898315 (age 46.653s)
> hex dump (first 32 bytes):
> 21 00 00 00 04 00 00 00 15 00 01 00 3e 00 00 c0 !...........>...
> 06 00 00 00 00 00 00 00 21 00 00 00 00 00 00 00 ........!.......
> backtrace:
> [<ffffffff8151414e>] kmemleak_alloc+0x4e/0xb0
> [<ffffffff811a3a40>] __kmalloc+0x280/0x320
> [<ffffffff8110842e>] prctl_set_seccomp+0x11e/0x3b0
> [<ffffffff8107bb6b>] SyS_prctl+0x3bb/0x4a0
> [<ffffffff8152ef2d>] system_call_fastpath+0x1a/0x1f
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> This memory leak happend in seccomp_attach_filter().
> The fp pointer was allocated via kzalloc so that it needs to realase memory
> when leaving from function.
>
> This patch changed two things.
> One is set -ENOMEM to ret, if fp is unable to get memory.
> The other is removes "return 0" statement, and frees fp pointer before
> leaving.
>
> Signed-off-by: Masami Ichikawa <masami256@gmail.com>
> ---
> kernel/seccomp.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index d8d046c..a9ce7a9 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -259,8 +259,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
> filter = kzalloc(sizeof(struct seccomp_filter) +
> sizeof(struct sock_filter_int) * new_len,
> GFP_KERNEL|__GFP_NOWARN);
> - if (!filter)
> + if (!filter) {
> + ret = -ENOMEM;
> goto free_prog;
> + }
agree. that's a good addition.
> ret = sk_convert_filter(fp, fprog->len, filter->insnsi, &new_len);
> if (ret)
> @@ -275,10 +277,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
> */
> filter->prev = current->seccomp.filter;
> current->seccomp.filter = filter;
> - return 0;
I think mixing error and ok return paths is ugly.
Can you add kfree(fp) here instead of removing return 0?
Thanks!
> free_filter:
> - kfree(filter);
> + if (ret)
> + kfree(filter);
> free_prog:
> kfree(fp);
> return ret;
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] seccomp: Release fp pointer when leaving from seccomp_attach_filter().
2014-04-14 16:18 ` Alexei Starovoitov
@ 2014-04-14 17:08 ` Kees Cook
2014-04-14 23:06 ` Masami Ichikawa
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2014-04-14 17:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Masami Ichikawa, David S. Miller, Josh Triplett, Eric Paris,
Rashika Kheria, LKML, Network Development
On Mon, Apr 14, 2014 at 9:18 AM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Mon, Apr 14, 2014 at 9:02 AM, Masami Ichikawa <masami256@gmail.com> wrote:
>> kmemleak reported some memory leak as below.
>
> grrr. yes. sorry.
>
>> unreferenced object 0xffff8800d6ea4000 (size 512):
>> comm "sshd", pid 278, jiffies 4294898315 (age 46.653s)
>> hex dump (first 32 bytes):
>> 21 00 00 00 04 00 00 00 15 00 01 00 3e 00 00 c0 !...........>...
>> 06 00 00 00 00 00 00 00 21 00 00 00 00 00 00 00 ........!.......
>> backtrace:
>> [<ffffffff8151414e>] kmemleak_alloc+0x4e/0xb0
>> [<ffffffff811a3a40>] __kmalloc+0x280/0x320
>> [<ffffffff8110842e>] prctl_set_seccomp+0x11e/0x3b0
>> [<ffffffff8107bb6b>] SyS_prctl+0x3bb/0x4a0
>> [<ffffffff8152ef2d>] system_call_fastpath+0x1a/0x1f
>> [<ffffffffffffffff>] 0xffffffffffffffff
>>
>> This memory leak happend in seccomp_attach_filter().
>> The fp pointer was allocated via kzalloc so that it needs to realase memory
>> when leaving from function.
Thanks for the catch!
>> This patch changed two things.
>> One is set -ENOMEM to ret, if fp is unable to get memory.
>> The other is removes "return 0" statement, and frees fp pointer before
>> leaving.
>>
>> Signed-off-by: Masami Ichikawa <masami256@gmail.com>
>> ---
>> kernel/seccomp.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index d8d046c..a9ce7a9 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -259,8 +259,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
>> filter = kzalloc(sizeof(struct seccomp_filter) +
>> sizeof(struct sock_filter_int) * new_len,
>> GFP_KERNEL|__GFP_NOWARN);
>> - if (!filter)
>> + if (!filter) {
>> + ret = -ENOMEM;
>> goto free_prog;
>> + }
>
> agree. that's a good addition.
>
>> ret = sk_convert_filter(fp, fprog->len, filter->insnsi, &new_len);
>> if (ret)
>> @@ -275,10 +277,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
>> */
>> filter->prev = current->seccomp.filter;
>> current->seccomp.filter = filter;
>> - return 0;
>
> I think mixing error and ok return paths is ugly.
> Can you add kfree(fp) here instead of removing return 0?
>
> Thanks!
>
>> free_filter:
>> - kfree(filter);
>> + if (ret)
>> + kfree(filter);
>> free_prog:
>> kfree(fp);
>> return ret;
>> --
>> 1.9.1
>>
Yeah, I'd prefer a different approach that follows the existing
conventions in the code. I'll send a separate patch.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] seccomp: Release fp pointer when leaving from seccomp_attach_filter().
2014-04-14 17:08 ` Kees Cook
@ 2014-04-14 23:06 ` Masami Ichikawa
0 siblings, 0 replies; 4+ messages in thread
From: Masami Ichikawa @ 2014-04-14 23:06 UTC (permalink / raw)
To: Kees Cook
Cc: Alexei Starovoitov, David S. Miller, Josh Triplett, Eric Paris,
Rashika Kheria, LKML, Network Development
On Tue, Apr 15, 2014 at 2:08 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Apr 14, 2014 at 9:18 AM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Mon, Apr 14, 2014 at 9:02 AM, Masami Ichikawa <masami256@gmail.com> wrote:
>>> kmemleak reported some memory leak as below.
>>
>> grrr. yes. sorry.
>>
>>> unreferenced object 0xffff8800d6ea4000 (size 512):
>>> comm "sshd", pid 278, jiffies 4294898315 (age 46.653s)
>>> hex dump (first 32 bytes):
>>> 21 00 00 00 04 00 00 00 15 00 01 00 3e 00 00 c0 !...........>...
>>> 06 00 00 00 00 00 00 00 21 00 00 00 00 00 00 00 ........!.......
>>> backtrace:
>>> [<ffffffff8151414e>] kmemleak_alloc+0x4e/0xb0
>>> [<ffffffff811a3a40>] __kmalloc+0x280/0x320
>>> [<ffffffff8110842e>] prctl_set_seccomp+0x11e/0x3b0
>>> [<ffffffff8107bb6b>] SyS_prctl+0x3bb/0x4a0
>>> [<ffffffff8152ef2d>] system_call_fastpath+0x1a/0x1f
>>> [<ffffffffffffffff>] 0xffffffffffffffff
>>>
>>> This memory leak happend in seccomp_attach_filter().
>>> The fp pointer was allocated via kzalloc so that it needs to realase memory
>>> when leaving from function.
>
> Thanks for the catch!
>
>>> This patch changed two things.
>>> One is set -ENOMEM to ret, if fp is unable to get memory.
>>> The other is removes "return 0" statement, and frees fp pointer before
>>> leaving.
>>>
>>> Signed-off-by: Masami Ichikawa <masami256@gmail.com>
>>> ---
>>> kernel/seccomp.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>>> index d8d046c..a9ce7a9 100644
>>> --- a/kernel/seccomp.c
>>> +++ b/kernel/seccomp.c
>>> @@ -259,8 +259,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
>>> filter = kzalloc(sizeof(struct seccomp_filter) +
>>> sizeof(struct sock_filter_int) * new_len,
>>> GFP_KERNEL|__GFP_NOWARN);
>>> - if (!filter)
>>> + if (!filter) {
>>> + ret = -ENOMEM;
>>> goto free_prog;
>>> + }
>>
>> agree. that's a good addition.
>>
>>> ret = sk_convert_filter(fp, fprog->len, filter->insnsi, &new_len);
>>> if (ret)
>>> @@ -275,10 +277,10 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
>>> */
>>> filter->prev = current->seccomp.filter;
>>> current->seccomp.filter = filter;
>>> - return 0;
>>
>> I think mixing error and ok return paths is ugly.
I troubled that have two kfree(fp) path or merge error and success path.
But separate paths is easy to read.
>> Can you add kfree(fp) here instead of removing return 0?
>>
>> Thanks!
>>
>>> free_filter:
>>> - kfree(filter);
>>> + if (ret)
>>> + kfree(filter);
>>> free_prog:
>>> kfree(fp);
>>> return ret;
>>> --
>>> 1.9.1
>>>
>
> Yeah, I'd prefer a different approach that follows the existing
> conventions in the code. I'll send a separate patch.
I see.
Thank you for considering it.
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
Cheeers,
--
Masami Ichikawa
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-14 23:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-14 16:02 [PATCH] seccomp: Release fp pointer when leaving from seccomp_attach_filter() Masami Ichikawa
2014-04-14 16:18 ` Alexei Starovoitov
2014-04-14 17:08 ` Kees Cook
2014-04-14 23:06 ` Masami Ichikawa
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.