Re: [PATCH 2/2] rseq: Kill process when unknown flags are encountered in ABI structures

* Ingo Molnar:

* Mathieu Desnoyers [off-list ref] wrote:

quoted

rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
currently unused.

The current behavior when those bits are set is to ignore them. This is
not an ideal behavior, because when future features will start using
those flags, if user-space fails to correctly validate that the kernel
indeed supports those flags (e.g. with a new sys_rseq flags bit) before
using them, it may incorrectly assume that the kernel will handle those
flags way when in fact those will be silently ignored on older kernels.

Validating that unused flags bits are cleared will allow a smoother
transition when those flags will start to be used by allowing
applications to fail early, and obviously, when they attempt to use the
new flags on an older kernel that does not support them.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 kernel/rseq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 81d7dc80787b..bda8175f8f99 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c

@@ -176,7 +176,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 	u32 flags, event_mask;
 	int ret;
 
-	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
 		return -EINVAL;
 
 	/* Get thread flags. */

@@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
 	if (ret)
 		return ret;
 
-	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
 		return -EINVAL;

Just to make it clear: no existing libraries/tooling out there have learned 
to rely on the old ABI that ignored unset flags, right? Only then is this 
patch ABI-safe.

I believe glibc initializes the flag fields to zero before calling the
rseq system call.  (I don't know if the rseq system call does its own
initialization; maybe it should if it doesn't do so already.)

Thanks,
Florian

----- On Aug 1, 2022, at 10:25 AM, Florian Weimer fweimer@redhat.com wrote:

* Ingo Molnar:

quoted

* Mathieu Desnoyers [off-list ref] wrote:

quoted

rseq_abi()->flags and rseq_abi()->rseq_cs->flags 29 upper bits are
currently unused.

The current behavior when those bits are set is to ignore them. This is
not an ideal behavior, because when future features will start using
those flags, if user-space fails to correctly validate that the kernel
indeed supports those flags (e.g. with a new sys_rseq flags bit) before
using them, it may incorrectly assume that the kernel will handle those
flags way when in fact those will be silently ignored on older kernels.

Validating that unused flags bits are cleared will allow a smoother
transition when those flags will start to be used by allowing
applications to fail early, and obviously, when they attempt to use the
new flags on an older kernel that does not support them.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 kernel/rseq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 81d7dc80787b..bda8175f8f99 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c

@@ -176,7 +176,7 @@ static int rseq_need_restart(struct task_struct *t, u32

cs_flags)
 	u32 flags, event_mask;
 	int ret;
 
-	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(cs_flags & RSEQ_CS_NO_RESTART_FLAGS) || cs_flags)
 		return -EINVAL;
 
 	/* Get thread flags. */

@@ -184,7 +184,7 @@ static int rseq_need_restart(struct task_struct *t, u32

cs_flags)
 	if (ret)
 		return ret;
 
-	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS))
+	if (WARN_ON_ONCE(flags & RSEQ_CS_NO_RESTART_FLAGS) || flags)
 		return -EINVAL;

Just to make it clear: no existing libraries/tooling out there have learned
to rely on the old ABI that ignored unset flags, right? Only then is this
patch ABI-safe.

I believe glibc initializes the flag fields to zero before calling the
rseq system call.  (I don't know if the rseq system call does its own
initialization; maybe it should if it doesn't do so already.)

Initialization and following updates of rseq_abi()->flags and
rseq_abi()->rseq_cs->flags is done by user-space, so the rseq
system call does not initialize any of those fields.

Indeed glibc initialize the rseq_abi()->flags to 0, and does not
use rseq_abi()->rseq_cs->flags as of now.

Thanks,

Mathieu

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