On Thu, Mar 12, 2020 at 03:27:01PM -0700, Dave Chinner wrote: > On Thu, Mar 12, 2020 at 08:01:36AM +0100, Takashi Iwai wrote: > > On Wed, 11 Mar 2020 23:09:14 +0100, > > Dave Chinner wrote: > > > > > > On Wed, Mar 11, 2020 at 10:35:52AM +0100, Takashi Iwai wrote: > > > > Since snprintf() returns the would-be-output size instead of the > > > > actual output size, the succeeding calls may go beyond the given > > > > buffer limit. Fix it by replacing with scnprintf(). > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > > > --- > > > > fs/xfs/xfs_stats.c | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > what about all the other calls to snprintf() in fs/xfs/xfs_sysfs.c > > > and fs/xfs/xfs_error.c that return the "would be written" length to > > > their callers? i.e. we can return a length longer than the buffer > > > provided to the callers... > > > > > > Aren't they all broken, too? > > > > The one in xfs_error.c is a oneshot call for a sysfs output with > > PAGE_SIZE limit, so it's obviously safe. > > Until the sysfs code changes. Then it's a landmine that explodes. It's a pity we didn't make cursor management automatic and required for sysfs/procfs/configfs/debugfs files... ...but in the meantime, Takashi-san, would you mind fixing the other snprintfs in xfs, so at least the problems get fixed for the whole subsystem? > > OTOH, using snprintf() makes > > no sense as it doesn't return the right value if it really exceeds, so > > it should be either simplified to sprintf() or use scnprintf() to > > align both the truncation and the return value. > > Right, we have technical debt here, and lots of it. scnprintf() is > the right thing to use here. > > > > A quick survey of random snprintf() calls shows there's an abundance > > > of callers that do not check the return value of snprintf for > > > overflow when outputting stuff to proc/sysfs files. This seems like > > > a case of "snprintf() considered harmful, s/snprintf/scnprintf/ > > > kernel wide, remove snprintf()"... > > > > Yeah, snprintf() is a hard-to-use function if you evaluate the return > > value. I've submitted many similar patches like this matching a > > pattern like > > pos += snprintf(buf + pos, limit - pos, ...) > > which is a higher risk of breakage than a single shot call. > > > > We may consider flagging snprintf() to be harmful, but I guess it > > wasn't done at the time scnprintf() was introduced just because there > > are too many callers of snprintf(). And some code actually needs the > > size that would be output for catching the overflow explicitly (hence > > warning or resizing after that). > > So, after seeing the technical debt the kernel has accumulated, it's > been given a "somebody else's problem to solve" label, rather than > putting in the effort to fix it. > > Basically you are saying "we know our software sucks and we don't > care enough to fix it", yes? > > > Practically seen, the recent kernel snprintf() already protects the > > negative length with WARN(). > > That's a truly awful way of handling out of bounds accesses: not > only are we saying we know our software sucks, we're telling the > user and making it their problem. It's a cop-out. > > > But it's error-prone and would hit other > > issue if you access to the buffer position by other than snprintf(), > > so please see my patch just as a precaution. > > Obviously, but slapping band-aids around like this not a fix for > all the other existing (and future) buggy snprintf code. > > I'm annoyed that every time a fundamental failing or technical debt > is uncovered in the kernel, nobody takes responsibility to fix the > problem completely, for everyone, for ever. > > As Thomas said recently: correctness first. > > https://lwn.net/ml/linux-kernel/87v9nc63io.fsf@xxxxxxxxxxxxxxxxxxxxxxx/ > > This is not "good enough" - get rid of snprintf() altogether. $ git grep snprintf | wc -l 8534 That's somebody's 20 year project... :/ --D > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
