linux-kernel - Re: [PATCH] fat: fix buffer overflow in vfat_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 25 Mar 2010 13:24:19 +0100
From:	Nikolaus Schulz <microschulz@....de>
To:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Cc:	Al Viro <viro@...iv.linux.org.uk>, Marton Balint <cus@...ekas.hu>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Kevin Dankwardt <k@...mputing.com>,
	Christoph Hellwig <hch@....de>, linux-kernel@...r.kernel.org,
	stable@...nel.org
Subject: Re: [PATCH] fat: fix buffer overflow in vfat_create_shortname()

On Thu, Mar 25, 2010 at 02:30:07PM +0900, OGAWA Hirofumi wrote:
> Nikolaus Schulz <microschulz@....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@....de>
> > Cc: stable@...nel.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@...l.parknet.co.jp>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/