On Dec 31, 2019, at 3:07 PM, Theodore Y. Ts'o <tytso@xxxxxxx> wrote:
>
> While clearing some compiler in e2fsprogs, I noticed that we are a bit
> inconsistent about the mmp_nodename and mmp_bdevname fields, and
> whether they are guaranteed to be null terminated or not. The kernel
> is using them in some printf contexts where it's assumed they are null
> terminated; but in other places, we have been filling them such that
> if the string is exactly 64 or 32 bytes, they will *not* necessarily
> be null terminated.
>
> This is potentially a problem both in the kernel as well as in
> e2fsprogs. I propose that we solve this problem by assuming that they
> *are* null terminated. But that means that if there are node names
> which are exactly 64 bytes long, or block device names which are
> exactly 32 bytes long, badness could happen.
>
> On the other hand, we kind of have this problem already, since in the
> kernel, we are using memcmp when comparing mmp_nodename, but in
> e2fsprogs userspace, we are using gethostbyname and there is no
> guarantee that bytes beyond the terminating NULL have been cleared.
> As a result I'm not sure the interlock between e2fsprogs and the
> kernel works in all cases anyway.
>
> Or we could go the other way, and try to fix all of the locations
> which are accessing and writing mmp_nodename and mmp_bdevname so that
> they are considered non-strings which are NULL padded.
Hi Ted,
it is worthwhile to note that mmp_nodename is not really a critical part
of the correctness of the MMP checking, but mostly so that admins have a
chance to figure out which other node is accessing the filesystem in a
complex SAN environment. The one place that it is checked in the kernel
with memcmp() it should only ever be "the same" as it was before the MMP
thread slept for longer than it should have, so consistency between user
and kernel versions or old and new versions do not really matter.
My thinking is that since some of the strings _may_ not be NUL terminated
(e.g. from an earlier version of the kernel or user tools) that the safest
approach is to assume there is no trailing NUL, and use a printf string
format like:
"%.*s", sizeof(mmp->mmp_nodename), mmp->mmp_nodename,
to ensure it doesn't overflow the string buffer. Fortunately, there are
a lot of '\0' values immediately following those strings, so in the worst
case, "mmp_nodename" may also print out "mmp_bdevname" (which is likely
NUL terminated, or the (likely) 0 high byte of mmp_check_interval, or
mmp_pad1, ... so there is very little risk of a very bad string access
causing an access beyond the end of the buffer.
I will send patches for both.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
