linux-ext4 - comments on "ext4: don't assume that mmp

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

Message-ID: <1e3f607b-a23a-dbd6-b695-cfa0fe38d7e6@rasmusvillemoes.dk>
Date:   Tue, 14 Apr 2020 18:47:59 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     "Theodore Y. Ts'o" <tytso@....edu>, linux-ext4@...r.kernel.org
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