linux-kernel - Re: [PATCH] md: Remove risk of overflow via sprintf) by using snprintf() in md_check

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] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1102132118360.18930@swampdragon.chaosbits.net>
Date:	Sun, 13 Feb 2011 21:18:52 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	"Daniel K." <dk@...no>
cc:	Michael Tokarev <mjt@....msk.ru>, linux-kernel@...r.kernel.org,
	linux-raid@...r.kernel.org, Neil Brown <neilb@...e.de>,
	Neil Brown <neilb@....unsw.edu.au>
Subject: Re: [PATCH] md: Remove risk of overflow via sprintf) by using
 snprintf() in md_check_recovery()

On Sat, 12 Feb 2011, Daniel K. wrote:

> Michael Tokarev wrote:
> > 12.02.2011 12:34, Daniel K. wrote:
> > > Jesper Juhl wrote:
> > > > sprintf() is dangerous - given the wrong source string it will
> > > > overflow the destination. snprintf() is safer in that at least we'll
> > > > never overflow the destination. Even if overflow will never happen
> > > > today, code changes over time and snprintf() is just safer in the long
> > > > run.
> > > > -                        sprintf(nm,"rd%d", rdev->raid_disk);
> > > > +                        snprintf(nm, sizeof(nm), "rd%d",
> > > > rdev->raid_disk);
> > > >                          sysfs_remove_link(&mddev->kobj, nm);
> > > What if "rd1234" get truncated to "rd123" and you remove the wrong link.
> > > (No, I didn't actually bother to check how much room was allocated.)
> > 
> > That allocation is in the line above first sprintf which you deleted.
> > Sure, didn't bother, it's very difficult.
> 
> Yeah, early morning, I cut to much, and I didn't bother to look it up again,
> sorry for being lazy. Nevertheless, the actual size is of the allocation is of
> no particular importance. As you've shown, the current allocation of 20 bytes
> is more than enough.
> 
> > C'mon guys, this is pointless.  20 bytes allocated for the device
> > name, and this is for raid disk number.  It is impossible to have
> > more than 10^17 (20 bytes total, 2 for "rd" and on for the zero
> > terminator) drives in a single array.
> 
> Agreed, and this was sort of the point.
> 
> In all probability it would not overflow, and if it did, it would be better
> for it to crash and burn, than to unlink the wrong files.
> 

Point taken. Ignore the patch.

-- 
Jesper Juhl <jj@...osbits.net>            http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html

--
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/