* Re: [PATCH] mm/madvise: allow MADV_DONTNEED to free memory that is MLOCK_ONFAULT
From: Jason Baron @ 2018-06-28 20:20 UTC (permalink / raw)
To: Michal Hocko
Cc: Vlastimil Babka, akpm, linux-kernel, linux-mm, Joonsoo Kim,
Mel Gorman, Kirill A. Shutemov, linux-api, emunson
In-Reply-To: <20180620110022.GK13685@dhcp22.suse.cz>
On 06/20/2018 07:00 AM, Michal Hocko wrote:
> On Fri 15-06-18 15:36:07, Jason Baron wrote:
>>
>>
>> On 06/13/2018 03:15 AM, Michal Hocko wrote:
>>> On Wed 13-06-18 08:32:19, Vlastimil Babka wrote:
> [...]
>>>> BTW I didn't get why we should allow this for MADV_DONTNEED but not
>>>> MADV_FREE. Can you expand on that?
>>>
>>> Well, I wanted to bring this up as well. I guess this would require some
>>> more hacks to handle the reclaim path correctly because we do rely on
>>> VM_LOCK at many places for the lazy mlock pages culling.
>>>
>>
>> The point of not allowing MADV_FREE on mlock'd pages for me was that
>> with mlock and even MLOCK_ON_FAULT, one can always can always determine
>> if a page is present or not (and thus avoid the major fault). Allowing
>> MADV_FREE on lock'd pages breaks that assumption.
>
> But once you have called MADV_FREE you cannot assume anything about the
> content until you touch the memory again. So you can safely assume a
> major fault for the worst case. Btw. why knowing whether you major fault
> is important in the first place? What is an application going to do
> about that information?
>
Fair enough, I think that means you end up with a MADV_FREE_FORCE to
support that case? As I said I worked around this by using tmpfs and
fallocate(FALLOC_FL_PUNCH_HOLE). However, I still think there is a
use-case for doing this for anonymous memory, to avoid the unlock() calls.
The use-case I had in mind was simply an application that has a fast
path for when it knows that the requested item is locked in memory.
Thanks,
-Jason
^ permalink raw reply
* Re: [RFC PATCH for 4.18 2/2] rseq: check that rseq->rseq_cs padding is zero
From: Will Deacon @ 2018-06-28 16:53 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H . Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Linus Torvalds, Catalin Marinas,
Michael Kerrisk
In-Reply-To: <20180628162359.9054-2-mathieu.desnoyers@efficios.com>
Hi Mathieu,
On Thu, Jun 28, 2018 at 12:23:59PM -0400, Mathieu Desnoyers wrote:
> On 32-bit kernels, the rseq->rseq_cs_padding field is never read by the
> kernel. However, 64-bit kernels dealing with 32-bit compat tasks read the
> full 64-bit in its entirety, and terminates the offending process with
> a segmentation fault if the upper 32 bits are set due to failure of
> copy_from_user().
>
> Ensure that both 32-bit and 64-bit kernels dealing with 32-bit tasks end
> up terminating offending tasks with a segmentation fault if the upper
> 32-bit padding bits (rseq->rseq_cs_padding) are set by adding an explicit
> check that padding is zero on 32-bit kernels.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Paul Turner <pjt@google.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Andi Kleen <andi@firstfloor.org>
> CC: Dave Watson <davejwatson@fb.com>
> CC: Chris Lameter <cl@linux.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Ben Maurer <bmaurer@fb.com>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Josh Triplett <josh@joshtriplett.org>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Russell King <linux@arm.linux.org.uk>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Michael Kerrisk <mtk.manpages@gmail.com>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: linux-api@vger.kernel.org
> ---
> kernel/rseq.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 4ba582046fcd..b038f35a60d6 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -112,6 +112,29 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> return 0;
> }
>
> +#ifndef __LP64__
> +/*
> + * Ensure that padding is zero.
> + */
> +static int check_rseq_cs_padding(struct task_struct *t)
> +{
> + unsigned long pad;
> + int ret;
> +
> + ret = __get_user(pad, &t->rseq->rseq_cs_padding);
> + if (ret)
> + return ret;
> + if (pad)
> + return -EFAULT;
> + return 0;
> +}
> +#else
> +static int check_rseq_cs_padding(struct task_struct *t)
> +{
> + return 0;
> +}
> +#endif
I'm still not sure how this works with a 64-bit kernel and a compat (32-bit)
task. The check_rseq_cs_padding() will return 0 regardless of the upper bits
of the rseq_cs field, whereas a native 32-bit kernel would actually go and
check them.
What am I missing here?
Will
^ permalink raw reply
* [RFC PATCH for 4.18 2/2] rseq: check that rseq->rseq_cs padding is zero
From: Mathieu Desnoyers @ 2018-06-28 16:23 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michael Kerrisk
In-Reply-To: <20180628162359.9054-1-mathieu.desnoyers@efficios.com>
On 32-bit kernels, the rseq->rseq_cs_padding field is never read by the
kernel. However, 64-bit kernels dealing with 32-bit compat tasks read the
full 64-bit in its entirety, and terminates the offending process with
a segmentation fault if the upper 32 bits are set due to failure of
copy_from_user().
Ensure that both 32-bit and 64-bit kernels dealing with 32-bit tasks end
up terminating offending tasks with a segmentation fault if the upper
32-bit padding bits (rseq->rseq_cs_padding) are set by adding an explicit
check that padding is zero on 32-bit kernels.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
kernel/rseq.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 4ba582046fcd..b038f35a60d6 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -112,6 +112,29 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
return 0;
}
+#ifndef __LP64__
+/*
+ * Ensure that padding is zero.
+ */
+static int check_rseq_cs_padding(struct task_struct *t)
+{
+ unsigned long pad;
+ int ret;
+
+ ret = __get_user(pad, &t->rseq->rseq_cs_padding);
+ if (ret)
+ return ret;
+ if (pad)
+ return -EFAULT;
+ return 0;
+}
+#else
+static int check_rseq_cs_padding(struct task_struct *t)
+{
+ return 0;
+}
+#endif
+
static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
@@ -123,6 +146,8 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
ret = __get_user(ptr, &t->rseq->rseq_cs);
if (ret)
return ret;
+ if (check_rseq_cs_padding(t))
+ return -EFAULT;
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
--
2.11.0
^ permalink raw reply related
* [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-28 16:23 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michael Kerrisk
Validating the abort_ip field of rseq_cs ensures that the kernel don't
return to an invalid address when returning to userspace after an abort.
I don't fully trust each architecture code to cleanly deal with invalid
return addresses.
Validating the range [ start_ip, start_ip + post_commit_offset ] is an
extra validation step ensuring that userspace provides valid values to
describe the critical section.
If validation fails, the process is killed with a segmentation fault.
Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
that ignoring the 32 upper bits on 32-bit kernels. This ensures we have
a consistent behavior for a 32-bit binary executed on 32-bit kernels and
in compat mode on 64-bit kernels.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
include/uapi/linux/rseq.h | 6 +++---
kernel/rseq.c | 7 +++++--
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index d620fa43756c..519ad6e176d1 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -52,10 +52,10 @@ struct rseq_cs {
__u32 version;
/* enum rseq_cs_flags */
__u32 flags;
- LINUX_FIELD_u32_u64(start_ip);
+ __u64 start_ip;
/* Offset from start_ip. */
- LINUX_FIELD_u32_u64(post_commit_offset);
- LINUX_FIELD_u32_u64(abort_ip);
+ __u64 post_commit_offset;
+ __u64 abort_ip;
} __attribute__((aligned(4 * sizeof(__u64))));
/*
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 22b6acf1ad63..4ba582046fcd 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -128,7 +128,10 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
return 0;
}
urseq_cs = (struct rseq_cs __user *)ptr;
- if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
+ if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)) ||
+ rseq_cs->abort_ip >= TASK_SIZE ||
+ rseq_cs->start_ip >= TASK_SIZE ||
+ rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE)
return -EFAULT;
if (rseq_cs->version > 0)
return -EINVAL;
@@ -137,7 +140,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
return -EINVAL;
- usig = (u32 __user *)(rseq_cs->abort_ip - sizeof(u32));
+ usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
ret = get_user(sig, usig);
if (ret)
return ret;
--
2.11.0
^ permalink raw reply related
* Re: [RFC PATCH for 4.18 2/2] rseq: compat: clear high bits of rseq_cs fields
From: Thomas Gleixner @ 2018-06-28 8:04 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Mathieu Desnoyers, linux-kernel, Joel Fernandes, Peter Zijlstra,
Catalin Marinas, Dave Watson, Will Deacon, Andi Kleen,
H . Peter Anvin, Chris Lameter, Russell King, Andrew Hunter,
Michael Kerrisk, Paul E . McKenney, Paul Turner, Boqun Feng,
Josh Triplett, Steven Rostedt, Ben Maurer, linux-api, linux-arch,
x86
In-Reply-To: <D9B93DE9-F61C-4488-B109-432C2DFDCEC2@amacapital.net>
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
On Tue, 26 Jun 2018, Andy Lutomirski wrote:
> > On Jun 26, 2018, at 2:16 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >
> > Make the behavior rseq on compat tasks more robust by ensuring that
> > kernel/rseq.c:rseq_get_rseq_cs() clears the high bits of
> > rseq_cs->abort_ip, rseq_cs->start_ip and rseq_cs->post_commit_offset
> > when a 32-bit binary is run on a 64-bit kernel.
> >
> > The intent here is that if user-space has garbage rather than zeroes
> > in its struct rseq_cs fields padding, the behavior will be the same
> > whether the binary is run on 32-bit or 64-bit kernels.
> >
> > Use in_compat_syscall() when rseq_get_rseq_cs() is invoked from
> > system call context, and use is_compat_frame() when invoked from
> > signal delivery.
> >
>
> And when it’s invoked due to preemption unrelated to a syscall or signal,
> you malfunction?
>
> I think the only sane solution is to make these fields be u64, delete the
> LINUX_FIELD_ macros, and possibly teach the x86 slowpath return to inject
> a signal if it’s trying to return to a 32-bit context with garbage in the
> high bits of regs->ip so that we determistically fail if the user screws
> up.
Right. That's the only sane solution. Trying to play games with 32/64bit
for a dubious value is going to bite us within no time and just create ugly
workarounds left and right. Forcing a clear handling upfront avoids all of
that.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] mm: reject MAP_SHARED_VALIDATE without new flags
From: Eric Sandeen @ 2018-06-28 4:18 UTC (permalink / raw)
To: Linus Torvalds, Eric Sandeen
Cc: Christoph Hellwig, Jan Kara,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, Linux API,
zhibli-H+wXaHxf7aLQT0dZR+AlfA, linux-xfs, linux-mm, linux-fsdevel,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CA+55aFxs7Cc30fCiENw0R+XDJhUJ-w=z=NLLzYfT5gF2Qh-60Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 6/27/18 9:37 PM, Linus Torvalds wrote:
> On Wed, Jun 27, 2018 at 7:17 PM Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org> wrote:
>>
>> What broke is that mmap(MAP_SHARED|MAP_PRIVATE) now succeeds without error,
>> whereas before it rightly returned -EINVAL.
>
> You're still confusing *behavior* with breakage.
>
> Yes. New *behavior* is that MAP_SHARED|MAP_PRIVATE is now a valid
> thing. It means "MAP_SHARED_VALIDATE".
>
> Behavior changed. That's normal. Every single time we add a system
> call, behavior changes: a system call that used to return -ENOSYS now
> returns something else.
>
> That's not breakage, that's just intentional new behavior.
*shrug* semantics aside, the new behavior is out there in a public
API, so I guess there's nothing to do at this point other than
to document the change more clearly. It's true that my patch could
possibly break existing users.
The man page is clearly wrong at this point, both in terms of the
error code section, and the claim that MAP_SHARED and MAP_PRIVATE
behave as described in POSIX (because POSIX states that these
two flags may not be specified together.)
-Eric
^ permalink raw reply
* Re: [PATCH] mm: reject MAP_SHARED_VALIDATE without new flags
From: Linus Torvalds @ 2018-06-28 2:37 UTC (permalink / raw)
To: Eric Sandeen
Cc: Eric Sandeen, Jan Kara,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, Linux API,
zhibli-H+wXaHxf7aLQT0dZR+AlfA, linux-xfs, linux-mm,
Christoph Hellwig, linux-fsdevel,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1e2ad827-6ff4-4b1e-c4d9-79ca4e432a6c-+82itfer+wXR7s880joybQ@public.gmane.org>
On Wed, Jun 27, 2018 at 7:17 PM Eric Sandeen <sandeen-+82itfer+wXR7s880joybQ@public.gmane.org> wrote:
>
> What broke is that mmap(MAP_SHARED|MAP_PRIVATE) now succeeds without error,
> whereas before it rightly returned -EINVAL.
You're still confusing *behavior* with breakage.
Yes. New *behavior* is that MAP_SHARED|MAP_PRIVATE is now a valid
thing. It means "MAP_SHARED_VALIDATE".
Behavior changed. That's normal. Every single time we add a system
call, behavior changes: a system call that used to return -ENOSYS now
returns something else.
That's not breakage, that's just intentional new behavior.
> What behavior should a user expect from a successful mmap(MAP_SHARED|MAP_PRIVATE)?
MAP_SHARED|MAP_PRIVATE makes no sense and nobody uses it (because it
has always returned an error and never done anything interesting).
Nobody uses it, and it used to return an error is *exactly* why it was
defined to be MAP_SHARED_VALIDATE.
So you should expect MAP_SHARED_VALIDATE behavior - which is
MAP_SHARED together with "validate that all the flags are things that
we support".
Actual BREAKAGE is if some application or user workflow no longer
works. Did LibreOffice stop working? That is breakage.
And by application, I mean exactly that: a real program. Not some
manual-page, and not some test-program that people don't actually rely
on, and that just reports on some particular behavior.
Because I can write a test program that verifies that system call #335
doesn't exist:
#define _GNU_SOURCE
#include <unistd.h>
#include <sys/syscall.h>
#include <errno.h>
#include <assert.h>
int main(int argc, char **argv)
{
assert(syscall(335, 0) == -1 && errno == ENOSYS);
return 0;
}
and the next system call we add will break that test program on x86-64.
And that's still not a "regression" - it's just a change in behavior.
But if firefox no longer runs, because it depended on that system call
not existing (or it depended on that MAP_SHARED_VALIDATE returning
EINVAL) then it's a regression.
See the difference?
One case is "we added new behavior".
The other case is "we have a regression".
Linus
^ permalink raw reply
* Re: [PATCH] mm: reject MAP_SHARED_VALIDATE without new flags
From: Eric Sandeen @ 2018-06-28 2:17 UTC (permalink / raw)
To: Linus Torvalds, Eric Sandeen
Cc: Christoph Hellwig, Jan Kara,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, Linux API,
zhibli-H+wXaHxf7aLQT0dZR+AlfA, linux-xfs, linux-mm, linux-fsdevel,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CA+55aFzeA7N3evSF2jKHu8JoTQuKDLCMKx7RiPhmym97-8HY7A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 6/27/18 9:10 PM, Linus Torvalds wrote:
> On Wed, Jun 27, 2018 at 6:45 PM Eric Sandeen <sandeen-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> Thus the invalid flag combination of (MAP_SHARED|MAP_PRIVATE) now
>> passes without error, which is a regression.
>
> It's not a regression, it's just new behavior.
>
> "regression" doesn't mean "things changed". It means "something broke".
>
> What broke?
My commit log perhaps was not clear enough.
What broke is that mmap(MAP_SHARED|MAP_PRIVATE) now succeeds without error,
whereas before it rightly returned -EINVAL.
What behavior should a user expect from a successful mmap(MAP_SHARED|MAP_PRIVATE)?
-Eric
> Because if it's some manual page breakage, just fix the manual. That's
> what "new behavior" is all about.
>
> There is nothing that says that "MAP_SHARED_VALIDATE" can't work with
> just the legacy flags.
>
> Because I'd be worried about your patch breaking some actual new user
> of MAP_SHARED_VALIDATE.
>
> Because it's actual *users* of behavior we care about, not some
> test-suite or manual pages.
>
> Linus
^ permalink raw reply
* Re: [PATCH] mm: reject MAP_SHARED_VALIDATE without new flags
From: Linus Torvalds @ 2018-06-28 2:10 UTC (permalink / raw)
To: Eric Sandeen
Cc: Christoph Hellwig, Jan Kara,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, Linux API,
zhibli-H+wXaHxf7aLQT0dZR+AlfA, linux-xfs, linux-mm, linux-fsdevel,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <60052659-7b37-cb69-bf9f-1683caa46219-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Jun 27, 2018 at 6:45 PM Eric Sandeen <sandeen-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Thus the invalid flag combination of (MAP_SHARED|MAP_PRIVATE) now
> passes without error, which is a regression.
It's not a regression, it's just new behavior.
"regression" doesn't mean "things changed". It means "something broke".
What broke?
Because if it's some manual page breakage, just fix the manual. That's
what "new behavior" is all about.
There is nothing that says that "MAP_SHARED_VALIDATE" can't work with
just the legacy flags.
Because I'd be worried about your patch breaking some actual new user
of MAP_SHARED_VALIDATE.
Because it's actual *users* of behavior we care about, not some
test-suite or manual pages.
Linus
^ permalink raw reply
* [PATCH] mm: reject MAP_SHARED_VALIDATE without new flags
From: Eric Sandeen @ 2018-06-28 1:45 UTC (permalink / raw)
To: fsdevel, linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Linux API, linux-xfs,
linux-ext4-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nvdimm
Cc: Christoph Hellwig, Linus Torvalds, Jan Kara, Zhibin Li
mmap(2) says the syscall will return EINVAL if "flags contained neither
MAP_PRIVATE or MAP_SHARED, or contained both of these values."
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
However, commit
1c972597 ("mm: introduce MAP_SHARED_VALIDATE ...")
introduced a new flag, MAP_SHARED_VALIDATE, with a value of 0x3,
which is indistinguishable from (MAP_SHARED|MAP_PRIVATE).
Thus the invalid flag combination of (MAP_SHARED|MAP_PRIVATE) now
passes without error, which is a regression.
I'm not sure of the best way out of this, other than to change the
API description to say that MAP_SHARED_VALIDATE is only allowed in
combination with "new" flags, and reject it if it's used only with
flags contained in LEGACY_MAP_MASK.
This will require the mmap(2) manpage to enumerate which flags don't
require validation, as well, so the user knows when to use the
VALIDATE flag.
I'm not super happy with this, because it also means that code
which explicitly asks for mmap(MAP_SHARED|MAP_PRIVATE|MAP_SYNC) will
also pass, but I'm not sure there's anything to do about that.
Reported-by: Zhibin Li <zhibli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Eric Sandeen <sandeen-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
diff --git a/mm/mmap.c b/mm/mmap.c
index d1eb87ef4b1a..b1dc84466365 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1440,6 +1440,16 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
if (!file_mmap_ok(file, inode, pgoff, len))
return -EOVERFLOW;
+ /*
+ * MAP_SHARED_VALIDATE is indistinguishable from
+ * (MAP_SHARED|MAP_PRIVATE) which must return -EINVAL.
+ * If the flags contain MAP_SHARED_VALIDATE and none of the
+ * non-legacy flags, the user gets EINVAL.
+ */
+ if (((flags & MAP_SHARED_VALIDATE) == MAP_SHARED_VALIDATE) &&
+ !(flags & ~LEGACY_MAP_MASK)) {
+ return -EINVAL;
+ }
flags_mask = LEGACY_MAP_MASK | file->f_op->mmap_supported_flags;
^ permalink raw reply related
* Re: [RFC PATCH for 4.18 2/2] rseq: compat: clear high bits of rseq_cs fields
From: Mathieu Desnoyers @ 2018-06-26 22:17 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Thomas Gleixner, linux-kernel, Joel Fernandes, Peter Zijlstra,
Catalin Marinas, Dave Watson, Will Deacon, Andi Kleen,
H. Peter Anvin, Chris Lameter, Russell King, Andrew Hunter,
Michael Kerrisk, Paul E. McKenney, Paul Turner, Boqun Feng,
Josh Triplett, rostedt, Ben Maurer, linux-api
In-Reply-To: <D9B93DE9-F61C-4488-B109-432C2DFDCEC2@amacapital.net>
----- On Jun 26, 2018, at 5:58 PM, Andy Lutomirski luto@amacapital.net wrote:
>> On Jun 26, 2018, at 2:16 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> wrote:
>>
>> Make the behavior rseq on compat tasks more robust by ensuring that
>> kernel/rseq.c:rseq_get_rseq_cs() clears the high bits of
>> rseq_cs->abort_ip, rseq_cs->start_ip and rseq_cs->post_commit_offset
>> when a 32-bit binary is run on a 64-bit kernel.
>>
>> The intent here is that if user-space has garbage rather than zeroes
>> in its struct rseq_cs fields padding, the behavior will be the same
>> whether the binary is run on 32-bit or 64-bit kernels.
>>
>> Use in_compat_syscall() when rseq_get_rseq_cs() is invoked from
>> system call context, and use is_compat_frame() when invoked from
>> signal delivery.
>>
>
> And when it’s invoked due to preemption unrelated to a syscall or signal, you
> malfunction?
Fair point! Hence the "RFC". ;)
So I understand better your intent to use the pt_regs to figure out whether it
is compat or not. My is_compat_frame()+in_compat_syscall() approach does not
handle this correctly.
>
> I think the only sane solution is to make these fields be u64,
I'm OK with turning the rseq_cs start_ip, post_commit_offset, and abort_ip
fields into normal u64.
> delete the
> LINUX_FIELD_ macros,
The LINUX_FIELD_ macros are still needed to ensure single-copy updates of
the (struct rseq *__tls_abi)->rseq_cs pointer by 32-bit user-space.
> and possibly teach the x86 slowpath return to inject a
> signal if it’s trying to return to a 32-bit context with garbage in the high
> bits of regs->ip so that we determistically fail if the user screws up.
I like the approach of dealing with the rseq_cs fields as u64 even on 32-bit
architectures. As a downside, it will require 32-bit architectures to do
arithmetic on 64-bit values, but it's not a fast-path. As you point out, the
tricky bit is to decide what happens when architecture code returns to
userspace with regs->ip containing garbage in the high bits.
An alternative approach is to ensure the high bits are cleared when returning
to an IP with garbage in the high bits.
> Rseq is brand new. It should not need compat code at all.
Dealing with u64 for start_ip, post_commit_offset, and abort_ip at the kernel
level would indeed provide this characteristic. However, I'm uneasy adding
64-bit arithmetic on operations really caring about 32 bits on 32-bit archs,
even though those are not fast paths.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 2/2] rseq: compat: clear high bits of rseq_cs fields
From: Andy Lutomirski @ 2018-06-26 21:58 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, linux-kernel, Joel Fernandes, Peter Zijlstra,
Catalin Marinas, Dave Watson, Will Deacon, Andi Kleen,
H . Peter Anvin, Chris Lameter, Russell King, Andrew Hunter,
Michael Kerrisk, Paul E . McKenney, Paul Turner, Boqun Feng,
Josh Triplett, Steven Rostedt, Ben Maurer, linux-api, linux-arch,
x86
In-Reply-To: <20180626211617.8933-2-mathieu.desnoyers@efficios.com>
> On Jun 26, 2018, at 2:16 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
> Make the behavior rseq on compat tasks more robust by ensuring that
> kernel/rseq.c:rseq_get_rseq_cs() clears the high bits of
> rseq_cs->abort_ip, rseq_cs->start_ip and rseq_cs->post_commit_offset
> when a 32-bit binary is run on a 64-bit kernel.
>
> The intent here is that if user-space has garbage rather than zeroes
> in its struct rseq_cs fields padding, the behavior will be the same
> whether the binary is run on 32-bit or 64-bit kernels.
>
> Use in_compat_syscall() when rseq_get_rseq_cs() is invoked from
> system call context, and use is_compat_frame() when invoked from
> signal delivery.
>
And when it’s invoked due to preemption unrelated to a syscall or signal, you malfunction?
I think the only sane solution is to make these fields be u64, delete the LINUX_FIELD_ macros, and possibly teach the x86 slowpath return to inject a signal if it’s trying to return to a 32-bit context with garbage in the high bits of regs->ip so that we determistically fail if the user screws up.
Rseq is brand new. It should not need compat code at all.
^ permalink raw reply
* [RFC PATCH for 4.18 2/2] rseq: compat: clear high bits of rseq_cs fields
From: Mathieu Desnoyers @ 2018-06-26 21:16 UTC (permalink / raw)
To: Andy Lutomirski, Thomas Gleixner
Cc: linux-kernel, Mathieu Desnoyers, Joel Fernandes, Peter Zijlstra,
Catalin Marinas, Dave Watson, Will Deacon, Andi Kleen,
H . Peter Anvin, Chris Lameter, Russell King, Andrew Hunter,
Michael Kerrisk, Paul E . McKenney, Paul Turner, Boqun Feng,
Josh Triplett, Steven Rostedt, Ben Maurer, linux-api, linux-arch
In-Reply-To: <20180626211617.8933-1-mathieu.desnoyers@efficios.com>
Make the behavior rseq on compat tasks more robust by ensuring that
kernel/rseq.c:rseq_get_rseq_cs() clears the high bits of
rseq_cs->abort_ip, rseq_cs->start_ip and rseq_cs->post_commit_offset
when a 32-bit binary is run on a 64-bit kernel.
The intent here is that if user-space has garbage rather than zeroes
in its struct rseq_cs fields padding, the behavior will be the same
whether the binary is run on 32-bit or 64-bit kernels.
Use in_compat_syscall() when rseq_get_rseq_cs() is invoked from
system call context, and use is_compat_frame() when invoked from
signal delivery.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Joel Fernandes <joelaf@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Watson <davejwatson@fb.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Chris Lameter <cl@linux.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Hunter <ahh@google.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Paul Turner <pjt@google.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Maurer <bmaurer@fb.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
kernel/rseq.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 22b6acf1ad63..7b1d51b965fc 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -13,6 +13,7 @@
#include <linux/syscalls.h>
#include <linux/rseq.h>
#include <linux/types.h>
+#include <linux/compat.h>
#include <asm/ptrace.h>
#define CREATE_TRACE_POINTS
@@ -112,7 +113,23 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
return 0;
}
-static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
+#ifdef CONFIG_COMPAT
+static void rseq_cs_compat(struct ksignal *ksig, struct rseq_cs *rseq_cs)
+{
+ if (!(ksig ? is_compat_frame(ksig) : in_compat_syscall()))
+ return;
+
+ rseq_cs->abort_ip = (compat_uptr_t) rseq_cs->abort_ip;
+ rseq_cs->start_ip = (compat_uptr_t) rseq_cs->start_ip;
+ rseq_cs->post_commit_offset =
+ (compat_uptr_t) rseq_cs->post_commit_offset;
+}
+#else
+static void rseq_cs_compat(struct ksignal *ksig, struct rseq_cs *rseq_cs) { }
+#endif
+
+static int rseq_get_rseq_cs(struct ksignal *ksig, struct task_struct *t,
+ struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
unsigned long ptr;
@@ -132,6 +149,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
return -EFAULT;
if (rseq_cs->version > 0)
return -EINVAL;
+ rseq_cs_compat(ksig, rseq_cs);
/* Ensure that abort_ip is not in the critical section. */
if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
@@ -209,14 +227,14 @@ static bool in_rseq_cs(unsigned long ip, struct rseq_cs *rseq_cs)
return ip - rseq_cs->start_ip < rseq_cs->post_commit_offset;
}
-static int rseq_ip_fixup(struct pt_regs *regs)
+static int rseq_ip_fixup(struct ksignal *ksig, struct pt_regs *regs)
{
unsigned long ip = instruction_pointer(regs);
struct task_struct *t = current;
struct rseq_cs rseq_cs;
int ret;
- ret = rseq_get_rseq_cs(t, &rseq_cs);
+ ret = rseq_get_rseq_cs(ksig, t, &rseq_cs);
if (ret)
return ret;
@@ -260,7 +278,7 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
return;
if (unlikely(!access_ok(VERIFY_WRITE, t->rseq, sizeof(*t->rseq))))
goto error;
- ret = rseq_ip_fixup(regs);
+ ret = rseq_ip_fixup(ksig, regs);
if (unlikely(ret < 0))
goto error;
if (unlikely(rseq_update_cpu_id(t)))
@@ -287,7 +305,7 @@ void rseq_syscall(struct pt_regs *regs)
if (!t->rseq)
return;
if (!access_ok(VERIFY_READ, t->rseq, sizeof(*t->rseq)) ||
- rseq_get_rseq_cs(t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
+ rseq_get_rseq_cs(NULL, t, &rseq_cs) || in_rseq_cs(ip, &rseq_cs))
force_sig(SIGSEGV, t);
}
--
2.11.0
^ permalink raw reply related
* [RFC PATCH for 4.18 1/2] compat: Introduce is_compat_frame
From: Mathieu Desnoyers @ 2018-06-26 21:16 UTC (permalink / raw)
To: Andy Lutomirski, Thomas Gleixner
Cc: linux-kernel, Mathieu Desnoyers, Joel Fernandes, Peter Zijlstra,
Catalin Marinas, Dave Watson, Will Deacon, Andi Kleen,
H . Peter Anvin, Chris Lameter, Russell King, Andrew Hunter,
Michael Kerrisk, Paul E . McKenney, Paul Turner, Boqun Feng,
Josh Triplett, Steven Rostedt, Ben Maurer, linux-api, linux-arch
x86 is moving from is_compat_task() to in_compat_syscall(). However,
in_compat_syscall cannot be used to check whether a signal is being
delivered on a compat task.
Introduce is_compat_frame to allow performing this check from
architecture agnostic code. On all architectures except x86, it
invokes is_compat_task(). On x86, it uses is_ia32_frame() and
is_x32_frame() to check whether the signal frame is 32-bit.
This is needed by restartable sequences to detect whether it needs
to clear the top bits of the start_ip, abort_ip, and post_commit_offset
rseq_cs fields on signal delivery, thus ensuring identical behavior
for a 32-bit binary executed on 32-bit and 64-bit kernels.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Joel Fernandes <joelaf@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Watson <davejwatson@fb.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Chris Lameter <cl@linux.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Hunter <ahh@google.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Paul Turner <pjt@google.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Maurer <bmaurer@fb.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
arch/x86/include/asm/compat.h | 24 ++++++++++++++++++++++++
arch/x86/kernel/signal.c | 17 -----------------
include/linux/compat.h | 17 +++++++++++++++++
3 files changed, 41 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index fb97cf7c4137..1405a8df5215 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -8,6 +8,7 @@
#include <linux/types.h>
#include <linux/sched.h>
#include <linux/sched/task_stack.h>
+#include <linux/signal.h>
#include <asm/processor.h>
#include <asm/user32.h>
#include <asm/unistd.h>
@@ -242,4 +243,27 @@ struct compat_siginfo;
int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
const siginfo_t *from, bool x32_ABI);
+static inline int is_ia32_compat_frame(struct ksignal *ksig)
+{
+ return IS_ENABLED(CONFIG_IA32_EMULATION) &&
+ ksig->ka.sa.sa_flags & SA_IA32_ABI;
+}
+
+static inline int is_ia32_frame(struct ksignal *ksig)
+{
+ return IS_ENABLED(CONFIG_X86_32) || is_ia32_compat_frame(ksig);
+}
+
+static inline int is_x32_frame(struct ksignal *ksig)
+{
+ return IS_ENABLED(CONFIG_X86_X32_ABI) &&
+ ksig->ka.sa.sa_flags & SA_X32_ABI;
+}
+
+static inline bool is_compat_frame(struct ksignal *ksig)
+{
+ return is_ia32_frame(ksig) || is_x32_frame(ksig);
+}
+#define is_compat_frame is_compat_frame /* override the generic impl */
+
#endif /* _ASM_X86_COMPAT_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 92a3b312a53c..cb488e3e952d 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -664,23 +664,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
return 0;
}
-static inline int is_ia32_compat_frame(struct ksignal *ksig)
-{
- return IS_ENABLED(CONFIG_IA32_EMULATION) &&
- ksig->ka.sa.sa_flags & SA_IA32_ABI;
-}
-
-static inline int is_ia32_frame(struct ksignal *ksig)
-{
- return IS_ENABLED(CONFIG_X86_32) || is_ia32_compat_frame(ksig);
-}
-
-static inline int is_x32_frame(struct ksignal *ksig)
-{
- return IS_ENABLED(CONFIG_X86_X32_ABI) &&
- ksig->ka.sa.sa_flags & SA_X32_ABI;
-}
-
static int
setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
{
diff --git a/include/linux/compat.h b/include/linux/compat.h
index b1a5562b3215..2e1ffba65117 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -1022,6 +1022,19 @@ static inline struct compat_timeval ns_to_compat_timeval(s64 nsec)
return ctv;
}
+/*
+ * For most but not all architectures, "is this a compat sigframe?" and
+ * "am I a compat task?" are the same question. For architectures on which
+ * they aren't the same question, arch code can override is_compat_frame.
+ */
+
+#ifndef is_compat_frame
+static inline bool is_compat_frame(struct ksignal *ksig)
+{
+ return is_compat_task();
+}
+#endif
+
#else /* !CONFIG_COMPAT */
#define is_compat_task() (0)
@@ -1029,6 +1042,10 @@ static inline struct compat_timeval ns_to_compat_timeval(s64 nsec)
static inline bool in_compat_syscall(void) { return false; }
#endif
+#ifndef is_compat_frame
+static inline bool is_compat_frame(struct ksignal *ksig) { return false; }
+#endif
+
#endif /* CONFIG_COMPAT */
#endif /* _LINUX_COMPAT_H */
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 00/10] Control Flow Enforcement - Part (3)
From: Yu-cheng Yu @ 2018-06-26 14:56 UTC (permalink / raw)
To: Andy Lutomirski, Linux API, Jann Horn, Florian Weimer
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrWYx5nCtwGAqTZBWOB+aw+eEcnQhe6Sn1o+O356g7Km9A@mail.gmail.com>
On Mon, 2018-06-25 at 22:26 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com>
> wrote:
> >
> >
> > This series introduces CET - Shadow stack
> I think you should add some mitigation against sigreturn-oriented
> programming. How about creating some special token on the shadow
> stack that indicates the presence of a signal frame at a particular
> address when delivering a signal and verifying and popping that token
> in sigreturn? The token could be literally the address of the signal
> frame, and you could make this unambiguous by failing sigreturn if
> CET
> is on and the signal frame is in executable memory.
>
> IOW, it would be a shame if sigreturn() itself became a convenient
> CET-bypassing gadget.
>
> --Andy
I will look into that.
Thanks,
Yu-cheng
^ permalink raw reply
* Re: [PATCH 00/10] Control Flow Enforcement - Part (3)
From: Andy Lutomirski @ 2018-06-26 5:26 UTC (permalink / raw)
To: Yu-cheng Yu, Linux API, Jann Horn, Florian Weimer
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143807.3611-1-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> This series introduces CET - Shadow stack
I think you should add some mitigation against sigreturn-oriented
programming. How about creating some special token on the shadow
stack that indicates the presence of a signal frame at a particular
address when delivering a signal and verifying and popping that token
in sigreturn? The token could be literally the address of the signal
frame, and you could make this unambiguous by failing sigreturn if CET
is on and the signal frame is in executable memory.
IOW, it would be a shame if sigreturn() itself became a convenient
CET-bypassing gadget.
--Andy
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Andy Lutomirski @ 2018-06-26 2:00 UTC (permalink / raw)
To: Tycho Andersen
Cc: Jann Horn, Kees Cook, kernel list, containers, Linux API,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <20180626013204.GA7261@cisco.cisco.com>
> On Jun 25, 2018, at 6:32 PM, Tycho Andersen <tycho@tycho.ws> wrote:
>
>> On Sat, Jun 23, 2018 at 12:27:43AM +0200, Jann Horn wrote:
>>> On Fri, Jun 22, 2018 at 11:51 PM Kees Cook <keescook@chromium.org> wrote:
>>>
>>>> On Fri, Jun 22, 2018 at 11:09 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> One possible extra issue: IIRC /proc/.../mem uses FOLL_FORCE, which is not what we want here.
>>
>> Uuugh, I forgot about that.
>>
>>>> How about just adding an explicit “read/write the seccomp-trapped task’s memory” primitive? That should be easier than a “open mem fd” primitive.
>>>
>>> Uuugh. Can we avoid adding another "read/write remote process memory"
>>> interface? The point of this series was to provide a lightweight
>>> approach to what should normally be possible via the existing
>>> seccomp+ptrace interface. I do like Jann's context idea, but I agree
>>> with Andy: it can't be a handle to /proc/$pid/mem, since it's
>>> FOLL_FORCE. Is there any other kind of process context id we can use
>>> for this instead of pid? There was once an idea of pid-fd but it never
>>> landed... This would let us get rid of the "id" in the structure too.
>>> And if that existed, we could make process_vm_*v() safer too (taking a
>>> pid-fd instead of a pid).
>>
>> Or make a duplicate of /proc/$pid/mem that only differs in whether it
>> sets FOLL_FORCE? The code is basically already there... something like
>> this:
>
> But we want more than just memory access, I think. rootfs access, ns
> fds, etc. all seem like they might be useful, and racy to open.
>
> I guess I see two options: use the existing id and add something to
> seccomp() to ask if it's still valid or independent of this patchset
> add some kind of pid id :\
>
I think we use the existing id / cookie / whatever and ask seccomp, or new syscalls, to do the requested operation. This is because we know the target task is in a very special stopping point. As a result, a seccomp-specific mechanism can do RCU-less fd modifications against a single-threaded target, can muck with things like struct cred, etc, while a more general interface can’t.
It might be nice to add a syscall with flags such that it could be used on ptrace-stopped targets later on. Something like:
access_remote_task(int fd, u64 id, u32 type, ...)
Where type is 16 bits of “id and fd is from seccomp” and 16 bits of “write memory” or such.
^ permalink raw reply
* Re: [PATCH v4 1/4] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-06-26 1:32 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, Andy Lutomirski, kernel list, containers, Linux API,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro, Tobin C. Harding
In-Reply-To: <CAG48ez3gobTL5mUnZKjhLedotZx49nGrxYKKud5_7+512PaOFw@mail.gmail.com>
On Sat, Jun 23, 2018 at 12:27:43AM +0200, Jann Horn wrote:
> On Fri, Jun 22, 2018 at 11:51 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Jun 22, 2018 at 11:09 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> > > One possible extra issue: IIRC /proc/.../mem uses FOLL_FORCE, which is not what we want here.
>
> Uuugh, I forgot about that.
>
> > > How about just adding an explicit “read/write the seccomp-trapped task’s memory” primitive? That should be easier than a “open mem fd” primitive.
> >
> > Uuugh. Can we avoid adding another "read/write remote process memory"
> > interface? The point of this series was to provide a lightweight
> > approach to what should normally be possible via the existing
> > seccomp+ptrace interface. I do like Jann's context idea, but I agree
> > with Andy: it can't be a handle to /proc/$pid/mem, since it's
> > FOLL_FORCE. Is there any other kind of process context id we can use
> > for this instead of pid? There was once an idea of pid-fd but it never
> > landed... This would let us get rid of the "id" in the structure too.
> > And if that existed, we could make process_vm_*v() safer too (taking a
> > pid-fd instead of a pid).
>
> Or make a duplicate of /proc/$pid/mem that only differs in whether it
> sets FOLL_FORCE? The code is basically already there... something like
> this:
But we want more than just memory access, I think. rootfs access, ns
fds, etc. all seem like they might be useful, and racy to open.
I guess I see two options: use the existing id and add something to
seccomp() to ask if it's still valid or independent of this patchset
add some kind of pid id :\
Tycho
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Eric W. Biederman @ 2018-06-25 16:21 UTC (permalink / raw)
To: Ingo Molnar
Cc: Arnd Bergmann, y2038 Mailman List, Linux Kernel Mailing List,
the arch/x86 maintainers, Linux API, linux-arch, Paul Eggert,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Al Viro,
Dominik Brodowski, Thomas Gleixner, Andrew Morton, linux-alpha,
Deepa Dinamani
In-Reply-To: <20180625091426.GA18351@gmail.com>
Ingo Molnar <mingo@kernel.org> writes:
> * Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> Ingo Molnar <mingo@kernel.org> writes:
>>
>> > * Eric W. Biederman <ebiederm@xmission.com> wrote:
>> >
>> >> The trouble with attributes is that means you can't filter your system
>> >> call arguments with seccomp. [...]
>> >
>> > There's nothing keeping seccomp from securely fetching those arguments and
>> > extending filtering to them as well ...
>> >
>> > Allowing that would make sense for a lot of other system calls as
>> > well.
>>
>> Possibly. The challenge is that if the fetch for the kernel to use
>> those arguments is different from the fetch of seccomp to test those
>> arguments you have a time of test vs time of use race.
>
> Those fetched values should obviously then be used to call permitted
> system calls.
Agreed. To my knowledge no one has figured out how to make that work
yet. For the most part it has been unnecessary.
>> Given the location of the seccomp hook at the kernel user space border
>> there is no easy way for seccomp to share the fetch with the system
>> call itself.
>>
>> So I don't see how seccomp could perform the fetch securely.
>
> Looks like more of a seccomp mis-design/mis-implementation than some fundamental
> problem.
Frankly. Given that there are some very good solutions in other
operating systems, I think the misdesign is in unix/linux not providing
a good answer to what to do when you need more than 6 arguments to a
system call.
> Mis-designed security features should not hinder system call design.
I certainly agree that seccomp should not be the sole reason for not
doing something. However there are lots of reasons to avoid
extensibility in general.
Excess extensibility has been the cause of more than one security issue.
Lots of flexibility comes at the price of lots of conditional execution
which tends to explode the test matrix of possibilities to test, with
the result that some combinations are never thought about or tested
because they don't make sense to combine. Then someone with
mischievious intent see that combination and thinks what happens when I
do this.
Further that conditional execution can frequently be the cause of slow
code as well.
So while there are many nice features of tagged values. I don't think
they are a general solution. The lack of seccomp support (today) is
just one downside among many.
I do think it would be nice to have a general pattern for those
system calls that require extensibility. My gut feel says something
like the L4 pseudo registers (to give a maxium request size)
combined with something like netlink encoding would make a very
nice template for making fast and flexible system calls.
Eric
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Arnd Bergmann @ 2018-06-25 11:42 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-arch, Paul Eggert, Thomas Gleixner, Andrew Morton,
y2038 Mailman List, Linux API, the arch/x86 maintainers,
Linux Kernel Mailing List, Dominik Brodowski, Deepa Dinamani,
Ivan Kokshaysky, Al Viro, linux-alpha, Matt Turner, Ingo Molnar,
Richard Henderson
In-Reply-To: <87a7rm3eb5.fsf@xmission.com>
On Fri, Jun 22, 2018 at 7:45 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Ingo Molnar <mingo@kernel.org> writes:
>
> So I suspect the simplest thing to do would be to set a flag in the
> idtype member of waitid that says give me rusage64 and then we would
> be done.
It would have to be a flag in both the 'idtype' field of waitid(), and also
'who' field of getrusage(), which unfortunately uses a separate set of
flags. Not hard to do, but still a bit more complexity.
> Alternately we could use the low bits of the resource usage
> pointer. Assuming we don't want to introduce another syscall that is.
> I really don't see much incremental extensibility potential in the wait
> or rusage interface right now.
This is also my conclusion after looking at how various other
operating systems implement getrusage() and wait4() today.
It seems that this is one of the most stable APIs, everyone uses
exactly the same structure layout (Linux/x32 being one exception,
they have the 64-bit Linux compatible layout using __s64 instead of
long members).
For the other ~20 system calls we introduce for y2038, the general
idea has been to stay mostly compatible with the source level interface,
just using a new syscall number. statx() is a notable exception here,
with clock_adjtime() and getitimer()/setitimer() still being undecided.
If we don't do an extensible layout or any other new fields, there
are still the open questions about whether any types should change:
- changing everything to 64-bit would allow sharing the kernel
code between compat and native
- changing only __old_kernel_timeval to new 64-bit timeval would
be the simplest user space change (only the syscall number
changes with sizeof(time_t)), avoiding an extra copy thorough
the user space stack.
- changing timeval to (64-bit) timespec would seem the most
sensible update, since it avoids the silly nanosecond-to-
microsecond conversion in the kernel (glibc would still need
to do it for compatibility). This is what I'm considering for
getitimer/setitimer, too.
Arnd
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Ingo Molnar @ 2018-06-25 9:14 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-arch, Paul Eggert, Andrew Morton, Arnd Bergmann,
y2038 Mailman List, Linux API, the arch/x86 maintainers,
Linux Kernel Mailing List, Dominik Brodowski, Deepa Dinamani,
Ivan Kokshaysky, Al Viro, linux-alpha, Matt Turner,
Thomas Gleixner, Richard Henderson
In-Reply-To: <87y3f31wsv.fsf@xmission.com>
* Eric W. Biederman <ebiederm@xmission.com> wrote:
> Ingo Molnar <mingo@kernel.org> writes:
>
> > * Eric W. Biederman <ebiederm@xmission.com> wrote:
> >
> >> The trouble with attributes is that means you can't filter your system
> >> call arguments with seccomp. [...]
> >
> > There's nothing keeping seccomp from securely fetching those arguments and
> > extending filtering to them as well ...
> >
> > Allowing that would make sense for a lot of other system calls as
> > well.
>
> Possibly. The challenge is that if the fetch for the kernel to use
> those arguments is different from the fetch of seccomp to test those
> arguments you have a time of test vs time of use race.
Those fetched values should obviously then be used to call permitted system calls.
> Given the location of the seccomp hook at the kernel user space border
> there is no easy way for seccomp to share the fetch with the system
> call itself.
>
> So I don't see how seccomp could perform the fetch securely.
Looks like more of a seccomp mis-design/mis-implementation than some fundamental
problem.
Mis-designed security features should not hinder system call design.
Thanks,
Ingo
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* Re: [PATCH v2 1/4] lib/rhashtable: simplify bucket_table_alloc()
From: Michal Hocko @ 2018-06-25 9:13 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: akpm, torvalds, tgraf, herbert, manfred, guillaume.knispel,
linux-api, linux-kernel, Davidlohr Bueso
In-Reply-To: <20180622181540.5gul4lx5dteqzzk3@linux-r8p5>
On Fri 22-06-18 11:15:40, Davidlohr Bueso wrote:
> As of ce91f6ee5b3 (mm: kvmalloc does not fallback to vmalloc for incompatible gfp flag),
> we can simplify the caller and trust kvzalloc() to just do the right thing. For the
> case of the GFP_ATOMIC context, we can drop the __GFP_NORETRY flag for obvious reasons,
> and for the __GFP_NOWARN case, however, it is changed such that the caller passes the
> flag instead of making bucket_table_alloc() handle it.
>
> This slightly changes the gfp flags passed on to nested_table_alloc() as it will now
> also use GFP_ATOMIC | __GFP_NOWARN. However, I consider this a positive consequence
> as for the same reasons we want nowarn semantics in bucket_table_alloc().
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>
> v2:
> - Changes based on Neil's concerns about keeping nowarn flag.
> - Better changelog.
>
>
> lib/rhashtable.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index 9427b5766134..083f871491a1 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -175,10 +175,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
> int i;
>
> size = sizeof(*tbl) + nbuckets * sizeof(tbl->buckets[0]);
> - if (gfp != GFP_KERNEL)
> - tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
> - else
> - tbl = kvzalloc(size, gfp);
> + tbl = kvzalloc(size, gfp);
>
> size = nbuckets;
>
> @@ -459,7 +456,7 @@ static int rhashtable_insert_rehash(struct rhashtable *ht,
>
> err = -ENOMEM;
>
> - new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC);
> + new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC | __GFP_NOWARN);
> if (new_tbl == NULL)
> goto fail;
>
> --
> 2.16.4
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH 04/24] 32-bit userspace ABI: introduce ARCH_32BIT_OFF_T config option
From: Yury Norov @ 2018-06-25 6:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:DOCUMENTATION, Szabolcs Nagy, Catalin Marinas,
Heiko Carstens, Philipp Tomsich, Joseph Myers, linux-arch,
Steve Ellcey, Prasun Kapoor, Andreas Schwab, Alexander Graf,
Geert Uytterhoeven, Bamvor Zhangjian, Dave Martin, Adam Borowski,
Manuel Montezelo, James Hogan, Chris Metcalf
In-Reply-To: <20180611112736.GA19952@yury-thinkpad>
On Mon, Jun 11, 2018 at 02:27:36PM +0300, Yury Norov wrote:
> On Mon, Jun 11, 2018 at 09:48:02AM +0200, Arnd Bergmann wrote:
> > On Sat, Jun 9, 2018 at 9:42 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> > > On Fri, Jun 08, 2018 at 06:32:07PM +0100, Catalin Marinas wrote:
> > >> On Wed, May 16, 2018 at 11:18:49AM +0300, Yury Norov wrote:
> > >> > diff --git a/arch/Kconfig b/arch/Kconfig
> > >> > index 76c0b54443b1..ee079244dc3c 100644
> > >> > --- a/arch/Kconfig
> > >> > +++ b/arch/Kconfig
> > >> > @@ -264,6 +264,21 @@ config ARCH_THREAD_STACK_ALLOCATOR
> > >> > config ARCH_WANTS_DYNAMIC_TASK_STRUCT
> > >> > bool
> > >> >
> > >> > +config ARCH_32BIT_OFF_T
> > >> > + bool
> > >> > + depends on !64BIT
> > >> > + help
> > >> > + All new 32-bit architectures should have 64-bit off_t type on
> > >> > + userspace side which corresponds to the loff_t kernel type. This
> > >> > + is the requirement for modern ABIs. Some existing architectures
> > >> > + already have 32-bit off_t. This option is enabled for all such
> > >> > + architectures explicitly. Namely: arc, arm, blackfin, cris, frv,
> > >> > + h8300, hexagon, m32r, m68k, metag, microblaze, mips32, mn10300,
> > >> > + nios2, openrisc, parisc32, powerpc32, score, sh, sparc, tile32,
> > >> > + unicore32, x86_32 and xtensa. This is the complete list. Any
> > >> > + new 32-bit architecture should declare 64-bit off_t type on user
> > >> > + side and so should not enable this option.
> > >>
> > >> Do you know if this is the case for riscv and nds32, merged in the
> > >> meantime? If not, I suggest you drop this patch altogether and just
> > >> define force_o_largefile() for arm64/ilp32 as we don't seem to stick to
> > >> "all new 32-bit architectures should have 64-bit off_t".
> > >
> > > I wrote this patch at request of Arnd Bergmann. This is actually his
> > > words that all new 32-bit architectures should have 64-bit off_t. So
> > > I was surprized when riscv was merged with 32-bit off_t (and I didn't
> > > follow nds32).
> > >
> > > If this rule is still in force, we'd better add new exceptions to this
> > > patch. Otherwise, we can drop it.
> > >
> > > Arnd, could you please comment it?
> >
> > I completely forgot about it and had assumed that it was merged long
> > ago, sorry about that.
>
> Hi Arnd,
>
> There are 3 patches like this in ILP32 series that change ABI for new
> targets. I've submitted them in separated series:
> https://lkml.org/lkml/2017/9/25/574
>
> They all seems to be acked by you. If you ready to upstream the
> series, I can rebase it and add riscv32 and nds32 exceptions.
>
> If Palmer and riscv people will decide to follow new rules, we can
> easily drop the exception.
Ping?
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Eric W. Biederman @ 2018-06-25 1:26 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-arch, Paul Eggert, Andrew Morton, Arnd Bergmann,
y2038 Mailman List, Linux API, the arch/x86 maintainers,
Linux Kernel Mailing List, Dominik Brodowski, Deepa Dinamani,
Ivan Kokshaysky, Al Viro, linux-alpha, Matt Turner,
Thomas Gleixner, Richard Henderson
In-Reply-To: <20180624071258.GB29407@gmail.com>
Ingo Molnar <mingo@kernel.org> writes:
> * Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> The trouble with attributes is that means you can't filter your system
>> call arguments with seccomp. [...]
>
> There's nothing keeping seccomp from securely fetching those arguments and
> extending filtering to them as well ...
>
> Allowing that would make sense for a lot of other system calls as
> well.
Possibly. The challenge is that if the fetch for the kernel to use
those arguments is different from the fetch of seccomp to test those
arguments you have a time of test vs time of use race.
Given the location of the seccomp hook at the kernel user space border
there is no easy way for seccomp to share the fetch with the system
call itself.
So I don't see how seccomp could perform the fetch securely.
Eric
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* Re: [PATCH v2 2/2] rusage: allow 64-bit times ru_utime/ru_stime
From: Ingo Molnar @ 2018-06-24 7:12 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-arch, Paul Eggert, Andrew Morton, Arnd Bergmann,
y2038 Mailman List, Linux API, the arch/x86 maintainers,
Linux Kernel Mailing List, Dominik Brodowski, Deepa Dinamani,
Ivan Kokshaysky, Al Viro, linux-alpha, Matt Turner,
Thomas Gleixner, Richard Henderson
In-Reply-To: <87a7rm3eb5.fsf@xmission.com>
* Eric W. Biederman <ebiederm@xmission.com> wrote:
> The trouble with attributes is that means you can't filter your system
> call arguments with seccomp. [...]
There's nothing keeping seccomp from securely fetching those arguments and
extending filtering to them as well ...
Allowing that would make sense for a lot of other system calls as well.
Thanks,
Ingo
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox