linux-kernel - Re: [GIT PULL] Please pull NFS changes for Linux 5.3

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

Message-ID: <CAHk-=wiiLJ3r2t02iqCtMxTufTCpKrPBmQ_L7ePZ4f-MwJ8o6A@mail.gmail.com>
Date:   Thu, 18 Jul 2019 14:47:56 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Trond Myklebust <trondmy@...merspace.com>
Cc:     "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Please pull NFS changes for Linux 5.3

On Thu, Jul 18, 2019 at 1:25 PM Trond Myklebust <trondmy@...merspace.com> wrote:
>
>   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git tags/nfs-for-5.3-1

This got a conflict with the debugfs "don't behave differently on
failures" changes in net/sunrpc/debugfs.c.

See commit 0a0762c6c604 ("sunrpc: no need to check return value of
debugfs_create functions") by Greg, but I suspect you were already
aware of this.

I did a hack-and-slash "remove the error handling", and the end result
looks sane. Except I left the "if the snprintf overflows" error
handling in place, even if nothing then cares about the returned
error.

I think my merge resolution makes sense, but I thought I'd mention it
in case you had something else in mind. Honestly, the snprintf()
checks in do_xprt_debugfs() look kind o fpointless, but the comment is
also wrong:

        char link[9]; /* enough for 8 hex digits + NULL */

that comment was copied from the "name[]" array in
rpc_clnt_debugfs_register(), but it's bogus, since you actually use

                len = snprintf(link, sizeof(link), "xprt%d", *nump);

on the thing.

And you know what? If you have so many links that "xprt%d" doesn't fit
in 8 chars plus NUL, maybe you don't really care?

But it's also worth noting that the whole snprintf() overflow check is
*wrong* to begin with. When you do

                if (len > sizeof(link))
                        return -1;

you're testing the wrong thing entirely. The returned "len" is the
length that would have been printed _without_ the ending NUL
character, so you actually had a truncation even if it returns
"sizeof(link)" - because then the NUL character was written instead of
the last character.

So the overflow test *should* have been

                if (len >= sizeof(link))
                        return -1;

but I suspect the correct thing to do is to just say "we don't care"
and remove that error check entirely.  Same goes for the other case
("len > sizeof(name)").

At some point error handling doesn't actually add value, as long as
the error itself isn't fatal. And when the error handling itself is
wrong, it's doubly suspect.

But as mentioned, I did *not* remove this part of the error handling.
I only removed the debugfs parts. The error handling may be wrong, but
it is what it is, and it doesn't really matter.

                  Linus