x86: Verify access_ok() context

I recently encountered wreckage because access_ok() was used where it should not be, add an explicit WARN when access_ok() is used wrongly.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/x86/include/asm/uaccess.h |  7 +++++--
 include/linux/preempt.h        | 21 +++++++++++++--------
 2 files changed, 18 insertions(+), 10 deletions(-)

arch/x86/include/asm/uaccess.h +0 -0

index faf3687f1035..b139c46ba122 100644

include/linux/preempt.h +10 -3

index 75e4e30677f1..7eeceac52dea 100644
@@ -65,19 +65,24 @@
 
 /*
  * Are we doing bottom half or hardware interrupt processing?
- * Are we in a softirq context? Interrupt context?
- * in_softirq - Are we currently processing softirq or have bh disabled?
- * in_serving_softirq - Are we currently processing softirq?
+ *
+ * in_irq()       - We're in (hard) IRQ context
+ * in_softirq()   - We have BH disabled, or are processing softirqs
+ * in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled
+ * in_serving_softirq() - We're in softirq context
+ * in_nmi()       - We're in NMI context
+ * in_task()     - We're in task context
+ *
+ * Note: due to the BH disabled confusion: in_softirq(),in_interrupt() really
+ *       should not be used in new code.
  */
 #define in_irq()               (hardirq_count())
 #define in_softirq()           (softirq_count())

arch/x86/include/asm/uaccess.h static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un · line 93

Quoted 3 lines · Peter Zijlstra

arch/x86/include/asm/uaccess.h +3 -0

+#define access_ok(type, addr, size)                                    \
+({                                                                     \
+       WARN_ON_ONCE(!in_task());                                       \

Should this be guarded by some debug option? This may hurt performance on production systems quite a bit.

For what it's worth, I think ARM recently started saving the address limit and resetting it to USER_DS on NMI entry.

CONFIG_DEBUG_VM seems somehow inappropriate.

Quoted 2 lines · Peter Zijlstra

CONFIG_DEBUG_VM seems somehow inappropriate.

The usual might_fault() logic? That uses

    defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP)

(and "might_sleep()" uses just CONFIG_DEBUG_ATOMIC_SLEEP, maybe that's fine).

If you call access_ok() with page faulting disabled, you'll still see this new warning.

If you put that new access_ok() call in a module that gets loaded/unloaded, you see one warning for every module load, which gets a bit annoying.

Quoted 2 lines · David Smith

If you call access_ok() with page faulting disabled, you'll still see this new warning.

And how so? It's just checking for task context. page fault disable/enable has absolutely nothing to do with that.

Quoted 3 lines

If you put that new access_ok() call in a module that gets loaded/unloaded, you see one warning for every module load, which gets a bit annoying.

Can you please elaborate where this access_ok() is placed in the module code?

Quoted 7 lines · Thomas Gleixner

Quoted 2 lines · David Smith

If you call access_ok() with page faulting disabled, you'll still see this new warning.

And how so? It's just checking for task context. page fault disable/enable has absolutely nothing to do with that.

True, task context and page fault disable/enable have nothing to do with each other. However, the access_ok() comment states:

* Context: User context only. This function may sleep if pagefaults are
* enabled.

That seems to indicate that the function won't sleep if pagefaults are disabled, and thus there is no need for a CONFIG_DEBUG_ATOMIC_SLEEP warning if pagefaults are disabled.

Quoted 7 lines

Quoted 4 lines

If you put that new access_ok() call in a module that gets loaded/unloaded, you see one warning for every module load, which gets a bit annoying.

Can you please elaborate where this access_ok() is placed in the module code?

It doesn't really matter where you place the access_ok() call in the module code. If you call access_ok() in a module, then that module has its own WARN_ON_ONCE() static variable. If access_ok() was a function exported from the kernel, then there would be only one copy of the WARN_ON_ONCE() static variable.

Quoted 2 lines · David Smith

Quoted 1 line · Thomas Gleixner

On Mon, 16 Jan 2017, David Smith wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

If you put that new access_ok() call in a module that gets loaded/unloaded, you see one warning for every module load, which gets a bit annoying.

Can you please elaborate where this access_ok() is placed in the module code?

Quoted 5 lines

It doesn't really matter where you place the access_ok() call in the module code. If you call access_ok() in a module, then that module has its own WARN_ON_ONCE() static variable. If access_ok() was a function exported from the kernel, then there would be only one copy of the WARN_ON_ONCE() static variable.

That doesn't seem like such a big deal to me.

ISTM even with pagefault_disable() in play, using access_ok() from, say, interrupt context is dangerous unless you've first checked that you're in a task. But I guess that in_task() would still return false, e.g. in perf.

Quoted 4 lines · Andy Lutomirski

ISTM even with pagefault_disable() in play, using access_ok() from, say, interrupt context is dangerous unless you've first checked that you're in a task. But I guess that in_task() would still return false, e.g. in perf.

The test was created exactly because perf was using access_ok() _wrongly_. See commit: ae31fe51a3cc ("perf/x86: Restore TASK_SIZE check on frame pointer").

Quoted 6 lines · Thomas Gleixner

Quoted 3 lines

If you put that new access_ok() call in a module that gets loaded/unloaded, you see one warning for every module load, which gets a bit annoying.

Can you please elaborate where this access_ok() is placed in the module code?

It doesn't really matter where you place the access_ok() call in the module code.

If you call access_ok() in a module, then that module has its own WARN_ON_ONCE() static variable. If access_ok() was a function exported from the kernel, then there would be only one copy of the WARN_ON_ONCE() static variable.

[...]
It does matter very much, because the fact that the warning triggers tells me that it's placed in code which is NOT executed in task context. [...]
We are not papering over problems.

Hi, Thomas -

Quoted 5 lines · Thomas Gleixner

[...]
It does matter very much, because the fact that the warning triggers tells me that it's placed in code which is NOT executed in task context. [...]
We are not papering over problems.

Understood. We were interpreting the comments around access_ok to mean that the underlying hazard condition was different (stricter) than in_task(). If the warning could be made to match that hazard condition more precisely, then safe but non-in_task() callers can use access_ok() without the warning.

Well, if you are not in thread context then the check is pointless:

	__range_not_ok(addr, size, user_addr_max())

and:
#define user_addr_max() (current->thread.addr_limit.seg)

So what guarantees when you are not in context of current, i.e. in thread context, that the addr/size which is checked against the limits of current actually belongs to current?

I assume this is about systemtap modules. Can you please explain what you are trying to achieve? I guess you know that you actually access current, but then we need a seperate special function and not relaxing of the checks.

Hi, Thomas -

Quoted 5 lines

Well, if you are not in thread context then the check is pointless:

	__range_not_ok(addr, size, user_addr_max())

and:
#define user_addr_max() (current->thread.addr_limit.seg)

Quoted 3 lines

So what guarantees when you are not in context of current, i.e. in thread context, that the addr/size which is checked against the limits of current actually belongs to current?

We're probably in task context in that there is a valid current(), but running with preemption and/or interrupts and/or pagefaults disabled at that point, so in_task() objects. Think of it like from a kprobes handler callback, except maybe more temporary preemption blocking.

Quoted 8 lines

Well, if you are not in thread context then the check is pointless:

	__range_not_ok(addr, size, user_addr_max())

and:
#define user_addr_max() (current->thread.addr_limit.seg)

So what guarantees when you are not in context of current, i.e. in thread context, that the addr/size which is checked against the limits of current actually belongs to current?

We're probably in task context in that there is a valid current(), but

running with preemption and/or interrupts and/or pagefaults disabled at that point, so in_task() objects.