[PATCH 0/1] cgroups: strcpy destination string overflow

[PATCH 0/1] cgroups: strcpy destination string overflow

[Evgeny Kuznetsov]

Hi,

Here is patch which fixes minor bug in /kernel/cgroup.c file.
Function "strcpy" is used without check for maximum allowed source
string length and could cause destination string overflow.

Thanks,
Best Regards,
Evgeny

Evgeny Kuznetsov (1):
  cgroups: strcpy destination string overflow

 kernel/cgroup.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

[PATCH 1/1] cgroups: strcpy destination string overflow

[Evgeny Kuznetsov]

From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>

Function "strcpy" is used without check for maximum allowed source
string length and could cause destination string overflow.
Check for string length is added before using "strcpy".
Function now is return error if source string length is more than
a maximum.

Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
---
 kernel/cgroup.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index c9483d8..82bbede 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1883,6 +1883,8 @@ static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
 				      const char *buffer)
 {
 	BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
+	if (strlen(buffer) >= PATH_MAX)
+		return -EINVAL;
 	if (!cgroup_lock_live_group(cgrp))
 		return -ENODEV;
 	strcpy(cgrp->root->release_agent_path, buffer);
-- 
1.6.3.3

Re: [PATCH 1/1] cgroups: strcpy destination string overflow

[Andrew Morton]

On Tue,  5 Oct 2010 12:38:05 +0400
Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com> wrote:

> From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>
> 
> Function "strcpy" is used without check for maximum allowed source
> string length and could cause destination string overflow.
> Check for string length is added before using "strcpy".
> Function now is return error if source string length is more than
> a maximum.
> 
> Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
> ---
>  kernel/cgroup.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index c9483d8..82bbede 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1883,6 +1883,8 @@ static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
>  				      const char *buffer)
>  {
>  	BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
> +	if (strlen(buffer) >= PATH_MAX)
> +		return -EINVAL;
>  	if (!cgroup_lock_live_group(cgrp))
>  		return -ENODEV;
>  	strcpy(cgrp->root->release_agent_path, buffer);

I don't think this can happen, because cftype.max_write_len is
PATH_MAX.

But it's pretty unobvious if this is actually true, and the code is
fragile against future changes.

Re: [PATCH 1/1] cgroups: strcpy destination string overflow

[Paul Menage]

On Tue, Oct 5, 2010 at 12:48 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue,  5 Oct 2010 12:38:05 +0400
> Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com> wrote:
>
>> From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>
>>
>> Function "strcpy" is used without check for maximum allowed source
>> string length and could cause destination string overflow.
>> Check for string length is added before using "strcpy".
>> Function now is return error if source string length is more than
>> a maximum.
>>
>> Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
>> ---
>>  kernel/cgroup.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index c9483d8..82bbede 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -1883,6 +1883,8 @@ static int cgroup_release_agent_write(struct cgroup *cgrp, struct cftype *cft,
>>                                     const char *buffer)
>>  {
>>       BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
>> +     if (strlen(buffer) >= PATH_MAX)
>> +             return -EINVAL;
>>       if (!cgroup_lock_live_group(cgrp))
>>               return -ENODEV;
>>       strcpy(cgrp->root->release_agent_path, buffer);
>
> I don't think this can happen, because cftype.max_write_len is
> PATH_MAX.

Yes, it shouldn't be possible.

>
> But it's pretty unobvious if this is actually true, and the code is
> fragile against future changes.

Fair enough - adding the check doesn't hurt anything.

Acked-by: Paul Menage <menage@google.com>