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.
v2:
- Rebase onto v6.2-rc1
To: Catalin Marinas <[email protected]>
To: Will Deacon <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Mark Brown <[email protected]>
---
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: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20221212-arm64-signal-cleanup-bcd7272de5a9
Best regards,
--
Mark Brown <[email protected]>
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 <[email protected]>
---
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 82a89b0852ee..26192ab56de4 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -265,18 +265,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;
@@ -288,7 +290,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)) {
@@ -298,7 +300,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;
@@ -326,7 +328,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
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 <[email protected]>
---
arch/arm64/kernel/signal.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e9c6ffc1ebba..82a89b0852ee 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 */
@@ -271,12 +270,12 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
struct user_fpsimd_state fpsimd;
struct sve_context sve;
+ if (user->sve_size < sizeof(*user->sve))
+ return -EINVAL;
+
if (__copy_from_user(&sve, user->sve, sizeof(sve)))
return -EFAULT;
- if (sve.head.size < sizeof(*user->sve))
- return -EINVAL;
-
if (sve.flags & SVE_SIG_FLAG_SM) {
if (!system_supports_sme())
return -EINVAL;
@@ -292,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 (user->sve_size == sizeof(*user->sve)) {
clear_thread_flag(TIF_SVE);
current->thread.svcr &= ~SVCR_SM_MASK;
current->thread.fp_type = FP_STATE_FPSIMD;
@@ -301,7 +300,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;
/*
@@ -404,23 +403,23 @@ static int restore_za_context(struct user_ctxs *user)
unsigned int vq;
struct za_context za;
+ if (user->za_size < sizeof(*user->za))
+ return -EINVAL;
+
if (__copy_from_user(&za, user->za, sizeof(za)))
return -EFAULT;
- if (za.head.size < sizeof(*user->za))
- return -EINVAL;
-
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;
/*
@@ -517,6 +516,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:
@@ -531,6 +531,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:
@@ -541,6 +542,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
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 <[email protected]>
---
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 26192ab56de4..bed27d4f8ce9 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -401,17 +401,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)) {
@@ -419,7 +420,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
Hi Mark,
On Tue, Jan 03, 2023 at 08:25:15PM +0000, Mark Brown wrote:
> 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
I'm fine with the first three patches, they seem correct and make the
frame checking more 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
I'm not sure these add much to the code readability (and the performance
improvement I guess is negligible). We avoid some copy_from_user() into
the context structures but rely on data read previously or some
get_user() into local variables. Personally I'd make the
restore_fpsimd_context() also do a copy_from_user() for consistency with
the current sve and za frames restoring.
Personal preference, not sure whether Will has the same view.
--
Catalin
On Tue, Jan 31, 2023 at 12:51:33PM +0000, Catalin Marinas wrote:
> On Tue, Jan 03, 2023 at 08:25:15PM +0000, Mark Brown wrote:
> > 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
> I'm not sure these add much to the code readability (and the performance
> improvement I guess is negligible). We avoid some copy_from_user() into
> the context structures but rely on data read previously or some
> get_user() into local variables. Personally I'd make the
> restore_fpsimd_context() also do a copy_from_user() for consistency with
> the current sve and za frames restoring.
> Personal preference, not sure whether Will has the same view.
I don't particularly care about those changs either, Will seemed to be
asking for something like that.
Note that I should at some point today send a version of this series
rebased on for-next/core due to the TPIDR2 and SME2 changes.
On Tue, Jan 31, 2023 at 12:51:33PM +0000, Catalin Marinas wrote:
> Hi Mark,
>
> On Tue, Jan 03, 2023 at 08:25:15PM +0000, Mark Brown wrote:
> > 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
>
> I'm fine with the first three patches, they seem correct and make the
> frame checking more 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
>
> I'm not sure these add much to the code readability (and the performance
> improvement I guess is negligible). We avoid some copy_from_user() into
> the context structures but rely on data read previously or some
> get_user() into local variables. Personally I'd make the
> restore_fpsimd_context() also do a copy_from_user() for consistency with
> the current sve and za frames restoring.
>
> Personal preference, not sure whether Will has the same view.
That main thing I'm worried about is the potential for ToCToU bugs if we
read userdata more than once. For example, if we end up splitting checks
between the two reads, then an attacker could update the value in the
middle and potential cause us issues. If we just read the stuff once, we
don't have to worry about that.
Will
On Tue, Jan 31, 2023 at 03:38:16PM +0000, Will Deacon wrote:
> On Tue, Jan 31, 2023 at 12:51:33PM +0000, Catalin Marinas wrote:
> > Hi Mark,
> >
> > On Tue, Jan 03, 2023 at 08:25:15PM +0000, Mark Brown wrote:
> > > 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
> >
> > I'm fine with the first three patches, they seem correct and make the
> > frame checking more 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
> >
> > I'm not sure these add much to the code readability (and the performance
> > improvement I guess is negligible). We avoid some copy_from_user() into
> > the context structures but rely on data read previously or some
> > get_user() into local variables. Personally I'd make the
> > restore_fpsimd_context() also do a copy_from_user() for consistency with
> > the current sve and za frames restoring.
> >
> > Personal preference, not sure whether Will has the same view.
>
> That main thing I'm worried about is the potential for ToCToU bugs if we
> read userdata more than once. For example, if we end up splitting checks
> between the two reads, then an attacker could update the value in the
> middle and potential cause us issues. If we just read the stuff once, we
> don't have to worry about that.
I thought this may be the case but I couldn't come up with an exploit.
The actual size validation is done after the second read. The first read
of the size is used to walk the frame and check the magic values.
Anyway, you are probably right, it's easier to reason about ToCToU if we
only read the size once.
--
Catalin