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" <redacted>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <redacted>
CC: Thomas Gleixner <redacted>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <redacted>
CC: Dave Watson <redacted>
CC: Chris Lameter <redacted>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <redacted>
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 <redacted>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <redacted>
CC: Michael Kerrisk <redacted>
CC: Boqun Feng <redacted>
CC: linux-api@vger.kernel.org
---
include/uapi/linux/rseq.h | 6 +++---
kernel/rseq.c | 5 +++--
2 files changed, 6 insertions(+), 5 deletions(-)
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" <redacted>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <redacted>
CC: Thomas Gleixner <redacted>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <redacted>
CC: Dave Watson <redacted>
CC: Chris Lameter <redacted>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <redacted>
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 <redacted>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <redacted>
CC: Michael Kerrisk <redacted>
CC: Boqun Feng <redacted>
CC: linux-api@vger.kernel.org
---
kernel/rseq.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
On Mon, Jul 2, 2018 at 1:41 PM Mathieu Desnoyers
[off-list ref] 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.
----- 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
[off-list ref] wrote:
quoted
- 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:
quoted
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.
On Mon, Jul 2, 2018 at 2:03 PM Mathieu Desnoyers
[off-list ref] 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
----- 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
[off-list ref] wrote:
quoted
/* 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
On Mon, Jul 2, 2018 at 3:03 PM Mathieu Desnoyers
[off-list ref] 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
----- 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
[off-list ref] wrote:
quoted
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