Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1605134506.git.dxu@dxuuu.xyz> <f5eed57b42cc077d24807fc6f2f7b961d65691e5.1605134506.git.dxu@dxuuu.xyz>
 <20201113170338.3uxdgb4yl55dgto5@ast-mbp> <CAHk-=wjNv9z6-VOFhpYbXb_7ePvsfQnjsH5ipUJJ6_KPe1PWVA@mail.gmail.com>
 <20201113191751.rwgv2gyw5dblhe3j@ast-mbp>
In-Reply-To: <20201113191751.rwgv2gyw5dblhe3j@ast-mbp>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 13 Nov 2020 11:46:36 -0800
Message-ID: <CAHk-=wj4EjAq=bN1aKUyfVR4xTM9K1QO-99+N0132V1DHs1XBQ@mail.gmail.com>
Subject: Re: [PATCH bpf v5 1/2] lib/strncpy_from_user.c: Don't overcopy bytes
 after NUL terminator
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Daniel Xu <dxu@dxuuu.xyz>, bpf <bpf@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>, kernel-team@fb.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Nov 13, 2020 at 11:17 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> The destination buffer was already initialized with zeros before the call.

Side note: this is another example of you using the interface incorrectly.

You should *not* initialize the buffer with zeroes. That's just extra
work. One of the points of the strncpy_from_user() interface is that
it is *not* the incredibly broken garbage that "strncpy()" is.

strncpy_from_user() returns the size of the resulting string,
*EXACTLY* so that people who care can then use that information
directly and efficiently.

Most of the time it's to avoid a strlen() on the result (and check for
overflow), of course, but the other use-case is exactly that "maybe I
need to pad out the result", so that you don't need to initialize the
buffer beforehand.

I'm not sure exactly which strncpy_from_user() user you have issues
with, but I did a

     git grep strncpy_from_user -- '*bpf*'

and several of them look quite questionable.

All of the ones in kernel/bpf/syscall.c seem to handle overflow
incorrectly, for example, with a silent truncation instead of error.
Maybe that's fine,  but it's questionable.

And the bpf_trace_copy_string() thing doesn't check the result at all,
so the end result may not be NUL-terminated. Maybe that's ok. I didn't
check the call chain.

              Linus