On Feb 15, 2019, at 4:19 PM, Steven Rostedt rostedt@goodmis.org wrote:

On Fri, 15 Feb 2019 15:49:35 -0800 Andy Lutomirski luto@amacapital.net wrote:

...

I’m missing most of the context here, but even probe_kernel_...() is unwise for a totally untrustworthy address. It could be MMIO, for example.

True, but kprobes are used like modules, and only allowed by root. They are used to poke literally anywhere one wants. That's the entire purpose of kprobes.

...

If needed, we could come up with a safe-ish helper for tracing. For direct-map addresses, probe_kernel_...() is probably okay. Same for the current stack. Otherwise we could walk the page tables and check that the address is cacheable, I suppose, although this is slightly dubious if we don’t also check MTRRs. We could also check that the PA is in main memory, I suppose, although this may have unfortunate interactions with the MCE code.

I added you just because I wanted help getting the change log correct, as that's what Linus was complaining about. I kept using "kernel address" when the sample bug used for the patch was really a non-canonical address (as Linus said, it's just garbage. Neither kernel or user space). But I pointed out that this can also bug if the address is canonical and in the kernel address space. The old code didn't complain about non-canonical or kernel address faulting before commit 9da3f2b7405, which only talks about kernel address space faulting (which is why I only mentioned that in my messages).

Would changing all the mention of "kernel address" to "non user space" be accurate?

I think “kernel address” is right. It’s illegal to access anything that isn’t known to be a valid kernel address while in KERNEL_DS.

The old __copy seems likely to have always been a bit bogus.

BTW, what is this probe_mem_read() thing? Some minimal inspection suggests it’s a buggy reimplementation of probe_kernel_read(). Can you delete it and just use probe_kernel_read() directly?

For reference:

http://lkml.kernel.org/r/20190215174945.557218316@goodmis.org http://lkml.kernel.org/r/20190215142015.860423791@goodmis.org

-- Steve

On Feb 15, 2019, at 6:08 PM, Steven Rostedt rostedt@goodmis.org wrote:

On Fri, 15 Feb 2019 17:32:55 -0800 Andy Lutomirski luto@amacapital.net wrote:

...

...

I added you just because I wanted help getting the change log correct, as that's what Linus was complaining about. I kept using "kernel address" when the sample bug used for the patch was really a non-canonical address (as Linus said, it's just garbage. Neither kernel or user space). But I pointed out that this can also bug if the address is canonical and in the kernel address space. The old code didn't complain about non-canonical or kernel address faulting before commit 9da3f2b7405, which only talks about kernel address space faulting (which is why I only mentioned that in my messages).

Would changing all the mention of "kernel address" to "non user space" be accurate?

I think “kernel address” is right. It’s illegal to access anything that isn’t known to be a valid kernel address while in KERNEL_DS.

But an non-canonical address is not a "kernel address", and that will cause a bug too.

Indeed. A non-canonical address is not known to be a valid kernel address. I stand by my slightly pedantic statement :)

This patch came about because it was changed that if we do a uaccess on something other than a user space address and take a fault (either because it was a non-canonical address, or a kernel address), we BUG! Where before that one patch, it would just return a fault.

...

The old __copy seems likely to have always been a bit bogus.

BTW, what is this probe_mem_read() thing? Some minimal inspection suggests it’s a buggy reimplementation of probe_kernel_read(). Can you delete it and just use probe_kernel_read() directly?

Well, the issue is that we have trace_probe_tmpl.h in that same directory, which does the work for kprobes and uprobes. The trace_kprobes.c defines all the functions for handling kprobes, and trace_uprobes.c does all the handling of uprobes, then they include trace_probe_tmpl.h which does the bulk of the work.

In the uprobes case, we have:

static nokprobe_inline int probe_mem_read(void *dest, void *src, size_t size) { void __user *vaddr = (void __force __user *)src;

return copy_from_user(dest, vaddr, size) ? -EFAULT : 0; }

Because that is adding probes on userspace code.

Can the kprobe case call probe_kernel_read? Maybe it does already?

[ Sorry for not looking at this earlier, I have family in town so I was mostly busy over the weekend... ]

On Fri, Feb 15, 2019 at 4:19 PM Steven Rostedt rostedt@goodmis.org wrote:

Would changing all the mention of "kernel address" to "non user space" be accurate?

That would have worked, but in the meantime I just decided to pull the existing tag, because it's not _horribly_ misleading any more and now we're just talking details of what is a "kernel address" etc. And the patch itself is better than what we used to have.

That said, I do agree with Andy that both the old _copy_from_user_inatomic() thing and the new probe_mem_read() are just fundamentally broken, nasty and slow. It just so happens that probe_mem_read() works _reasonably_ well in practice on x86.

Basically, probe_mem_read() -> probe_kernel_read() really only works on true kernel addresses. And some of that is very fundamental: some architectures can have two entirely different address spaces for user and kernel memory, so if you give _any_ function an "try to read this address", it fundamentally has to be one or the other.

The fact that on x86, there is a unified address space for kernel/user, and it can work for one or the other, is just happenstance (and admittedly the common case).

So I've pulled the existing pull request, because it papers over one particular issue, but the real fix would require:

- knowing whether it's kernel or user space you access

- actually using that knowledge to then limit the addresses we are willing to probe, and _how_ we probe them.

The user-space case is fairly easy: just check the address with "access_ok()", and then use _copy_user_atomic() without any set_fs() games. That should "JustWork(tm)".

And if it's truly supposed to be a kernel address, then we probably need to add a "is this possibly a valid kernel data pointer" interface. Before we then do what "probe_kernel_read()" currently does.

Linus

[ Added Masami too. Start of thread is here: http://lkml.kernel.org/r/20190215174712.372898450@goodmis.org ]

On Mon, 18 Feb 2019 10:23:44 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:

So it would be good to not just say "user or kernel", but actually say what *kind* of kernel access it expects.

Note, kprobes are a different kind of beast. I've used kprobes to probe userspace information as well as kernel. Heck, I could see someone even using kprobes to probe IO memory to check if a device is doing what they expect it's doing.

Basically, a kprobe is mostly used for debugging what's happening in a live kernel, to read any address. But for those that are more security minded, perhaps we could add "layers" via CONFIG or /sys files that prevent kprobes from doing certain kinds of probes?

-- Steve

On Tue, 19 Feb 2019 10:43:36 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:

On Tue, Feb 19, 2019 at 8:18 AM Steven Rostedt rostedt@goodmis.org wrote:

...

...

So it would be good to not just say "user or kernel", but actually say what *kind* of kernel access it expects.

Note, kprobes are a different kind of beast. I've used kprobes to probe userspace information as well as kernel. Heck, I could see someone even using kprobes to probe IO memory to check if a device is doing what they expect it's doing.

Note that even if that is the case, you _need_ to special "user vs kernel" information.

Because the exact same address might exist in both.

Right now I think that only happens on sparc32, but vendors used to have that issue on x86-32 too (if they had the 4G:4G patches).

Hmm, I didn't realize that Linux allowed the same address to be different depending on being in kernel or user space (learn something new everyday). I guess it makes sense, and even easier with the switch of cr3 from user to kernel. And I even knew of 4G:4G, but never used it, nor put too much thought about its implementation.

...

Basically, a kprobe is mostly used for debugging what's happening in a live kernel, to read any address.

My point is that "any address" is not sufficient to begin with. You need "kernel or user".

Having a flag for what _kind_ of kernel address is ok might then be required for other cases if they might not be ok with following page tables to IO space..

Good point. Looks like we should add a new flag for kprobe trace parameters, that tell kprobes if the address is expected to be user or kernel. That would be good regardless of the duplicate meanings, as we could use copy_from_user without touching KERNEL_DS, if the probe argument specifically states "this is user space". For example, when probing do_sys_open, and you want to read what path string was passed into the kernel.

Masami, thoughts?

-- Steve

On Wed, Feb 20, 2019 at 9:10 AM Masami Hiramatsu mhiramat@kernel.org wrote:

On Tue, 19 Feb 2019 14:03:30 -0500 Steven Rostedt rostedt@goodmis.org wrote:

...

...

...

Basically, a kprobe is mostly used for debugging what's happening in a live kernel, to read any address.

My point is that "any address" is not sufficient to begin with. You need "kernel or user".

Having a flag for what _kind_ of kernel address is ok might then be required for other cases if they might not be ok with following page tables to IO space..

Good point. Looks like we should add a new flag for kprobe trace parameters, that tell kprobes if the address is expected to be user or kernel. That would be good regardless of the duplicate meanings, as we could use copy_from_user without touching KERNEL_DS, if the probe argument specifically states "this is user space". For example, when probing do_sys_open, and you want to read what path string was passed into the kernel.

Masami, thoughts?

Let me ensure what you want. So you want to access a "string" in user-space, not a data structure? In that case, it is very easy to me. It is enough to add a "ustring" type to kprobe events. For example, do_sys_opsn's path variable is one example. That will be +0(+0(%si)):ustring, and fetcher finally copy the string using strncpy_from_user() instead of strncpy_from_unsafe(). (*)

[...]

(*) BTW, there is another concern to use _from_user APIs in kprobe. Are those APIs might sleep??

If you want to access userspace without sleeping, and ignore data in non-present pages, you can do `pagefault_disable(); err = __copy_from_user_inatomic(...); pagefault_enable();`. (Actually, maybe the kernel should have a helper for that...)

Hi Jann,

On Wed, 20 Feb 2019 14:57:31 +0100 Jann Horn jannh@google.com wrote:

On Wed, Feb 20, 2019 at 9:10 AM Masami Hiramatsu mhiramat@kernel.org wrote:

...

On Tue, 19 Feb 2019 14:03:30 -0500 Steven Rostedt rostedt@goodmis.org wrote:

...

...

...

Basically, a kprobe is mostly used for debugging what's happening in a live kernel, to read any address.

My point is that "any address" is not sufficient to begin with. You need "kernel or user".

Having a flag for what _kind_ of kernel address is ok might then be required for other cases if they might not be ok with following page tables to IO space..

Good point. Looks like we should add a new flag for kprobe trace parameters, that tell kprobes if the address is expected to be user or kernel. That would be good regardless of the duplicate meanings, as we could use copy_from_user without touching KERNEL_DS, if the probe argument specifically states "this is user space". For example, when probing do_sys_open, and you want to read what path string was passed into the kernel.

Masami, thoughts?

Let me ensure what you want. So you want to access a "string" in user-space, not a data structure? In that case, it is very easy to me. It is enough to add a "ustring" type to kprobe events. For example, do_sys_opsn's path variable is one example. That will be +0(+0(%si)):ustring, and fetcher finally copy the string using strncpy_from_user() instead of strncpy_from_unsafe(). (*)

[...]

...

(*) BTW, there is another concern to use _from_user APIs in kprobe. Are those APIs might sleep??

If you want to access userspace without sleeping, and ignore data in non-present pages, you can do `pagefault_disable(); err = __copy_from_user_inatomic(...); pagefault_enable();`. (Actually, maybe the kernel should have a helper for that...)

Ok, we are going back to the start point of this thread :)

http://lkml.kernel.org/r/20190215174712.372898450@goodmis.org

So, if user tells kprobe it is user-pointer, we check it with access_ok(), and will do something similar to the strnlen_user() and strncpy_from_user(), but using __copy_from_user_inatomic() and pagefault_disable() for kprobes.

Thank you!

On Wed, 20 Feb 2019 09:49:26 -0500 Steven Rostedt rostedt@goodmis.org wrote:

On Wed, 20 Feb 2019 17:10:19 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

Let me ensure what you want. So you want to access a "string" in user-space, not a data structure? In that case, it is very easy to me. It is enough to add a "ustring" type to kprobe events. For example, do_sys_opsn's path variable is one example. That will be +0(+0(%si)):ustring, and fetcher finally copy the string using strncpy_from_user() instead of strncpy_from_unsafe(). (*)

ustring would be good.

OK.

...

But if you consider to access a field in a data-structure in user space, it might need some more work (E.g. ioctl's parameter), becase if the __user pointer to the data structure is on the memory, we have to dereference the address inside kernel using probe_kernel_read(), but after getting the data strucutre address, we have to dereference the address with copy_from_user(). At this moment, we have no such strong syntax...

To solve that, maybe we need to introduce something like "back reference" of arguments in the event, e.g.

p somewhere user_data=+0(%si) field_val=+8(\user_data):u32:user

or

p somewhere +0(%si) field_val=+8(\1):u32:user

This ":user" additional suffix tells kprobe events to change fetching method to fetch the data by copy_from_user().

What about just adding 'u' to the end of the offset? Say you have a data structure in kernel space that has a field in user space you want to reference?

field_val=+8u(+0(%si))

Ah, that looks good :~) thank you for this idea!

Although, I would say having ways to access current parameters may also be a nice touch ;-)

Hehe, OK, let's implement both.

Thanks!

-- Steve

On Wed, 20 Feb 2019 11:42:17 -0500 Steven Rostedt rostedt@goodmis.org wrote:

On Thu, 21 Feb 2019 01:04:53 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

...

What about just adding 'u' to the end of the offset? Say you have a data structure in kernel space that has a field in user space you want to reference?

field_val=+8u(+0(%si))

Ah, that looks good :~) thank you for this idea!

<bike-shed>

Hmm, I wonder if we should make it +u8 or u+8? as +8u may be confused as unsigned? Like 8ULL. I don't know. Kernel developers suck at naming :-p

I like +u8 since it is easier to implement :-p.

Thank you,

</bike-shed>

-- Steve

On Fri, 22 Feb 2019 17:27:45 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

Hi Steve,

On Wed, 20 Feb 2019 09:49:26 -0500 Steven Rostedt rostedt@goodmis.org wrote:

...

On Wed, 20 Feb 2019 17:10:19 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

Let me ensure what you want. So you want to access a "string" in user-space, not a data structure? In that case, it is very easy to me. It is enough to add a "ustring" type to kprobe events. For example, do_sys_opsn's path variable is one example. That will be +0(+0(%si)):ustring, and fetcher finally copy the string using strncpy_from_user() instead of strncpy_from_unsafe(). (*)

ustring would be good.

I've tried to implement ustring and u-offsets, but I got some issues.

• access_ok() warns if it is called in IRQ context (kprobes is.)
• copy_from_user uses access_ok(), so it is not designed for irq handler.

Moreover, if we have different kernel/user address spaces, we have to assign target user-pages to kernel vma. Can we do that (doesn't it involve mutex locks)?

Or, can we do this?

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

set_fs(USER_DS); /* Only this is changed */ pagefault_disable(); current->kernel_uaccess_faults_ok++; ret = __copy_from_user_inatomic(dst, (__force const void __user *)src, size); current->kernel_uaccess_faults_ok--; pagefault_enable(); set_fs(old_fs);

return ret ? -EFAULT : 0; }

On Feb 22, 2019, at 9:43 AM, Linus Torvalds torvalds@linux-foundation.org wrote:

...

On Fri, Feb 22, 2019 at 12:35 AM Masami Hiramatsu mhiramat@kernel.org wrote:

Or, can we do this?

long __probe_user_read(void *dst, const void *src, size_t size) {

Add a

   if (!access_ok(src, size))
           ret = -EFAULT;
   else {

.. do the pagefault_disable() etc .. }

to after the "set_fs()", and it looks good to me. Make it clear that yes, this works _only_ for user reads.

Adn that makes all the games with "kernel_uaccess_faults_ok" pointless, so you can just remove them.

(note that the "access_ok()" has to come after we've done "set_fs()", because it takes the address limit from that).

Also, since normally we'd expect that we already have USER_DS, it might be worthwhile to do this with a wrapper, something along the lines of

   mm_segment_t old_fs = get_fs();

   if (segment_eq(old_fs, USER_DS))
         return __normal_probe_user_read();
   set_fs(USER_DS);
   ret = __normal_probe_user_read();
   set_fs(old_fs);
   return ret;

and have that __normal_probe_user_read() just do

   if (!access_ok(src, size))
           return -EFAULT;
   pagefault_disable();
   ret = __copy_from_user_inatomic(dst, ...);
   pagefault_enable();
   return ret ? -EFAULT : 0;

which looks more obvious.

Also, I would suggest that you just make the argument type be "const void __user *", since the whole point is that this takes a user pointer, and nothing else.

Then we should still probably fix up "__probe_kernel_read()" to not allow user accesses. The easiest way to do that is actually likely to use the "unsafe_get_user()" functions *without* doing a uaccess_begin(), which will mean that modern CPU's will simply fault on a kernel access to user space.

The nice thing about that is that usually developers will have access to exactly those modern boxes, so the people who notice that it doesn't work are the right people.

We use probe_kernel_read() from oops code. I’d rather it return -EFAULT than oops harder and kill the first oops.

On Feb 22, 2019, at 10:28 AM, Linus Torvalds torvalds@linux-foundation.org wrote:

...

On Fri, Feb 22, 2019 at 9:48 AM Andy Lutomirski luto@amacapital.net wrote:

...

On Feb 22, 2019, at 9:43 AM, Linus Torvalds torvalds@linux-foundation.org wrote:

Then we should still probably fix up "__probe_kernel_read()" to not allow user accesses. The easiest way to do that is actually likely to use the "unsafe_get_user()" functions *without* doing a uaccess_begin(), which will mean that modern CPU's will simply fault on a kernel access to user space.

The nice thing about that is that usually developers will have access to exactly those modern boxes, so the people who notice that it doesn't work are the right people.

We use probe_kernel_read() from oops code. I’d rather it return -EFAULT than oops harder and kill the first oops.

It would still do that.

Using the unsafe_get_user() macros doesn't remove the exception handling, and we wouldn't remove the whole "pagefault_disable()" either. So it would work exactly the same way it does now, except on a modern CPU it would return -EFAULT for a user space access due to AC not being set.

Hmm. I misunderstood you. I thought you wanted the oops.

We’d have to check that we don’t trip the “SMAP violation, egads!” check.

On Fri, 22 Feb 2019 11:27:05 -0800 Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:

...

Then we should still probably fix up "__probe_kernel_read()" to not allow user accesses. The easiest way to do that is actually likely to use the "unsafe_get_user()" functions *without* doing a uaccess_begin(), which will mean that modern CPU's will simply fault on a kernel access to user space.

On bpf side the bpf_probe_read() helper just calls probe_kernel_read() and users pass both user and kernel addresses into it and expect that the helper will actually try to read from that address.

If __probe_kernel_read will suddenly start failing on all user addresses it will break the expectations. How do we solve it in bpf_probe_read? Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte in the loop? That's doable, but people already complain that bpf_probe_read() is slow and shows up in their perf report.

We're changing kprobes to add a specific flag to say that we want to differentiate between kernel or user reads. Can this be done with bpf_probe_read()? If it's showing up in perf report, I doubt a single check is going to cause an issue. In fact, it may actually help speed things up as the read will be optimized for either user or kernel address reading.

-- Steve

On Fri, 22 Feb 2019 11:34:58 -0800 Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

so you're saying you will break existing kprobe scripts?

Yes we may.

I don't think it's a good idea. It's not acceptable to break bpf_probe_read uapi.

Then you may need to add more code to determine if the address is user space or not in the kernel, and then go the appropriate route, before calling probe_kernel_read().

-- Steve

On Fri, Feb 22, 2019 at 11:27 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

On bpf side the bpf_probe_read() helper just calls probe_kernel_read() and users pass both user and kernel addresses into it and expect that the helper will actually try to read from that address.

As mentioned earlier in the thread, that's actually fundamentally broken.

There are architectures that have physically separate address spaces, with the same pointer value in both kernel and user space.

They are rare, but they exist. At least sparc32 and the old 4G:4G split x86.

So a pointer really should always unambiguously always be explicitly _either_ a kernel pointer, or a user pointer. You can't have "this is a pointer", and then try to figure it out by looking at the value. That may happen to work on x86-64, but it's literally a "happen to work on the most common architectures", not a design thing.

Linus

On Fri, Feb 22, 2019 at 1:38 PM David Miller davem@davemloft.net wrote:

Don't be surprised if we see more separation like this in the future too.

Yes, with the whole meltdown fiasco, there's actually more pressure to add more support for separation of kernel/user address spaces. As Andy pointed out, it's been discussed as a future wish-list for x86-64 too.

But yeah, right now the *common* architectures all distinguish kernel and user space by pointers (ie x86-64, arm64 and powerpc).

Linus

On Fri, Feb 22, 2019 at 11:51 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

On Fri, Feb 22, 2019 at 01:59:10PM -0800, Linus Torvalds wrote:

...

On Fri, Feb 22, 2019 at 1:38 PM David Miller davem@davemloft.net wrote:

...

Don't be surprised if we see more separation like this in the future too.

Yes, with the whole meltdown fiasco, there's actually more pressure to add more support for separation of kernel/user address spaces. As Andy pointed out, it's been discussed as a future wish-list for x86-64 too.

But yeah, right now the *common* architectures all distinguish kernel and user space by pointers (ie x86-64, arm64 and powerpc).

That's all fine. I'm missing rationale for making probe_kernel_read() fail on user addresses. What is fundamentally wrong with a function probe_any_address_read() ?

I think what Linus is saying is: There are some scenarios (like a system with the old 4G/4G X86 patch) where *the same* address can refer to two different pieces of memory, depending on whether you interpret it as a kernel pointer or a user pointer. So for example, if your BPF program tries to read tsk->comm, it works, but if the BPF program then tries to read from PT_REGS_PARM2(ctx), it's going to actually interpret the userspace address as a kernel address and read completely different memory.

On top of that, from the security angle, this means that if a user passes a kernel pointer into a syscall, they can trick a tracing BPF program into looking at random kernel memory instead of the user's memory. That may or may not be problematic, depending on what you do afterwards with the data you've read. (For example, if this is a service that collects performance data and then saves it to some world-readable location on disk because the data it is collecting (including comm strings) isn't supposed to be sensitive, you might have a problem.)

For context, typical bpf kprobe program looks like this: #define probe_read(P) \ ({typeof(P) val = 0; bpf_probe_read(&val, sizeof(val), &P); val;}) SEC("kprobe/__set_task_comm") int bpf_prog(struct pt_regs *ctx) { struct signal_struct *signal; struct task_struct *tsk; char oldcomm[16] = {}; char newcomm[16] = {}; u16 oom_score_adj; u32 pid;

tsk = (void *)PT_REGS_PARM1(ctx);
pid = probe_read(tsk->pid);
bpf_probe_read(oldcomm, sizeof(oldcomm), &tsk->comm);
bpf_probe_read(newcomm, sizeof(newcomm), (void *)PT_REGS_PARM2(ctx));
signal = probe_read(tsk->signal);
oom_score_adj = probe_read(signal->oom_score_adj);
...

}

where PT_REGS_PARMx macros are defined per architecture. On x86 it's #define PT_REGS_PARM1(x) ((x)->di)

The program writer has to know the meaning of function arguments. In this example they need to know that __set_task_comm is defined as void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) in the kernel.

Right now these programs just call bpf_probe_read() on whatever data they need to access and not differentiating whether it's user or kernel.

One idea we discussed is to split bpf_probe_read() into kernel_read and user_read helpers, but in the BPF verifier we cannot determine which address space the program wants to access. The prog writer needs to manually analyze the program to use correct one. But mistakes are possible and cannot be fatal. On the kernel side we have to be safe. Both probe_kernel_read and probe_user_read must not panic if a pointer from wrong address space was passed.

Hence my preference is to keep probe_kernel_read as "try read any address". The function can be renamed to indicate so.

On Fri, Feb 22, 2019 at 2:51 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

That's all fine. I'm missing rationale for making probe_kernel_read() fail on user addresses.

Because it already WON'T WORK in general!

What is fundamentally wrong with a function probe_any_address_read() ?

What part of "the same pointer value can be a user address and a kernel address" is not getting through?

The user address space and the kernel address space have separate page tables on some architectures. We used to avoid it on x86, because switching address spaces was expensive, but even on x86 some vendors did it on 32-bit simply to get 4GB of user (and kernel) address space. And now we end up doing it anyway just because of meltdown.

So a kernel pointer value of 0x12345678 could be a value kernel pointer pointing to some random kmalloc'ed kernel memory, and a user pointer value of 0x12345678 could be a valid _user_ pointer pointing to some user mapping.

See?

If you access a user pointer, you need to use a user accessor function (eg "get_user()"), while if you access a kernel pointer you need to just dereference it directly (unless you can't trust it, in which case you need to use a _different_ accessor function).

The fact that user and kernel pointers happen to be distinct on x86-64 (right now) is just a random implementation detail.

Really.

I didn't realize how many people seem to have been confused about this. But it's always been true. It's just that the common architectures have had that "one single address space for both kernel and user pointers" in practice.

In fact, the *very* first kernel version had separate address spaces for kernel and user mode even on x86 (using segments, not paging). So it has literally been true since day one in Linux that a kernel address can be indistinguishable from a user address from a pure value standpoint.

Linus

On Fri, Feb 22, 2019 at 3:56 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

It will preserve existing bpf_probe_read() behavior on x86.

... but that's the worst possible situation.

It appears that people haven't understood that kernel and user addresses are distinct, and may have written programs that are fundamentally buggy.

And we _want_ to make it clear that they are buggy on x86-64, exactly because x86-64 is the one that gets the most testing - by far.

So if x86-64 continues working for buggy programs, then that only means that those bugs never get fixed.

It would be much better to try to get those things fixed, and make the x86-64 implementation stricter, exactly so that people end up _realizing_ that they can't just think "a pointer is a pointer, and the context doesn't matter".

From a pure functional safety standpoint, I thought bpf already knew

what kind of a pointer it had?

Linus

On Fri, Feb 22, 2019 at 6:29 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

imo this kernel release should finish as-is and in the next cycle we can change probe_kernel_read() to reject user address [..]

Absolutely. Nothing is going to change right now for 5.0, which is imminent.

It's really a "long-term we really need to fix this", where the only question is how soon "long-term" is.

Linus

On Fri, 22 Feb 2019 15:56:20 -0800 Alexei Starovoitov alexei.starovoitov@gmail.com wrote:

On Fri, Feb 22, 2019 at 03:16:35PM -0800, Linus Torvalds wrote:

...

So a kernel pointer value of 0x12345678 could be a value kernel pointer pointing to some random kmalloc'ed kernel memory, and a user pointer value of 0x12345678 could be a valid _user_ pointer pointing to some user mapping.

See?

If you access a user pointer, you need to use a user accessor function (eg "get_user()"), while if you access a kernel pointer you need to just dereference it directly (unless you can't trust it, in which case you need to use a _different_ accessor function).

that was clear already. Reading 0x12345678 via probe_kernel_read can return valid value and via get_user() can return another valid value on _some_ architectures.

...

The fact that user and kernel pointers happen to be distinct on x86-64 (right now) is just a random implementation detail.

yes and my point that people already rely on this implementation detail. Say we implement int bpf_probe_read(void *val, void *unsafe_ptr) { if (probe_kernel_read(val, unsafe_ptr) == OK) { return 0; } else (get_user(val, unsafe_ptr) == OK) { return 0; } else { *val = 0; return -EFAULT; } }

Note that we can not use get_user() form kprobe handler. If you use it, you have to prepare fault_handler() and make bpf itself can be aborted. So, maybe you can use probe_user_read().

Hmm, however, it still doesn't work correctly on "some" architecture, since whether a pointer (address) points user-space or kernel-space depends on the context. In kprobe/bpf, the context means where you put the probe and which pointer you record.

I think only "__user" tag tells us which one is user-space. But unfortunately, that "__user" tag is only for compiler or checker, not for runtime binary. Such useful attribute goes away when we execute it.

So, even if we introduce "ustring", ftrace/perf users has to decide to use it by themselves. As far as I know, DWARF(debuginfo) also doesn't have that attribute. So perf-probe can not help it from debuginfo. (Maybe if we introduce C parser, it might be detected...)

It will preserve existing bpf_probe_read() behavior on x86. If x86 implementation changes tomorrow then progs that read user addresses may start failing randomly because first probe_kernel_read() will be returning random values from kernel memory and that's no good, but at least we won't be breaking them today, so we have time to introduce bpf_user_read and bpf_kernel_read and folks have time to adopt them.

I see. I think bpf also has to introduce new bpf_probe_read_user() and keep bpf_probe_read() for kernel dataa only.

Imo that's much better than making current bpf_probe_read() fail on user addresses today and not providing a non disruptive path forward.

Agreed.

Thank you,

On Fri, Feb 22, 2019 at 11:27:05AM -0800, Alexei Starovoitov wrote:

On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:

...

Then we should still probably fix up "__probe_kernel_read()" to not allow user accesses. The easiest way to do that is actually likely to use the "unsafe_get_user()" functions *without* doing a uaccess_begin(), which will mean that modern CPU's will simply fault on a kernel access to user space.

On bpf side the bpf_probe_read() helper just calls probe_kernel_read() and users pass both user and kernel addresses into it and expect that the helper will actually try to read from that address.

Slightly related and FWIW, BCC's eBPF-based opensnoop tool [1] installs a kprobe on do_sys_open to monitor calls to the open syscall globally.

do_sys_open() has prototype:

long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode);

This causes a "blank" filename to be displayed by opensnoop when I run it on my Pixel 3 (arm64), possibly because this is a user pointer. However, it works fine on x86-64.

So it seems to me that on arm64, reading user pointers directly still doesn't work even if there is a distinction between user/kernel addresses. In that case reading the user pointer using user accessors (possibly using bpf_probe_user_read helper) should be needed to fix this issue (as Yonghong also privately discussed with me).

[1] https://github.com/iovisor/bcc/blob/master/tools/opensnoop.py#L140

thanks!

- Joel

If __probe_kernel_read will suddenly start failing on all user addresses it will break the expectations. How do we solve it in bpf_probe_read? Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte in the loop? That's doable, but people already complain that bpf_probe_read() is slow and shows up in their perf report.

On Thu, Feb 28, 2019 at 09:29:13PM +0900, Masami Hiramatsu wrote:

On Tue, 26 Feb 2019 10:24:47 -0500 Joel Fernandes joel@joelfernandes.org wrote:

...

On Fri, Feb 22, 2019 at 11:27:05AM -0800, Alexei Starovoitov wrote:

...

On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:

...

Then we should still probably fix up "__probe_kernel_read()" to not allow user accesses. The easiest way to do that is actually likely to use the "unsafe_get_user()" functions *without* doing a uaccess_begin(), which will mean that modern CPU's will simply fault on a kernel access to user space.

On bpf side the bpf_probe_read() helper just calls probe_kernel_read() and users pass both user and kernel addresses into it and expect that the helper will actually try to read from that address.

Slightly related and FWIW, BCC's eBPF-based opensnoop tool [1] installs a kprobe on do_sys_open to monitor calls to the open syscall globally.

do_sys_open() has prototype:

long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode);

This causes a "blank" filename to be displayed by opensnoop when I run it on my Pixel 3 (arm64), possibly because this is a user pointer. However, it works fine on x86-64.

So it seems to me that on arm64, reading user pointers directly still doesn't work even if there is a distinction between user/kernel addresses. In that case reading the user pointer using user accessors (possibly using bpf_probe_user_read helper) should be needed to fix this issue (as Yonghong also privately discussed with me).

OK, it sounds like the same issue. Please add a bpf_user_read() and use it for __user pointer.

CC'd Yonghong who said eariler to me he would add it, but I could add it too if he wants me to.

thanks,

- Joel

On Sat, 23 Feb 2019 12:47:46 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

Since kprobes handler runs in IRQ context, we can not use access_ok() in it. (only on x86 + CONFIG_DEBUG_ATOMIC_SLEEP=y)

Is it really IRQ context or exception context? That is, one (interrupts) happen for any task, but exceptions happen because of the software that is executed (like a breakpoint). Although you can have a kprobe trigger in an interrupt handler (where user access wouldn't make sense anyway). But there should be no problem with user access from an exception handler.

-- Steve

On Sat, 23 Feb 2019 20:38:03 -0800 Andy Lutomirski luto@kernel.org wrote:

On Sat, Feb 23, 2019 at 4:44 PM Steven Rostedt rostedt@goodmis.org wrote:

...

On Sat, 23 Feb 2019 12:47:46 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

Since kprobes handler runs in IRQ context, we can not use access_ok() in it. (only on x86 + CONFIG_DEBUG_ATOMIC_SLEEP=y)

Is it really IRQ context or exception context? That is, one (interrupts) happen for any task, but exceptions happen because of the software that is executed (like a breakpoint). Although you can have a kprobe trigger in an interrupt handler (where user access wouldn't make sense anyway). But there should be no problem with user access from an exception handler.

Can we just get rid of this might_sleep()? access_ok() doesn't sleep as far as I know.

Hmm, which might_sleep() would you pointed? What I talked was a WARN_ON_ONCE(!in_task()) in access_ok() on x86 (only!), and in_task() just checks preempt_count.

I guess PeterZ assumed that access_ok() is used only with user space access APIs (e.g. copy_from_user) which can cause page-fault and locks mm (and might sleep :)), but now we are trying to use access_ok() with new functions which disables page-fault and just return -EFAULT.

Thank you,

On Sun, 24 Feb 2019 09:26:45 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:

On Sun, Feb 24, 2019 at 7:18 AM Masami Hiramatsu mhiramat@kernel.org wrote:

...

On Sat, 23 Feb 2019 20:38:03 -0800 Andy Lutomirski luto@kernel.org wrote:

...

Can we just get rid of this might_sleep()? access_ok() doesn't sleep as far as I know.

Hmm, which might_sleep() would you pointed? What I talked was a WARN_ON_ONCE(!in_task()) in access_ok() on x86 (only!), and in_task() just checks preempt_count.

So the in_task() check does kind of make sense. Using "access_ok()" outside of task context is certainly an odd thing, for several reasons. The main one being simply that outside of task context, the whole "which task" question is open, and you don't know if the task is the active one, and so it's not clear if whatever task you interrupt might have done "set_fs()" or not.

Ah I got it. Usual case access_ok() in IRQ handler is strange.

So PeterZ isn't wrong:

...

I guess PeterZ assumed that access_ok() is used only with user space access APIs (e.g. copy_from_user) which can cause page-fault and locks mm (and might sleep :)), but now we are trying to use access_ok() with new functions which disables page-fault and just return -EFAULT.

.. but in this case, if we do it all *within* code that saves and restores the user access flag with get_fs/set_fs, access_ok() would be ok and it doesn't have the above issue.

So access_ok() in _general_ is absolutely not safe to do from interrupts, but within the context of probing user memory from a tracing event it just happens to be ok.

Hmm, but user can specify user-memory access from the tracing event which is located in interrupt handler. So I understand that it is safe only if we correctly setup access flag with get_fs/set_fs, is that correct?

It would be lovely to have a special macro for this, and keep the warning for the general case, but because this is a "every architecture needs to build their own" it's probably too painful.

Agreed.

PeterZ, do you remember the particular use case that triggered that commit 7c4788950ba5 ("x86/uaccess, sched/preempt: Verify access_ok() context")?

                Linus

Thank you,

On Sun, 24 Feb 2019 20:49:45 -0800 Andy Lutomirski luto@kernel.org wrote:

On Sun, Feb 24, 2019 at 6:40 PM Masami Hiramatsu mhiramat@kernel.org wrote:

...

On Sun, 24 Feb 2019 09:26:45 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:

...

On Sun, Feb 24, 2019 at 7:18 AM Masami Hiramatsu mhiramat@kernel.org wrote:

...

On Sat, 23 Feb 2019 20:38:03 -0800 Andy Lutomirski luto@kernel.org wrote:

...

Can we just get rid of this might_sleep()? access_ok() doesn't sleep as far as I know.

Hmm, which might_sleep() would you pointed? What I talked was a WARN_ON_ONCE(!in_task()) in access_ok() on x86 (only!), and in_task() just checks preempt_count.

So the in_task() check does kind of make sense. Using "access_ok()" outside of task context is certainly an odd thing, for several reasons. The main one being simply that outside of task context, the whole "which task" question is open, and you don't know if the task is the active one, and so it's not clear if whatever task you interrupt might have done "set_fs()" or not.

Ah I got it. Usual case access_ok() in IRQ handler is strange.

...

So PeterZ isn't wrong:

...

I guess PeterZ assumed that access_ok() is used only with user space access APIs (e.g. copy_from_user) which can cause page-fault and locks mm (and might sleep :)), but now we are trying to use access_ok() with new functions which disables page-fault and just return -EFAULT.

.. but in this case, if we do it all *within* code that saves and restores the user access flag with get_fs/set_fs, access_ok() would be ok and it doesn't have the above issue.

So access_ok() in _general_ is absolutely not safe to do from interrupts, but within the context of probing user memory from a tracing event it just happens to be ok.

Hmm, but user can specify user-memory access from the tracing event which is located in interrupt handler. So I understand that it is safe only if we correctly setup access flag with get_fs/set_fs, is that correct?

...

It would be lovely to have a special macro for this, and keep the warning for the general case, but because this is a "every architecture needs to build their own" it's probably too painful.

Agreed.

This should probably go with whatever effort makes nmi_uaccess_ok() available on all architectures. That being said, how about just making copy_from_user_nmi() work on all architectures, even if it just fails unconditionally on some of them?

I think even if we have copy_from_user_nmi(), we need something like nmi_uaccess_ok() because without it we can not correctly use __copy_from_user_inatomic()...

Thank you,

On Mon, 25 Feb 2019 11:40:18 -0500 Steven Rostedt rostedt@goodmis.org wrote:

On Mon, 25 Feb 2019 17:09:45 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

...

This should probably go with whatever effort makes nmi_uaccess_ok() available on all architectures. That being said, how about just making copy_from_user_nmi() work on all architectures, even if it just fails unconditionally on some of them?

I think even if we have copy_from_user_nmi(), we need something like nmi_uaccess_ok() because without it we can not correctly use __copy_from_user_inatomic()...

But wouldn't that just be part of the implementation of "copy_from_user_nmi()" as being in an NMI just assumes being inatomic?

Yes for copy_from_user_nmi(). But there are some other fundamental functions, like __get_user(). And when we optimize the loop in strncpy/strnlen from user in atomic, I think one nmi_access_ok() at entry is enough.

Thank you,

On Mon, Feb 25, 2019 at 8:48 AM Kees Cook keescook@chromium.org wrote:

On Sat, Feb 23, 2019 at 8:38 PM Andy Lutomirski luto@kernel.org wrote:

...

On Sat, Feb 23, 2019 at 4:44 PM Steven Rostedt rostedt@goodmis.org wrote:

...

On Sat, 23 Feb 2019 12:47:46 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

Since kprobes handler runs in IRQ context, we can not use access_ok() in it. (only on x86 + CONFIG_DEBUG_ATOMIC_SLEEP=y)

Is it really IRQ context or exception context? That is, one (interrupts) happen for any task, but exceptions happen because of the software that is executed (like a breakpoint). Although you can have a kprobe trigger in an interrupt handler (where user access wouldn't make sense anyway). But there should be no problem with user access from an exception handler.

Can we just get rid of this might_sleep()? access_ok() doesn't sleep as far as I know.

We do need to be aware of the userfaultfd case of getting held by userspace in the middle of a copy_*_user()... that's a whole other problem.

I sure hope that pagefault_disable() already takes care of this. Otherwise we have major problems already.

On Thu, 21 Feb 2019 09:36:25 -0500 Steven Rostedt rostedt@goodmis.org wrote:

On Thu, 21 Feb 2019 16:52:52 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

...

Basically OK to me. Could you use probe_kernel_read() in this context, since probe_mem_read() is a wrapper function for template code.

With that change,

Acked-by: Masami Hiramatsu mhiramat@kernel.org

This already hit Linus's tree. I was able to reproduce the crash, so I streamlined it. I should have still pushed more for your ack first. Sorry about that.

Oh, never mind. That seems urgent issue for kprobe event. Thank you very much for fixing it!

For some reason, I thought the change was in the generic probe code, and accepted the probe_mem_read(). Anyway, did you want to send a patch to change it to probe_kernel_read(), for the merge window?

No problem.

...

And for the long term, I need to find more efficient (or smarter) way to do it, like strnlen_user() does.

Agreed.

Thank you,