----- On Jun 28, 2018, at 12:53 PM, Will Deacon [email protected] wrote: > 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 <[email protected]> >> CC: "Paul E. McKenney" <[email protected]> >> CC: Peter Zijlstra <[email protected]> >> CC: Paul Turner <[email protected]> >> CC: Thomas Gleixner <[email protected]> >> CC: Andy Lutomirski <[email protected]> >> CC: Andi Kleen <[email protected]> >> CC: Dave Watson <[email protected]> >> CC: Chris Lameter <[email protected]> >> CC: Ingo Molnar <[email protected]> >> CC: "H. Peter Anvin" <[email protected]> >> CC: Ben Maurer <[email protected]> >> CC: Steven Rostedt <[email protected]> >> CC: Josh Triplett <[email protected]> >> CC: Linus Torvalds <[email protected]> >> CC: Andrew Morton <[email protected]> >> CC: Russell King <[email protected]> >> CC: Catalin Marinas <[email protected]> >> CC: Will Deacon <[email protected]> >> CC: Michael Kerrisk <[email protected]> >> CC: Boqun Feng <[email protected]> >> CC: [email protected] >> --- >> 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?
With a 64-bit kernel, we end up in the #else, which means check_rseq_cs_padding() always returns 0. On that 64-bit kernel, all 64 bits of rseq->rseq_cs are read, including the padding. Therefore, all those bits are contained in the pointer passed as argument to copy_from_user(), which will cause copy_from_user() to accurately fail on an invalid user-space address. Therefore, 64-bit kernels already check those padding bits by means of trying to use that pointer to access user-space data with copy_from_user, which does an access_ok check. So both 32-bit and 64-bit kernels will end up killing the process with segmentation fault if a 32-bit userland populates those padding bits with anything other than 0. Does it seem acceptable ? Thanks, Mathieu > > Will -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
