[PATCH v1 0/6] arm64/signal: Signal handling cleanups

[PATCH v1 0/6] arm64/signal: Signal handling cleanups

[Mark Brown]

This series collects a number of small cleanups to the signal handling
code which removes redundant validation of size information and avoids
reading the same data from userspace twice.

There are some overlaps with both the TPIDR2 signal handling and SME2
serieses which are also in flight, applying this will require
adjustments in those serieses and vice versa.

Mark Brown (6):
  arm64/signal: Don't redundantly verify FPSIMD magic
  arm64/signal: Remove redundant size validation from
    parse_user_sigframe()
  arm64/signal: Make interface for restore_fpsimd_context() consistent
  arm64/signal: Avoid rereading context frame sizes
  arm64/signal: Only read new data when parsing the SVE context
  arm64/signal: Only read new data when parsing the ZA context

 arch/arm64/kernel/signal.c | 91 +++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 45 deletions(-)


base-commit: 30a0b95b1335e12efef89dd78518ed3e4a71a763
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/6] arm64/signal: Don't redundantly verify FPSIMD magic

[Mark Brown]

We validate that the magic in the struct fpsimd_context is correct in
restore_fpsimd_context() but this is redundant since parse_user_sigframe()
uses this magic to decide to call the function in the first place. Remove
the extra validation.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/signal.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 9ad911f1647c..f9a98948a2c8 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -189,15 +189,14 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 {
 	struct user_fpsimd_state fpsimd;
-	__u32 magic, size;
+	__u32 size;
 	int err = 0;
 
-	/* check the magic/size information */
-	__get_user_error(magic, &ctx->head.magic, err);
+	/* check the size information */
 	__get_user_error(size, &ctx->head.size, err);
 	if (err)
 		return -EFAULT;
-	if (magic != FPSIMD_MAGIC || size != sizeof(struct fpsimd_context))
+	if (size != sizeof(struct fpsimd_context))
 		return -EINVAL;
 
 	/* copy the FP and status/control registers */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/6] arm64/signal: Remove redundant size validation from parse_user_sigframe()

[Mark Brown]

There is some minimal size validation in parse_user_sigframe() however
all of the individual parsing functions perform frame specific validation
of the sizing information, remove the frame specific size checks in the
core so that there isn't any confusion about what we validate for size.

Since the checks in the SVE and ZA parsing are after we have read the
relevant context and since they won't report an error if the frame is
undersized they are adjusted to check for this before doing anything else.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/signal.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index f9a98948a2c8..1840e26f3f54 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -270,6 +270,9 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	struct user_fpsimd_state fpsimd;
 	struct sve_context sve;
 
+	if (sve.head.size < sizeof(*user->sve))
+		return -EINVAL;
+
 	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
 		return -EFAULT;
 
@@ -288,7 +291,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	if (sve.vl != vl)
 		return -EINVAL;
 
-	if (sve.head.size <= sizeof(*user->sve)) {
+	if (sve.head.size == sizeof(*user->sve)) {
 		clear_thread_flag(TIF_SVE);
 		current->thread.svcr &= ~SVCR_SM_MASK;
 		goto fpsimd_only;
@@ -398,13 +401,16 @@ static int restore_za_context(struct user_ctxs *user)
 	unsigned int vq;
 	struct za_context za;
 
+	if (za.head.size == sizeof(*user->za))
+		return -EINVAL;
+
 	if (__copy_from_user(&za, user->za, sizeof(za)))
 		return -EFAULT;
 
 	if (za.vl != task_get_sme_vl(current))
 		return -EINVAL;
 
-	if (za.head.size <= sizeof(*user->za)) {
+	if (za.head.size == sizeof(*user->za)) {
 		current->thread.svcr &= ~SVCR_ZA_MASK;
 		return 0;
 	}
@@ -507,9 +513,6 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			if (user->fpsimd)
 				goto invalid;
 
-			if (size < sizeof(*user->fpsimd))
-				goto invalid;
-
 			user->fpsimd = (struct fpsimd_context __user *)head;
 			break;
 
@@ -524,9 +527,6 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			if (user->sve)
 				goto invalid;
 
-			if (size < sizeof(*user->sve))
-				goto invalid;
-
 			user->sve = (struct sve_context __user *)head;
 			break;
 
@@ -537,9 +537,6 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			if (user->za)
 				goto invalid;
 
-			if (size < sizeof(*user->za))
-				goto invalid;
-
 			user->za = (struct za_context __user *)head;
 			break;
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/6] arm64/signal: Make interface for restore_fpsimd_context() consistent

[Mark Brown]

Instead of taking a pointer to struct user_ctxs like the other two
restore_blah_context() functions the FPSIMD function takes a pointer to the
user struct it should read. Change it to be consistent with the rest, both
for consistency and to prepare for changes which avoid rereading data that
has already been read by the core parsing code.

There should be no functional change from this patch.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/signal.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 1840e26f3f54..9603a468b104 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -168,6 +168,12 @@ static void __user *apply_user_offset(
 	return base + offset;
 }
 
+struct user_ctxs {
+	struct fpsimd_context __user *fpsimd;
+	struct sve_context __user *sve;
+	struct za_context __user *za;
+};
+
 static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 {
 	struct user_fpsimd_state const *fpsimd =
@@ -186,24 +192,24 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 	return err ? -EFAULT : 0;
 }
 
-static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
+static int restore_fpsimd_context(struct user_ctxs *user)
 {
 	struct user_fpsimd_state fpsimd;
 	__u32 size;
 	int err = 0;
 
 	/* check the size information */
-	__get_user_error(size, &ctx->head.size, err);
+	__get_user_error(size, &user->fpsimd->head.size, err);
 	if (err)
 		return -EFAULT;
 	if (size != sizeof(struct fpsimd_context))
 		return -EINVAL;
 
 	/* copy the FP and status/control registers */
-	err = __copy_from_user(fpsimd.vregs, ctx->vregs,
+	err = __copy_from_user(fpsimd.vregs, &(user->fpsimd->vregs),
 			       sizeof(fpsimd.vregs));
-	__get_user_error(fpsimd.fpsr, &ctx->fpsr, err);
-	__get_user_error(fpsimd.fpcr, &ctx->fpcr, err);
+	__get_user_error(fpsimd.fpsr, &(user->fpsimd->fpsr), err);
+	__get_user_error(fpsimd.fpcr, &(user->fpsimd->fpcr), err);
 
 	clear_thread_flag(TIF_SVE);
 
@@ -215,12 +221,6 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 }
 
 
-struct user_ctxs {
-	struct fpsimd_context __user *fpsimd;
-	struct sve_context __user *sve;
-	struct za_context __user *za;
-};
-
 #ifdef CONFIG_ARM64_SVE
 
 static int preserve_sve_context(struct sve_context __user *ctx)
@@ -656,7 +656,7 @@ static int restore_sigframe(struct pt_regs *regs,
 		if (user.sve)
 			err = restore_sve_fpsimd_context(&user);
 		else
-			err = restore_fpsimd_context(user.fpsimd);
+			err = restore_fpsimd_context(&user);
 	}
 
 	if (err == 0 && system_supports_sme() && user.za)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/6] arm64/signal: Avoid rereading context frame sizes

[Mark Brown]

We need to read the sizes of the signal context frames as part of parsing
the overall signal context in parse_user_sigframe(). In the cases where we
defer frame specific parsing to other functions those functions always
reread the size and validate the version they read, opening the possibility
that the value may change. Avoid this possibility by passing the size read
in parse_user_sigframe() through user_ctxs and referring to that.

Note that for SVE and ZA contexts we still read the size again but after
this change we no longer use the value, further changes will avoid the
read.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/signal.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 9603a468b104..3a11b87eec4d 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -170,8 +170,11 @@ static void __user *apply_user_offset(
 
 struct user_ctxs {
 	struct fpsimd_context __user *fpsimd;
+	u32 fpsimd_size;
 	struct sve_context __user *sve;
+	u32 sve_size;
 	struct za_context __user *za;
+	u32 za_size;
 };
 
 static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
@@ -195,14 +198,10 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 static int restore_fpsimd_context(struct user_ctxs *user)
 {
 	struct user_fpsimd_state fpsimd;
-	__u32 size;
 	int err = 0;
 
 	/* check the size information */
-	__get_user_error(size, &user->fpsimd->head.size, err);
-	if (err)
-		return -EFAULT;
-	if (size != sizeof(struct fpsimd_context))
+	if (user->fpsimd_size != sizeof(struct fpsimd_context))
 		return -EINVAL;
 
 	/* copy the FP and status/control registers */
@@ -270,7 +269,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	struct user_fpsimd_state fpsimd;
 	struct sve_context sve;
 
-	if (sve.head.size < sizeof(*user->sve))
+	if (user->sve_size < sizeof(*user->sve))
 		return -EINVAL;
 
 	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
@@ -291,7 +290,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	if (sve.vl != vl)
 		return -EINVAL;
 
-	if (sve.head.size == sizeof(*user->sve)) {
+	if (user->sve_size == sizeof(*user->sve)) {
 		clear_thread_flag(TIF_SVE);
 		current->thread.svcr &= ~SVCR_SM_MASK;
 		goto fpsimd_only;
@@ -299,7 +298,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 
 	vq = sve_vq_from_vl(sve.vl);
 
-	if (sve.head.size < SVE_SIG_CONTEXT_SIZE(vq))
+	if (user->sve_size < SVE_SIG_CONTEXT_SIZE(vq))
 		return -EINVAL;
 
 	/*
@@ -401,7 +400,7 @@ static int restore_za_context(struct user_ctxs *user)
 	unsigned int vq;
 	struct za_context za;
 
-	if (za.head.size == sizeof(*user->za))
+	if (user->za_size < sizeof(*user->za))
 		return -EINVAL;
 
 	if (__copy_from_user(&za, user->za, sizeof(za)))
@@ -410,14 +409,14 @@ static int restore_za_context(struct user_ctxs *user)
 	if (za.vl != task_get_sme_vl(current))
 		return -EINVAL;
 
-	if (za.head.size == sizeof(*user->za)) {
+	if (user->za_size == sizeof(*user->za)) {
 		current->thread.svcr &= ~SVCR_ZA_MASK;
 		return 0;
 	}
 
 	vq = sve_vq_from_vl(za.vl);
 
-	if (za.head.size < ZA_SIG_CONTEXT_SIZE(vq))
+	if (user->za_size < ZA_SIG_CONTEXT_SIZE(vq))
 		return -EINVAL;
 
 	/*
@@ -514,6 +513,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 				goto invalid;
 
 			user->fpsimd = (struct fpsimd_context __user *)head;
+			user->fpsimd_size = size;
 			break;
 
 		case ESR_MAGIC:
@@ -528,6 +528,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 				goto invalid;
 
 			user->sve = (struct sve_context __user *)head;
+			user->sve_size = size;
 			break;
 
 		case ZA_MAGIC:
@@ -538,6 +539,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 				goto invalid;
 
 			user->za = (struct za_context __user *)head;
+			user->za_size = size;
 			break;
 
 		case EXTRA_MAGIC:
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/6] arm64/signal: Only read new data when parsing the SVE context

[Mark Brown]

When we parse the SVE signal context we read the entire context from
userspace, including the generic signal context header which was already
read by parse_user_sigframe() and padding bytes that we ignore. Avoid the
possibility of relying on the second read of the data read twice by only
reading the data which we are actually going to use.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/signal.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 3a11b87eec4d..f7b24213818a 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -264,18 +264,20 @@ static int preserve_sve_context(struct sve_context __user *ctx)
 
 static int restore_sve_fpsimd_context(struct user_ctxs *user)
 {
-	int err;
+	int err = 0;
 	unsigned int vl, vq;
 	struct user_fpsimd_state fpsimd;
-	struct sve_context sve;
+	u16 user_vl, flags;
 
 	if (user->sve_size < sizeof(*user->sve))
 		return -EINVAL;
 
-	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
-		return -EFAULT;
+	__get_user_error(user_vl, &(user->sve->vl), err);
+	__get_user_error(flags, &(user->sve->flags), err);
+	if (err)
+		return err;
 
-	if (sve.flags & SVE_SIG_FLAG_SM) {
+	if (flags & SVE_SIG_FLAG_SM) {
 		if (!system_supports_sme())
 			return -EINVAL;
 
@@ -287,7 +289,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 		vl = task_get_sve_vl(current);
 	}
 
-	if (sve.vl != vl)
+	if (user_vl != vl)
 		return -EINVAL;
 
 	if (user->sve_size == sizeof(*user->sve)) {
@@ -296,7 +298,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 		goto fpsimd_only;
 	}
 
-	vq = sve_vq_from_vl(sve.vl);
+	vq = sve_vq_from_vl(vl);
 
 	if (user->sve_size < SVE_SIG_CONTEXT_SIZE(vq))
 		return -EINVAL;
@@ -324,7 +326,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
 	if (err)
 		return -EFAULT;
 
-	if (sve.flags & SVE_SIG_FLAG_SM)
+	if (flags & SVE_SIG_FLAG_SM)
 		current->thread.svcr |= SVCR_SM_MASK;
 	else
 		set_thread_flag(TIF_SVE);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 6/6] arm64/signal: Only read new data when parsing the ZA context

[Mark Brown]

When we parse the ZA signal context we read the entire context from
userspace, including the generic signal context header which was already
read by parse_user_sigframe() and padding bytes that we ignore. Avoid the
possibility of relying on the second read of the data read twice by only
reading the data which we are actually going to use.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/signal.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index f7b24213818a..4faaa5c6a249 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -398,17 +398,18 @@ static int preserve_za_context(struct za_context __user *ctx)
 
 static int restore_za_context(struct user_ctxs *user)
 {
-	int err;
+	int err = 0;
 	unsigned int vq;
-	struct za_context za;
+	u16 user_vl;
 
 	if (user->za_size < sizeof(*user->za))
 		return -EINVAL;
 
-	if (__copy_from_user(&za, user->za, sizeof(za)))
-		return -EFAULT;
+	__get_user_error(user_vl, &(user->za->vl), err);
+	if (err)
+		return err;
 
-	if (za.vl != task_get_sme_vl(current))
+	if (user_vl != task_get_sme_vl(current))
 		return -EINVAL;
 
 	if (user->za_size == sizeof(*user->za)) {
@@ -416,7 +417,7 @@ static int restore_za_context(struct user_ctxs *user)
 		return 0;
 	}
 
-	vq = sve_vq_from_vl(za.vl);
+	vq = sve_vq_from_vl(user_vl);
 
 	if (user->za_size < ZA_SIG_CONTEXT_SIZE(vq))
 		return -EINVAL;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel