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.1605560917.git.dxu@dxuuu.xyz> <470ffc3c76414443fc359b884080a5394dcccec3.1605560917.git.dxu@dxuuu.xyz>
In-Reply-To: <470ffc3c76414443fc359b884080a5394dcccec3.1605560917.git.dxu@dxuuu.xyz>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 16 Nov 2020 14:15:52 -0800
Message-ID: <CAHk-=wggUw3XYffJ-od8Dbfh-JkXkEuCPjSRR2Z+8HrNUNxJ=g@mail.gmail.com>
Subject: Re: [PATCH bpf v6 1/2] lib/strncpy_from_user.c: Don't overcopy bytes
 after NUL terminator
To:     Daniel Xu <dxu@dxuuu.xyz>
Cc:     bpf <bpf@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Song Liu <songliubraving@fb.com>, andrii.nakryiko@gmail.com,
        kernel-team@fb.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 16, 2020 at 1:17 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Based on on-list discussion and some off-list discussion with Alexei,
> I'd like to propose the v4-style patch without the `(*out & ~mask)`
> bit.

So I've verified that at least on x86-64, this doesn't really make
code generation any worse, and I'm ok with the patch from that
standpoint.

However, this was not what the discussion actually amended at as far
as I'm concerned.

I mentioned that if BPF cares about the bytes past the end of the
string, I want a *BIG COMMENT* about it. Yes, in strncpy_from_user()
itself, but even more in the place that cares.

And no, that does not mean bpf_probe_read_user_str().  That function
clearly doesn't care at all, and doesn't access anything past the end
of the string. I want a comment in whatever code that accesses past
the end of the string.

And your ABI point is actively misleading:

> We can't really zero out the rest of the buffer due to ABI issues.
> The bpf docs state for bpf_probe_read_user_str():
>
> > In case the string length is smaller than *size*, the target is not
> > padded with further NUL bytes.

This comment is actively wrong and misleading.

The code (after the patch) clearly *does* pad a bit with "further NUL
bytes". It's just that it doesn't pad all the way to the end.

Where is the actual buffer zeroing done?

Because without the buffer zeroing, this whole patch is completely
pointless. Which is why I want that comment, and I want a pointer to
where that zeroing is done.

Really. You have two cases:

 (a) the buffer isn't zeroed before the strncpy_from_user()

 (b) the buffer is guaranteed to be zero before that

and in case (a), this patch is pointless, since the data after the
string is already undefined.

And in case (b), I want to see a comment and a pointer to the code
that actually does the zeroing.

HOWEVER. Look at bpf_probe_read_user_str_common(), and notice how it
ALREADY does the zeroing of the buffer past the end, it's just that it
only does it in the error case.

Why do you send this patch, instead of

 (a) get rid of the pointless pre-zeroing

 (b) change bpf_probe_read_user_str_common() to do

        int ret;
        u32 offset;

        ret = strncpy_from_user_nofault(dst, unsafe_ptr, size);
        offset = ret < 0 ? 0 : ret;
        memset(dst+offset, 0, size-offset);
        return ret;

which seems to be much simpler anyway. The comment you quote about
"target is not padded with further NUL bytes" is clearly wrong anyway,
since that error case *does* pad the target with NUL bytes, and always
has.

So honestly, in this whole discussion, it seems rather clear to me
that the bug has always been in bpf, not in strncpy_from_user(). The
ABI comment you quote is clearly not true, and I can point to that
existing bpf_probe_read_user_str_common() code itself:

        ret = strncpy_from_user_nofault(dst, unsafe_ptr, size);
        if (unlikely(ret < 0))
                memset(dst, 0, size);

as to why that is.

But guys, as mentioned, I'm willing to apply this patch, but only if
you add some actually *correct* comments about the odd bpf use of this
string, and point to where the pre-zeroing is done.

               Linus