[RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Mathieu Desnoyers]

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.

Validating the value of abort_ip field to be below TASK_SIZE ensures 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 consistently
deal with invalid return addresses.

If validation fails, the process is killed with a segmentation fault.

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             | 5 +++--
 2 files changed, 6 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..2e5d88f09baf 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -128,7 +128,8 @@ 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)
 		return -EFAULT;
 	if (rseq_cs->version > 0)
 		return -EINVAL;
@@ -137,7 +138,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

[RFC PATCH for 4.18 2/2] rseq: validate rseq->rseq_cs padding to be zero

[Mathieu Desnoyers]

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 explicitly ensuring
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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 2e5d88f09baf..c4c48157198f 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,9 @@ 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;
+	ret = check_rseq_cs_padding(t);
+	if (ret)
+		return ret;
 	if (!ptr) {
 		memset(rseq_cs, 0, sizeof(*rseq_cs));
 		return 0;
-- 
2.11.0

Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Linus Torvalds]

On Mon, Jul 2, 2018 at 1:41 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> -       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)
>                 return -EFAULT;

I think the abort_ip check should have the same error value as the
other sanity checks, ie just be of this format:

>         if (rseq_cs->version > 0)
>                 return -EINVAL;

also, I think you should check start_ip to be consistent. You kind of
accidentally do it with the check for

        if (rseq_cs->abort_ip - rseq_cs->start_ip - rseq_cs->post_commit_offset)

but honestly, that has underflow issues already, so I think you want
to basically make the check be

        if (rseq_cs->abort_ip >= TASK_SIZE)
                return -EINVAL;

        if (rseq_cs->start_ip >= rseq_cs->abort_ip)
                return -EINVAL;

which takes care of checkint  start_ip, and also the underflow for the
post_commit_offset check.

If somebody is depending on negative offsets, then that
post_commit_offset logic is already wrong.

> +       usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
>         ret = get_user(sig, usig);

That can underflow too, but I guess we can just rely on get_user()
getting it right.

               Linus

Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Mathieu Desnoyers]

----- On Jul 2, 2018, at 4:52 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Mon, Jul 2, 2018 at 1:41 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> -       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)
>>                 return -EFAULT;
> 
> I think the abort_ip check should have the same error value as the
> other sanity checks, ie just be of this format:
> 
>>         if (rseq_cs->version > 0)
>>                 return -EINVAL;

OK, so I'll go for -EINVAL.

> 
> also, I think you should check start_ip to be consistent. You kind of
> accidentally do it with the check for
> 
>        if (rseq_cs->abort_ip - rseq_cs->start_ip - rseq_cs->post_commit_offset)

The check is actually:

        /* Ensure that abort_ip is not in the critical section. */
        if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
                return -EINVAL;

> 
> but honestly, that has underflow issues already, so I think you want
> to basically make the check be
> 
>        if (rseq_cs->abort_ip >= TASK_SIZE)
>                return -EINVAL;

that works.

> 
>        if (rseq_cs->start_ip >= rseq_cs->abort_ip)
>                return -EINVAL;

this one does not work. We need to ensure that abort_ip
is not between [ start_ip, start_ip + post_commit_offset ]. The
check you propose validates that start_ip is below abort_ip,
which is bogus. For instance, abort_ip can very well be in a
different section of the binary, at an address either below
or above start_ip.


> 
> which takes care of checkint  start_ip, and also the underflow for the
> post_commit_offset check.

What underflow issues are you concerned with ?

> 
> If somebody is depending on negative offsets, then that
> post_commit_offset logic is already wrong.
> 
>> +       usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
>>         ret = get_user(sig, usig);
> 
> That can underflow too, but I guess we can just rely on get_user()
> getting it right.

Yes, get_user() should handle that one properly.

Thanks,

Mathieu

> 
>                Linus

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Linus Torvalds]

On Mon, Jul 2, 2018 at 2:03 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
>         /* Ensure that abort_ip is not in the critical section. */
>         if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
>                 return -EINVAL;
> ...
> What underflow issues are you concerned with ?

That.

Looking closer, it looks like what you want to do is

     if (rseq_cs->abort_ip >= rseq_cs->start_ip && rseq_cs->abort_ip <
rseq_cs->start_ip + rseq_cs->post_commit_offset)

but you're not actually verifying that the range you're testing is
even vlid, because "rseq_cs->start_ip + rseq_cs->post_commit_offset"
could be something invalid that overflowed (or, put another way, the
subtraction you did on both sides to get the simplified version
underflowed).

So to actually get the range check you want, you should check the
overflow/underflow condition. Maybe it ends up being

        if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
                return -EINVAL;

after which your simplified conditional looks fine.

But I think you should also do

        if (rseq_cs->start_ip + rseq_cs->post_commit_offset > TASK_SIZE)
                return -EINVAL;

to make sure the range is valid in the first place.

             Linus

Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Mathieu Desnoyers]

----- On Jul 2, 2018, at 5:20 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Mon, Jul 2, 2018 at 2:03 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>>         /* Ensure that abort_ip is not in the critical section. */
>>         if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
>>                 return -EINVAL;
>> ...
>> What underflow issues are you concerned with ?
> 
> That.
> 
> Looking closer, it looks like what you want to do is
> 
>     if (rseq_cs->abort_ip >= rseq_cs->start_ip && rseq_cs->abort_ip <
> rseq_cs->start_ip + rseq_cs->post_commit_offset)
> 
> but you're not actually verifying that the range you're testing is
> even vlid, because "rseq_cs->start_ip + rseq_cs->post_commit_offset"
> could be something invalid that overflowed (or, put another way, the
> subtraction you did on both sides to get the simplified version
> underflowed).
> 
> So to actually get the range check you want, you should check the
> overflow/underflow condition. Maybe it ends up being
> 
>        if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
>                return -EINVAL;
> 
> after which your simplified conditional looks fine.
> 
> But I think you should also do
> 
>        if (rseq_cs->start_ip + rseq_cs->post_commit_offset > TASK_SIZE)
>                return -EINVAL;
> 
> to make sure the range is valid in the first place.

Taking into account your comments, and adding also an extra check for
rseq_cs->start_ip >= TASK_SIZE, and restricting the end of range
rseq_cs->start_ip + rseq_cs->post_commit_offset to exclude TASK_SIZE
(>= rather than >), the resulting function now looks like this:

static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
        struct rseq_cs __user *urseq_cs;
        unsigned long ptr;
        u32 __user *usig;
        u32 sig;

        if (__get_user(ptr, &t->rseq->rseq_cs))
                return -EINVAL;
        if (check_rseq_cs_padding(t))
                return -EINVAL;
        if (!ptr) {
                memset(rseq_cs, 0, sizeof(*rseq_cs));
                return 0;
        }
        urseq_cs = (struct rseq_cs __user *)ptr;
        if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)) ||
            rseq_cs->start_ip >= TASK_SIZE ||
            rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
            rseq_cs->abort_ip >= TASK_SIZE ||
            rseq_cs->version > 0)
                return -EINVAL;

        /* Check for overflow. */
        if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
                return -EINVAL;
        /* Ensure that abort_ip is not in the critical section. */
        if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
                return -EINVAL;

        usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
        if (get_user(sig, usig))
                return -EINVAL;

        if (current->rseq_sig != sig) {
                printk_ratelimited(KERN_WARNING
                        "Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
                        sig, current->rseq_sig, current->pid, usig);
                return -EINVAL;
        }
        return 0;
}

The end of range exclusion with (rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE)
stems from the reasoning that we need a valid user-space instruction _after_ the range, so
having the range end exactly at the very last byte of TASK_SIZE would require to have a
user-space instruction at TASK_SIZE, which is not valid.

Does it capture your intent ?

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Linus Torvalds]

On Mon, Jul 2, 2018 at 3:03 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
>         if (__get_user(ptr, &t->rseq->rseq_cs))
>                 return -EINVAL;
>         if (check_rseq_cs_padding(t))
>                 return -EINVAL;

Small nit.

I think the _actual_ user access faults should return -EFAULT, and
then the *validation* checks should return -EINVAL.

So when the "copy_from_user()" fails, that's -EFAULT, but when you
have (rseq_cs->start_ip >= TASK_SIZE), that's -EINVAL.

That said, nothing actually cares or exposes the error number, I
think. Afaik, all the callers just check "did it work" or not.

So this is more a "let's be consistent" than anything that matters.

             Linus

Re: [RFC PATCH for 4.18 1/2] rseq: use __u64 for rseq_cs fields, validate abort_ip < TASK_SIZE

[Mathieu Desnoyers]

----- On Jul 2, 2018, at 6:08 PM, Linus Torvalds torvalds@linux-foundation.org wrote:

> On Mon, Jul 2, 2018 at 3:03 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>>         if (__get_user(ptr, &t->rseq->rseq_cs))
>>                 return -EINVAL;
>>         if (check_rseq_cs_padding(t))
>>                 return -EINVAL;
> 
> Small nit.
> 
> I think the _actual_ user access faults should return -EFAULT, and
> then the *validation* checks should return -EINVAL.
> 
> So when the "copy_from_user()" fails, that's -EFAULT, but when you
> have (rseq_cs->start_ip >= TASK_SIZE), that's -EINVAL.

Fair enough.

> 
> That said, nothing actually cares or exposes the error number, I
> think. Afaik, all the callers just check "did it work" or not.

Indeed, it's a static function and callers just check for zero/nonzero.

> 
> So this is more a "let's be consistent" than anything that matters.

Allright, here is the function updated accordingly:

static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
        struct rseq_cs __user *urseq_cs;
        unsigned long ptr;
        u32 __user *usig;
        u32 sig;
        int ret;

        ret = __get_user(ptr, &t->rseq->rseq_cs);
        if (ret)
                return ret;
        if (check_rseq_cs_padding(t))
                return -EINVAL;
        if (!ptr) {
                memset(rseq_cs, 0, sizeof(*rseq_cs));
                return 0;
        }
        urseq_cs = (struct rseq_cs __user *)ptr;
        if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs)))
                return -EFAULT;

        if (rseq_cs->start_ip >= TASK_SIZE ||
            rseq_cs->start_ip + rseq_cs->post_commit_offset >= TASK_SIZE ||
            rseq_cs->abort_ip >= TASK_SIZE ||
            rseq_cs->version > 0)
                return -EINVAL;
        /* Check for overflow. */
        if (rseq_cs->start_ip + rseq_cs->post_commit_offset < rseq_cs->start_ip)
                return -EINVAL;
        /* Ensure that abort_ip is not in the critical section. */
        if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset)
                return -EINVAL;

        usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32));
        ret = get_user(sig, usig);
        if (ret)
                return ret;

        if (current->rseq_sig != sig) {
                printk_ratelimited(KERN_WARNING
                        "Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
                        sig, current->rseq_sig, current->pid, usig);
                return -EINVAL;
        }
        return 0;
}

Thanks for the feedback!

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com