Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560039 > unrolled thread

Re: [RFC][PATCH] x86: Verify access_ok() context

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH] x86: Verify access_ok() context David Smith <dsmith@redhat.com> - 2017-01-16 21:30 +0100
    Re: [RFC][PATCH] x86: Verify access_ok() context Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 22:50 +0100
      Re: [RFC][PATCH] x86: Verify access_ok() context David Smith <dsmith@redhat.com> - 2017-01-18 23:20 +0100
        Re: [RFC][PATCH] x86: Verify access_ok() context Andy Lutomirski <luto@amacapital.net> - 2017-01-19 01:30 +0100
          Re: [RFC][PATCH] x86: Verify access_ok() context David Smith <dsmith@redhat.com> - 2017-01-19 16:40 +0100
          Re: [RFC][PATCH] x86: Verify access_ok() context Peter Zijlstra <peterz@infradead.org> - 2017-01-20 09:30 +0100
            Re: [RFC][PATCH] x86: Verify access_ok() context Thomas Gleixner <tglx@linutronix.de> - 2017-01-20 10:00 +0100
        Re: [RFC][PATCH] x86: Verify access_ok() context Thomas Gleixner <tglx@linutronix.de> - 2017-01-19 19:50 +0100
          Re: [RFC][PATCH] x86: Verify access_ok() context "Frank Ch. Eigler" <fche@redhat.com> - 2017-01-19 21:30 +0100
            Re: [RFC][PATCH] x86: Verify access_ok() context Thomas Gleixner <tglx@linutronix.de> - 2017-01-19 22:20 +0100
              Re: [RFC][PATCH] x86: Verify access_ok() context "Frank Ch. Eigler" <fche@redhat.com> - 2017-01-19 22:30 +0100
                Re: [RFC][PATCH] x86: Verify access_ok() context Peter Zijlstra <peterz@infradead.org> - 2017-01-19 23:30 +0100
                Re: [RFC][PATCH] x86: Verify access_ok() context Thomas Gleixner <tglx@linutronix.de> - 2017-01-20 00:50 +0100

#1560039 — Re: [RFC][PATCH] x86: Verify access_ok() context

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.

How about modifying it like this:

---
From: David Smith <dsmith@redhat.com>
Date: Mon, 16 Jan 2017 14:07:31 -0600
Subject: [PATCH] Relax x86 new access_ok() warning a bit.

Signed-off-by: David Smith <dsmith@redhat.com>
---
 arch/x86/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ea148313570f..0cbd3cca5e7b 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -69,7 +69,7 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
 })
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
-# define WARN_ON_IN_IRQ()	WARN_ON_ONCE(!in_task())
+# define WARN_ON_IN_IRQ()	WARN_ON_ONCE(!in_task() && !pagefault_disabled())
 #else
 # define WARN_ON_IN_IRQ()
 #endif
-- 


-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

[toc] | [next] | [standalone]

#1560119

On Mon, 16 Jan 2017, David Smith wrote:

> 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.

> 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?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1562226

On 01/16/2017 03:14 PM, Thomas Gleixner wrote:
> On Mon, 16 Jan 2017, David Smith wrote:
> 
>> 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.

>> 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.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

[toc] | [prev] | [next] | [standalone]

#1562324

On Wed, Jan 18, 2017 at 2:16 PM, David Smith <dsmith@redhat.com> wrote:
> On 01/16/2017 03:14 PM, Thomas Gleixner wrote:
>> On Mon, 16 Jan 2017, David Smith wrote:
>>
>>> 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.

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.

>
>>> 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.

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

[toc] | [prev] | [next] | [standalone]

#1562828

On 01/18/2017 06:19 PM, Andy Lutomirski wrote:
> On Wed, Jan 18, 2017 at 2:16 PM, David Smith <dsmith@redhat.com> wrote:
>> On 01/16/2017 03:14 PM, Thomas Gleixner wrote:
>>> On Mon, 16 Jan 2017, David Smith wrote:

... stuff deleted ...

>>>> 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.
> 
> That doesn't seem like such a big deal to me.

To be clear here, I'm not suggesting we replace the access_ok() macro
with an exported kernel function. I'm just stating the fact that if you
have several modules that call the access_ok() macro (or one module that
gets loaded/unloaded/reloaded multiple times), each one can produce the
new warning. I'm not seeing a "flood" of these new warnings, but a
steady enough stream of them to be annoying.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

[toc] | [prev] | [next] | [standalone]

#1563375

On Wed, Jan 18, 2017 at 04:19:47PM -0800, Andy Lutomirski wrote:
> 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").

[toc] | [prev] | [next] | [standalone]

#1563396

On Fri, 20 Jan 2017, Peter Zijlstra wrote:

> On Wed, Jan 18, 2017 at 04:19:47PM -0800, Andy Lutomirski wrote:
> > 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").

If you validate a user space address against current outside the task
context, then what guarantees that this user space address belongs to
current? Nothing!

Sure, there are interrupts like breakpoints, etc. where we exactly know
that the address which we are looking at belongs to current, because the
code accesses soemthing which belongs exactly to that breakpoint. And in
these cases we need a check which is designed specifically for that case.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1563005

On Wed, 18 Jan 2017, David Smith wrote:
> On 01/16/2017 03:14 PM, Thomas Gleixner wrote:
> >> 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.

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.

> 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.

Not a big deal. If access_ok() is called from the wrong context in that
module then this should be fixed and not the warning supressed.

We are not papering over problems.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1563061

Hi, Thomas -

On Thu, Jan 19, 2017 at 07:12:48PM +0100, Thomas Gleixner wrote:
> [...]
> 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.

- FChE

[toc] | [prev] | [next] | [standalone]

#1563100

On Thu, 19 Jan 2017, Frank Ch. Eigler wrote:

> Hi, Thomas -
> 
> On Thu, Jan 19, 2017 at 07:12:48PM +0100, Thomas Gleixner wrote:
> > [...]
> > 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.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1563108

Hi, Thomas -

> 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.  Think of it like from a kprobes
handler callback, except maybe more temporary preemption blocking.


> 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.

This part is used in a part of the runtime that is a userspace
analogue of probe_kernel_address(), where we're given a potential
userspace address.  We would like to quickly test whether it's even
plausible as a userspace address, before doing a (pagefault-disabled)
trial fetch/store to it.


- FChE

[toc] | [prev] | [next] | [standalone]

#1563143

On Thu, Jan 19, 2017 at 04:27:18PM -0500, Frank Ch. Eigler wrote:
> Hi, Thomas -
> 
> > 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.  Think of it like from a kprobes
> handler callback, except maybe more temporary preemption blocking.

#define in_task()               (!(preempt_count() & \ 
                                   (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET))) 

So it doesn't care about preempt_disable(), and it doesn't care about
local_irq_disable(), it also doesn't care about local_bh_disable().

What it does care about are nmi_enter(), irq_enter() and __do_softirq().

[toc] | [prev] | [next] | [standalone]

#1563205

Frank.

On Thu, 19 Jan 2017, Frank Ch. Eigler wrote:
> > 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

current is always accessible no matter in which context you are - task,
softirq, hardirq, nmi ...

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

As Peter explained, neither preempt disable nor interrupt disable not
pagefault disabled have any influence on in_task(). It merily checks the
context: !in_softirq() && !in_hardirq() && !in_nmi().

So that warning happens definitely not from task context.

Care to share the code?

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web