Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     "Theodore Y. Ts'o" <tytso@mit.edu>, linux-ext4@vger.kernel.org
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Subject: comments on "ext4: don't assume that mmp_nodename/bdevname have NUL"
Message-ID: <1e3f607b-a23a-dbd6-b695-cfa0fe38d7e6@rasmusvillemoes.dk>
Date:   Tue, 14 Apr 2020 18:47:59 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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