Question on "uaccess: Add strict non-pagefault kernel-space read function"

Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Christoph Hellwig]

Hi Daniel,

I just stumbled over your above commit, and it really confuses me.

Not the newly added functions, which seems perfectly sane, but why you
left the crazy old functions in place instead of investing a little
bit of extra effort to clean the existing mess up and switch everyone
to the sane new variants?

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Daniel Borkmann]

Hi Christoph,

On 4/3/20 3:35 PM, Christoph Hellwig wrote:
[...]
> I just stumbled over your above commit, and it really confuses me.
> 
> Not the newly added functions, which seems perfectly sane, but why you
> left the crazy old functions in place instead of investing a little
> bit of extra effort to clean the existing mess up and switch everyone
> to the sane new variants?

With crazy old functions I presume you mean the old bpf_probe_read()
which is mapped to BPF_FUNC_probe_read helper or something else entirely?

For the former, basically my main concern was that these would otherwise
break existing tools like bcc/bpftrace/.. unfortunately until they are not
converted over yet to _strict variants.

At least on x86, they would still rely on the broken semantic to probe
kernel and user memory with probe_read where it 'happens to work', but not
on other archs where the address space is not shared.

But once these are fixed, I would love to deprecate these in one way or
another. The warning in 00c42373d397 ("x86-64: add warning for non-canonical
user access address dereferences") should be a good incentive to switch
since people have been hitting it in production as the non-canonical space
is sometimes used in user space to tag pointers, for example.

Thanks,
Daniel

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Thomas Gleixner]

Christoph Hellwig <hch@infradead.org> writes:
> I just stumbled over your above commit, and it really confuses me.

Where is that commit?

Thanks,

        tglx

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Thomas Gleixner]

Thomas Gleixner <tglx@linutronix.de> writes:

> Christoph Hellwig <hch@infradead.org> writes:
>> I just stumbled over your above commit, and it really confuses me.
>
> Where is that commit?

Nevermind.

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Christoph Hellwig]

On Fri, Apr 03, 2020 at 04:20:24PM +0200, Daniel Borkmann wrote:
> With crazy old functions I presume you mean the old bpf_probe_read()
> which is mapped to BPF_FUNC_probe_read helper or something else entirely?

I couldn't care less about bpf, this is about the kernel API.

What I mean is that your new probe_kernel_read_strict and
strncpy_from_unsafe_strict helpers are good and useful.  But for this
to actually make sense we need to get rid of the non-strict versions,
and we also need to get rid of some of the weak alias magic.

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Daniel Borkmann]

On 4/4/20 11:31 AM, Christoph Hellwig wrote:
> On Fri, Apr 03, 2020 at 04:20:24PM +0200, Daniel Borkmann wrote:
>> With crazy old functions I presume you mean the old bpf_probe_read()
>> which is mapped to BPF_FUNC_probe_read helper or something else entirely?
> 
> I couldn't care less about bpf, this is about the kernel API.
> 
> What I mean is that your new probe_kernel_read_strict and
> strncpy_from_unsafe_strict helpers are good and useful.  But for this
> to actually make sense we need to get rid of the non-strict versions,
> and we also need to get rid of some of the weak alias magic.

Yeah agree, the probe_kernel_read() should do the strict checks by default
and there would need to be some way to opt-out for the legacy helpers to
not break. So it would end up looking like the below ...

long __probe_kernel_read(void *dst, const void *src, size_t size)
{
         long ret = -EFAULT;
         mm_segment_t old_fs = get_fs();

         set_fs(KERNEL_DS);
         if (kernel_range_ok(src, size))
                 ret = probe_read_common(dst, (__force const void __user *)src, size);
         set_fs(old_fs);

         return ret;
}

... where archs with non-overlapping user and kernel address range would
only end up having to implementing kernel_range_ok() check. Or, instead of
a generic kernel_range_ok() this could perhaps be more probing-specific as
in probe_kernel_range_ok() where this would then also cover the special
cases we seem to have in parisc and um. Then, this would allow to get rid
of all the __weak aliasing as well which may just be confusing. I could look
into coming up with something along these lines. Thoughts?

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Christoph Hellwig]

On Tue, Apr 07, 2020 at 11:03:23AM +0200, Daniel Borkmann wrote:
> 
> ... where archs with non-overlapping user and kernel address range would
> only end up having to implementing kernel_range_ok() check. Or, instead of
> a generic kernel_range_ok() this could perhaps be more probing-specific as
> in probe_kernel_range_ok() where this would then also cover the special
> cases we seem to have in parisc and um. Then, this would allow to get rid
> of all the __weak aliasing as well which may just be confusing. I could look
> into coming up with something along these lines. Thoughts?

FYI, this is what I cooked up a few days ago:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/maccess-fixups

Still misses the final work to switch probe_kernel_read to be the
strict version.  Any good naming suggestion for the non-strict one?

Re: Question on "uaccess: Add strict non-pagefault kernel-space read function"

[Daniel Borkmann]

On 4/7/20 11:33 AM, Christoph Hellwig wrote:
> On Tue, Apr 07, 2020 at 11:03:23AM +0200, Daniel Borkmann wrote:
>>
>> ... where archs with non-overlapping user and kernel address range would
>> only end up having to implementing kernel_range_ok() check. Or, instead of
>> a generic kernel_range_ok() this could perhaps be more probing-specific as
>> in probe_kernel_range_ok() where this would then also cover the special
>> cases we seem to have in parisc and um. Then, this would allow to get rid
>> of all the __weak aliasing as well which may just be confusing. I could look
>> into coming up with something along these lines. Thoughts?
> 
> FYI, this is what I cooked up a few days ago:
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/maccess-fixups
> 
> Still misses the final work to switch probe_kernel_read to be the
> strict version.  Any good naming suggestion for the non-strict one?

Ah great, thanks for working on it including the cleanups in your branch above.
Good naming suggestion is usually the hardest ... ;-) Maybe adding an _unsafe or
_lax suffix ...

Regarding commits:

* http://git.infradead.org/users/hch/misc.git/commitdiff/019f5d7894711a8046d1d57640d3db47f690c61e

I think the extra HAVE_PROBE_KERNEL_ALLOWED / HAVE_PROBE_KERNEL_STRICT_ALLOWED
reads a bit odd. Could we simply have an equivalent for access_ok() that archs
implement under KERNEL_DS where it covers the allowed/restricted kernel-only range?
Like mentioned earlier e.g. probe_{user,kernel}_range_ok() helpers where the user
one defaults to access_ok() internally and the kernel one contains all the range
restrictions that archs can then define if needed (and if not there could be an
asm-generic `return true` fallback, for example).

* http://git.infradead.org/users/hch/misc.git/commitdiff/2d6070ac749d0af26367892545d1c288cc00823a

This would still need to set dst[0] = '\0' in that case to be consistent with
the other error handling cases there when count > 0.

Thanks,
Daniel