[PATCH] fat: fix buffer overflow in vfat_create_shortname()

[PATCH] fat: fix buffer overflow in vfat_create_shortname()

[Nikolaus Schulz]

When using the string representation of a random counter as part of the base
name, ensure that it is no longer than 4 bytes.

Since we are repeatedly decrementing the counter in a loop until we have found a
unique base name, the counter may wrap around zero; therefore, it is not enough
to mask its higher bits before entering the loop, this must be done inside the
loop.

Signed-off-by: Nikolaus Schulz <microschulz@web.de>
Cc: stable@kernel.org
---
 fs/fat/namei_vfat.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index c1ef501..a448ee5 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -309,7 +309,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
 {
 	struct fat_mount_options *opts = &MSDOS_SB(dir->i_sb)->options;
 	wchar_t *ip, *ext_start, *end, *name_start;
-	unsigned char base[9], ext[4], buf[8], *p;
+	unsigned char base[9], ext[4], buf[5], *p;
 	unsigned char charbuf[NLS_MAX_CHARSET_SIZE];
 	int chl, chi;
 	int sz = 0, extlen, baselen, i, numtail_baselen, numtail2_baselen;
@@ -467,7 +467,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
 			return 0;
 	}
 
-	i = jiffies & 0xffff;
+	i = jiffies;
 	sz = (jiffies >> 16) & 0x7;
 	if (baselen > 2) {
 		baselen = numtail2_baselen;
@@ -476,7 +476,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
 	name_res[baselen + 4] = '~';
 	name_res[baselen + 5] = '1' + sz;
 	while (1) {
-		sprintf(buf, "%04X", i);
+		sprintf(buf, "%04X", i & 0xffff);
 		memcpy(&name_res[baselen], buf, 4);
 		if (vfat_find_form(dir, name_res) < 0)
 			break;
-- 
1.7.0

Re: [PATCH] fat: fix buffer overflow in vfat_create_shortname()

[OGAWA Hirofumi]

Nikolaus Schulz <microschulz@web.de> writes:

> When using the string representation of a random counter as part of the base
> name, ensure that it is no longer than 4 bytes.
>
> Since we are repeatedly decrementing the counter in a loop until we have found a
> unique base name, the counter may wrap around zero; therefore, it is not enough
> to mask its higher bits before entering the loop, this must be done inside the
> loop.

This logic seems to still be strange after applying this patch. However,
anyway, your patch is much better off than current one. So, I'll apply
this in the next merge window.

Or should we apply this immediately?

Thanks.

> Signed-off-by: Nikolaus Schulz <microschulz@web.de>
> Cc: stable@kernel.org
> ---
>  fs/fat/namei_vfat.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index c1ef501..a448ee5 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -309,7 +309,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
>  {
>  	struct fat_mount_options *opts = &MSDOS_SB(dir->i_sb)->options;
>  	wchar_t *ip, *ext_start, *end, *name_start;
> -	unsigned char base[9], ext[4], buf[8], *p;
> +	unsigned char base[9], ext[4], buf[5], *p;
>  	unsigned char charbuf[NLS_MAX_CHARSET_SIZE];
>  	int chl, chi;
>  	int sz = 0, extlen, baselen, i, numtail_baselen, numtail2_baselen;
> @@ -467,7 +467,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
>  			return 0;
>  	}
>  
> -	i = jiffies & 0xffff;
> +	i = jiffies;
>  	sz = (jiffies >> 16) & 0x7;
>  	if (baselen > 2) {
>  		baselen = numtail2_baselen;
> @@ -476,7 +476,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
>  	name_res[baselen + 4] = '~';
>  	name_res[baselen + 5] = '1' + sz;
>  	while (1) {
> -		sprintf(buf, "%04X", i);
> +		sprintf(buf, "%04X", i & 0xffff);
>  		memcpy(&name_res[baselen], buf, 4);
>  		if (vfat_find_form(dir, name_res) < 0)
>  			break;

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: fix buffer overflow in vfat_create_shortname()

[Nikolaus Schulz]

On Thu, Mar 25, 2010 at 02:30:07PM +0900, OGAWA Hirofumi wrote:
> Nikolaus Schulz <microschulz@web.de> writes:
> 
> > When using the string representation of a random counter as part of the base
> > name, ensure that it is no longer than 4 bytes.
> >
> > Since we are repeatedly decrementing the counter in a loop until we have found a
> > unique base name, the counter may wrap around zero; therefore, it is not enough
> > to mask its higher bits before entering the loop, this must be done inside the
> > loop.
> 
> This logic seems to still be strange after applying this patch. 

Yeah, I find that code not terribly convincing either. :)

> However, anyway, your patch is much better off than current one. So,
> I'll apply this in the next merge window.
> 
> Or should we apply this immediately?
> 
> Thanks.

Given that this fixes a stack corruption which triggers the gcc stack
smashing protection and thus basically a crash, I vote for not
postponing it, but applying it immediately.

> > Signed-off-by: Nikolaus Schulz <microschulz@web.de>
> > Cc: stable@kernel.org
> > ---
> >  fs/fat/namei_vfat.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> > index c1ef501..a448ee5 100644
> > --- a/fs/fat/namei_vfat.c
> > +++ b/fs/fat/namei_vfat.c
> > @@ -309,7 +309,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
> >  {
> >  	struct fat_mount_options *opts = &MSDOS_SB(dir->i_sb)->options;
> >  	wchar_t *ip, *ext_start, *end, *name_start;
> > -	unsigned char base[9], ext[4], buf[8], *p;
> > +	unsigned char base[9], ext[4], buf[5], *p;
> >  	unsigned char charbuf[NLS_MAX_CHARSET_SIZE];
> >  	int chl, chi;
> >  	int sz = 0, extlen, baselen, i, numtail_baselen, numtail2_baselen;
> > @@ -467,7 +467,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
> >  			return 0;
> >  	}
> >  
> > -	i = jiffies & 0xffff;
> > +	i = jiffies;
> >  	sz = (jiffies >> 16) & 0x7;
> >  	if (baselen > 2) {
> >  		baselen = numtail2_baselen;
> > @@ -476,7 +476,7 @@ static int vfat_create_shortname(struct inode *dir, struct nls_table *nls,
> >  	name_res[baselen + 4] = '~';
> >  	name_res[baselen + 5] = '1' + sz;
> >  	while (1) {
> > -		sprintf(buf, "%04X", i);
> > +		sprintf(buf, "%04X", i & 0xffff);
> >  		memcpy(&name_res[baselen], buf, 4);
> >  		if (vfat_find_form(dir, name_res) < 0)
> >  			break;
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 

Re: [PATCH] fat: fix buffer overflow in vfat_create_shortname()

[OGAWA Hirofumi]

Nikolaus Schulz <microschulz@web.de> writes:

>> However, anyway, your patch is much better off than current one. So,
>> I'll apply this in the next merge window.
>> 
>> Or should we apply this immediately?
>> 
>> Thanks.
>
> Given that this fixes a stack corruption which triggers the gcc stack
> smashing protection and thus basically a crash, I vote for not
> postponing it, but applying it immediately.

Um..., where is a stack corruption? sprintf() overflow? But, it's
actually snprintf(), not sprintf()...

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: fix buffer overflow in vfat_create_shortname()

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Nikolaus Schulz <microschulz@web.de> writes:
>
>>> However, anyway, your patch is much better off than current one. So,
>>> I'll apply this in the next merge window.
>>> 
>>> Or should we apply this immediately?
>>> 
>>> Thanks.
>>
>> Given that this fixes a stack corruption which triggers the gcc stack
>> smashing protection and thus basically a crash, I vote for not
>> postponing it, but applying it immediately.
>
> Um..., where is a stack corruption? sprintf() overflow? But, it's
> actually snprintf(), not sprintf()...

Whoops, it was really sprintf().
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] fat: fix buffer overflow in vfat_create_shortname()

[Nikolaus Schulz]

On Thu, Mar 25, 2010 at 09:52:12PM +0900, OGAWA Hirofumi wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> > Nikolaus Schulz <microschulz@web.de> writes:
> >
> >>> However, anyway, your patch is much better off than current one. So,
> >>> I'll apply this in the next merge window.
> >>> 
> >>> Or should we apply this immediately?
> >>> 
> >>> Thanks.
> >>
> >> Given that this fixes a stack corruption which triggers the gcc stack
> >> smashing protection and thus basically a crash, I vote for not
> >> postponing it, but applying it immediately.
> >
> > Um..., where is a stack corruption? sprintf() overflow? But, it's
> > actually snprintf(), not sprintf()...
> 
> Whoops, it was really sprintf().

Ping?

I really think this should go into 2.6.34 and the next stable kernel
updates.

Nikolaus

[PATCH] fat: fix buffer overflow in vfat_create_shortname()

[OGAWA Hirofumi]

Nikolaus Schulz <microschulz@web.de> writes:

> Ping?
>
> I really think this should go into 2.6.34 and the next stable kernel
> updates.

I was testing a bit.

OK, please apply.


From: Nikolaus Schulz <microschulz@web.de>

When using the string representation of a random counter as part of the base
name, ensure that it is no longer than 4 bytes.

Since we are repeatedly decrementing the counter in a loop until we have found a
unique base name, the counter may wrap around zero; therefore, it is not enough
to mask its higher bits before entering the loop, this must be done inside the
loop.

[hirofumi@mail.parknet.co.jp: use snprintf()]
Signed-off-by: Nikolaus Schulz <microschulz@web.de>
Cc: stable@kernel.org
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/fat/namei_vfat.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -puN fs/fat/namei_vfat.c~fat-fix-numtail-logic fs/fat/namei_vfat.c
--- linux-2.6/fs/fat/namei_vfat.c~fat-fix-numtail-logic	2010-03-25 22:10:45.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/namei_vfat.c	2010-03-25 22:17:52.000000000 +0900
@@ -309,7 +309,7 @@ static int vfat_create_shortname(struct
 {
 	struct fat_mount_options *opts = &MSDOS_SB(dir->i_sb)->options;
 	wchar_t *ip, *ext_start, *end, *name_start;
-	unsigned char base[9], ext[4], buf[8], *p;
+	unsigned char base[9], ext[4], buf[5], *p;
 	unsigned char charbuf[NLS_MAX_CHARSET_SIZE];
 	int chl, chi;
 	int sz = 0, extlen, baselen, i, numtail_baselen, numtail2_baselen;
@@ -467,7 +467,7 @@ static int vfat_create_shortname(struct
 			return 0;
 	}
 
-	i = jiffies & 0xffff;
+	i = jiffies;
 	sz = (jiffies >> 16) & 0x7;
 	if (baselen > 2) {
 		baselen = numtail2_baselen;
@@ -476,7 +476,7 @@ static int vfat_create_shortname(struct
 	name_res[baselen + 4] = '~';
 	name_res[baselen + 5] = '1' + sz;
 	while (1) {
-		sprintf(buf, "%04X", i);
+		snprintf(buf, sizeof(buf), "%04X", i & 0xffff);
 		memcpy(&name_res[baselen], buf, 4);
 		if (vfat_find_form(dir, name_res) < 0)
 			break;
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>