2020-04-14 16:56:36

by Rasmus Villemoes

[permalink] [raw]
Subject: comments on "ext4: don't assume that mmp_nodename/bdevname have NUL"

The patch in $subject (now commit 14c9ca0583ee in mainline) says
snprintf does not guarantee nul-termination. If that was ever the case,
that is a bug in snprintf that needs fixing, but I don't believe that
ever happens. If called with a non-zero (well, and sane, i.e. less than
INT_MAX) buffer size, the kernel's snprintf is guaranteed to produce a
nul-terminated string.

It's true that mmp_bdevname is filled in via snprintf (via bdevname ->
disk_name), and bdevname assumes given a buffer of size BDEVNAME_SIZE,
so certainly the added BUILD_BUG makes sense. But perhaps the struct
member should just be sized BDEVNAME_SIZE instead of 32 (unless that
leads to #include madness). [If I'm reading the code right, and that's a
big if, the bdevname() in kmmpd() is redundant as the very same
mmp->mmp_bdevname was already filled in when the kthread was created.]

However, mmp_nodename is filled via a memcpy from
init_utsname()->nodename - the latter is actually (AFAICT from the code
in kernel/sys.c) always nul-terminated, but we're only copying 64 of the
65 bytes, so of course the copy may end up without a nul-terminator in
those 64 bytes. In that sense the commit does fix a potential problem,
but it has nothing to do with snprintf().

Rasmus