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

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

[Evgeny Kuznetsov]

Hi,

Here is patch which fixes bug in /init/initramfs.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):
  initramfs: strcpy destination string overflow

 init/initramfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH 1/1] initramfs: 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.
"strcpy()" is replaced by "strlcpy()" to prevent destination
string overflow.

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

diff --git a/init/initramfs.c b/init/initramfs.c
index 4b9c202..3e68568 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -56,7 +56,7 @@ static char __init *find_link(int major, int minor, int ino,
 	q->minor = minor;
 	q->ino = ino;
 	q->mode = mode;
-	strcpy(q->name, name);
+	strlcpy(q->name, name, sizeof(q->name));
 	q->next = NULL;
 	*p = q;
 	return NULL;
-- 
1.6.3.3

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

[Américo Wang]

On Tue, Oct 05, 2010 at 12:44:17PM +0400, Evgeny Kuznetsov 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.
>"strcpy()" is replaced by "strlcpy()" to prevent destination
>string overflow.
>
>Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>


Looks good to me,

Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>

>---
> init/initramfs.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/init/initramfs.c b/init/initramfs.c
>index 4b9c202..3e68568 100644
>--- a/init/initramfs.c
>+++ b/init/initramfs.c
>@@ -56,7 +56,7 @@ static char __init *find_link(int major, int minor, int ino,
> 	q->minor = minor;
> 	q->ino = ino;
> 	q->mode = mode;
>-	strcpy(q->name, name);
>+	strlcpy(q->name, name, sizeof(q->name));
> 	q->next = NULL;
> 	*p = q;
> 	return NULL;
>-- 
>1.6.3.3
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

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

[Linus Torvalds]

On Tue, Oct 5, 2010 at 1:44 AM, 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.
> "strcpy()" is replaced by "strlcpy()" to prevent destination
> string overflow.

I think this is wrong.

If the name is too long to fit in the hash table, we should not create
a new hash entry at all, because we'd be returning the wrong
(truncated) name when we find it next time.

So it would be much better to just do

   if (strlen(name) >= sizeof(q->name))
      return NULL;

in find_link(), because as far as I can tell, the hard-linking is
always just an optimization.

Comments? Peter?

                         Linus

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

[H. Peter Anvin]

On 10/05/2010 08:55 AM, Linus Torvalds wrote:
> On Tue, Oct 5, 2010 at 1:44 AM, 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.
>> "strcpy()" is replaced by "strlcpy()" to prevent destination
>> string overflow.
> 
> I think this is wrong.
> 
> If the name is too long to fit in the hash table, we should not create
> a new hash entry at all, because we'd be returning the wrong
> (truncated) name when we find it next time.
> 
> So it would be much better to just do
> 
>    if (strlen(name) >= sizeof(q->name))
>       return NULL;
> 
> in find_link(), because as far as I can tell, the hard-linking is
> always just an optimization.
> 
> Comments? Peter?

The hard-linking is unfortunately not just an optimization; if you
ignore it you will create one copy of the file and some number of
zero-length files, since the data isn't duplicated.  As such, printk'ing
an error message and ignoring it is probably the right thing to do.
Truncation is definitely wrong.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Al Viro]

On Tue, Oct 05, 2010 at 08:55:01AM -0700, Linus Torvalds wrote:

> I think this is wrong.
> 
> If the name is too long to fit in the hash table, we should not create
> a new hash entry at all, because we'd be returning the wrong
> (truncated) name when we find it next time.
> 
> So it would be much better to just do
> 
>    if (strlen(name) >= sizeof(q->name))
>       return NULL;
> 
> in find_link(), because as far as I can tell, the hard-linking is
> always just an optimization.
> 
> Comments? Peter?

Take a look at struct hash definition.  That sizeof is PATH_MAX and
do_header() will reject an entry with name longer than that.  IOW,
the whole thing is a non-issue; we can add
	if (strlen(name) >= PATH_MAX)
		BUG();
if we really care, but that's it.

As a side note, it looks like we need a fat warning about blind "improvements"
of that kind in CodingStyle; cargo-cult replacements like that can easily
hide real bugs... ;-/

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

[H. Peter Anvin]

On 10/05/2010 09:51 AM, Al Viro wrote:
> 
> Take a look at struct hash definition.  That sizeof is PATH_MAX and
> do_header() will reject an entry with name longer than that.  IOW,
> the whole thing is a non-issue; we can add
> 	if (strlen(name) >= PATH_MAX)
> 		BUG();
> if we really care, but that's it.
> 
> As a side note, it looks like we need a fat warning about blind "improvements"
> of that kind in CodingStyle; cargo-cult replacements like that can easily
> hide real bugs... ;-/

BUG_ON(strlen(name) >= PATH_MAX);

... would work for me.

OK, not an issue...

	-hpa

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

[Evgeny Kuznetsov]

On Tue, 2010-10-05 at 11:09 -0700, ext H. Peter Anvin wrote:
> On 10/05/2010 09:51 AM, Al Viro wrote:
> > 
> > Take a look at struct hash definition.  That sizeof is PATH_MAX and
> > do_header() will reject an entry with name longer than that.  IOW,
> > the whole thing is a non-issue; we can add
> > 	if (strlen(name) >= PATH_MAX)
> > 		BUG();
> > if we really care, but that's it.
> > 
> > As a side note, it looks like we need a fat warning about blind "improvements"
> > of that kind in CodingStyle; cargo-cult replacements like that can easily
> > hide real bugs... ;-/
> 
> BUG_ON(strlen(name) >= PATH_MAX);

Should it be:

BUG_ON(strlen(name) >= N_ALIGN(PATH_MAX));


-Evgeny
> 
> ... would work for me.
> 
> OK, not an issue...
> 
> 	-hpa