* [PATCH] arm64: mops: Do not dereference src reg for a set operation
@ 2025-03-26 7:02 Keir Fraser
2025-03-26 9:39 ` Marc Zyngier
2025-03-26 10:26 ` Mark Rutland
0 siblings, 2 replies; 6+ messages in thread
From: Keir Fraser @ 2025-03-26 7:02 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Keir Fraser, Kristina Martsenko, Catalin Marinas, Mark Rutland,
Will Deacon, Marc Zyngier, stable
The register is not defined and reading it can result in a UBSAN
out-of-bounds array access error, specifically when the srcreg field
value is 31.
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Keir Fraser <keirf@google.com>
---
arch/arm64/include/asm/traps.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index d780d1bd2eac..82cf1f879c61 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -109,10 +109,9 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
int dstreg = ESR_ELx_MOPS_ISS_DESTREG(esr);
int srcreg = ESR_ELx_MOPS_ISS_SRCREG(esr);
int sizereg = ESR_ELx_MOPS_ISS_SIZEREG(esr);
- unsigned long dst, src, size;
+ unsigned long dst, size;
dst = regs->regs[dstreg];
- src = regs->regs[srcreg];
size = regs->regs[sizereg];
/*
@@ -129,6 +128,7 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
}
} else {
/* CPY* instruction */
+ unsigned long src = regs->regs[srcreg];
if (!(option_a ^ wrong_option)) {
/* Format is from Option B */
if (regs->pstate & PSR_N_BIT) {
--
2.49.0.395.g12beb8f557-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] arm64: mops: Do not dereference src reg for a set operation
2025-03-26 7:02 [PATCH] arm64: mops: Do not dereference src reg for a set operation Keir Fraser
@ 2025-03-26 9:39 ` Marc Zyngier
2025-03-26 10:26 ` Mark Rutland
1 sibling, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2025-03-26 9:39 UTC (permalink / raw)
To: Keir Fraser
Cc: linux-arm-kernel, linux-kernel, Kristina Martsenko,
Catalin Marinas, Mark Rutland, Will Deacon, stable
On Wed, 26 Mar 2025 07:02:55 +0000,
Keir Fraser <keirf@google.com> wrote:
>
> The register is not defined and reading it can result in a UBSAN
> out-of-bounds array access error, specifically when the srcreg field
> value is 31.
Gah, XZR/SP encoding strikes back...
>
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Keir Fraser <keirf@google.com>
> ---
> arch/arm64/include/asm/traps.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> index d780d1bd2eac..82cf1f879c61 100644
> --- a/arch/arm64/include/asm/traps.h
> +++ b/arch/arm64/include/asm/traps.h
> @@ -109,10 +109,9 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> int dstreg = ESR_ELx_MOPS_ISS_DESTREG(esr);
> int srcreg = ESR_ELx_MOPS_ISS_SRCREG(esr);
> int sizereg = ESR_ELx_MOPS_ISS_SIZEREG(esr);
> - unsigned long dst, src, size;
> + unsigned long dst, size;
>
> dst = regs->regs[dstreg];
> - src = regs->regs[srcreg];
> size = regs->regs[sizereg];
>
> /*
> @@ -129,6 +128,7 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> }
> } else {
> /* CPY* instruction */
> + unsigned long src = regs->regs[srcreg];
> if (!(option_a ^ wrong_option)) {
> /* Format is from Option B */
> if (regs->pstate & PSR_N_BIT) {
Reviewed-by: Marc Zyngier <maz@kernel.org>
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] arm64: mops: Do not dereference src reg for a set operation
2025-03-26 7:02 [PATCH] arm64: mops: Do not dereference src reg for a set operation Keir Fraser
2025-03-26 9:39 ` Marc Zyngier
@ 2025-03-26 10:26 ` Mark Rutland
2025-03-26 10:45 ` Keir Fraser
1 sibling, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2025-03-26 10:26 UTC (permalink / raw)
To: Keir Fraser
Cc: linux-arm-kernel, linux-kernel, Kristina Martsenko,
Catalin Marinas, Will Deacon, Marc Zyngier, stable
On Wed, Mar 26, 2025 at 07:02:55AM +0000, Keir Fraser wrote:
> The register is not defined and reading it can result in a UBSAN
> out-of-bounds array access error, specifically when the srcreg field
> value is 31.
I'm assuming this is for a MOPS exception taken from a SET* sequence
with XZR as the source?
It'd be nice to say that explicitly, as this is the only case where any
of the src/dst/size fields in the ESR can be reported as 31. In all
other cases where a CPY* or SET* instruction takes register 31 as an
argument, the behaviour is CONSTRAINED UNPREDICTABLE and cannot generate
a MOPS exception.
Note that in ARM DDI 0487 L.a there's a bug where:
* The prose says that SET* taking XZR as a src is CONSTRAINED
UNPREDICTABLE, per K1.2.17.1.1 linked from C6.2.332.
The title for the K1.2.17.1.1 is "Memory Copy and Memory Set CPY*",
which looks like an editing error.
* The pseudocode is explicit that the CONSTRAINED UNPREDICTABLE
behaviours differ for CPY* and SET* per J1.1.3.121
CheckCPYConstrainedUnpredictable() and J1.1.3.125
CheckSETConstrainedUnpredictable().
... and I'll go file a ticket about that soon if someone doesn't beat me
to it.
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
Looks like this should have:
Fixes: 2de451a329cf662b ("KVM: arm64: Add handler for MOPS exceptions")
Prior to that, the code in do_el0_mops() was benign as the use of
pt_regs_read_reg() prevented the out-of-bounds access. It'd also be nice
to note that in the commit message.
Mark.
> Signed-off-by: Keir Fraser <keirf@google.com>
> ---
> arch/arm64/include/asm/traps.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> index d780d1bd2eac..82cf1f879c61 100644
> --- a/arch/arm64/include/asm/traps.h
> +++ b/arch/arm64/include/asm/traps.h
> @@ -109,10 +109,9 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> int dstreg = ESR_ELx_MOPS_ISS_DESTREG(esr);
> int srcreg = ESR_ELx_MOPS_ISS_SRCREG(esr);
> int sizereg = ESR_ELx_MOPS_ISS_SIZEREG(esr);
> - unsigned long dst, src, size;
> + unsigned long dst, size;
>
> dst = regs->regs[dstreg];
> - src = regs->regs[srcreg];
> size = regs->regs[sizereg];
>
> /*
> @@ -129,6 +128,7 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> }
> } else {
> /* CPY* instruction */
> + unsigned long src = regs->regs[srcreg];
> if (!(option_a ^ wrong_option)) {
> /* Format is from Option B */
> if (regs->pstate & PSR_N_BIT) {
> --
> 2.49.0.395.g12beb8f557-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] arm64: mops: Do not dereference src reg for a set operation
2025-03-26 10:26 ` Mark Rutland
@ 2025-03-26 10:45 ` Keir Fraser
0 siblings, 0 replies; 6+ messages in thread
From: Keir Fraser @ 2025-03-26 10:45 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, linux-kernel, Kristina Martsenko,
Catalin Marinas, Will Deacon, Marc Zyngier, stable
On Wed, Mar 26, 2025 at 10:26:47AM +0000, Mark Rutland wrote:
> On Wed, Mar 26, 2025 at 07:02:55AM +0000, Keir Fraser wrote:
> > The register is not defined and reading it can result in a UBSAN
> > out-of-bounds array access error, specifically when the srcreg field
> > value is 31.
>
> I'm assuming this is for a MOPS exception taken from a SET* sequence
> with XZR as the source?
Yes.
> It'd be nice to say that explicitly, as this is the only case where any
> of the src/dst/size fields in the ESR can be reported as 31. In all
> other cases where a CPY* or SET* instruction takes register 31 as an
> argument, the behaviour is CONSTRAINED UNPREDICTABLE and cannot generate
> a MOPS exception.
Okay, will do.
> Note that in ARM DDI 0487 L.a there's a bug where:
>
> * The prose says that SET* taking XZR as a src is CONSTRAINED
> UNPREDICTABLE, per K1.2.17.1.1 linked from C6.2.332.
>
> The title for the K1.2.17.1.1 is "Memory Copy and Memory Set CPY*",
> which looks like an editing error.
>
> * The pseudocode is explicit that the CONSTRAINED UNPREDICTABLE
> behaviours differ for CPY* and SET* per J1.1.3.121
> CheckCPYConstrainedUnpredictable() and J1.1.3.125
> CheckSETConstrainedUnpredictable().
>
> ... and I'll go file a ticket about that soon if someone doesn't beat me
> to it.
>
> > Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: stable@vger.kernel.org
>
> Looks like this should have:
>
> Fixes: 2de451a329cf662b ("KVM: arm64: Add handler for MOPS exceptions")
>
> Prior to that, the code in do_el0_mops() was benign as the use of
> pt_regs_read_reg() prevented the out-of-bounds access. It'd also be nice
> to note that in the commit message.
I will add this too. And Marc's reviewed-by. And re-send as v2. Thanks!
Keir
> Mark.
>
> > Signed-off-by: Keir Fraser <keirf@google.com>
> > ---
> > arch/arm64/include/asm/traps.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> > index d780d1bd2eac..82cf1f879c61 100644
> > --- a/arch/arm64/include/asm/traps.h
> > +++ b/arch/arm64/include/asm/traps.h
> > @@ -109,10 +109,9 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> > int dstreg = ESR_ELx_MOPS_ISS_DESTREG(esr);
> > int srcreg = ESR_ELx_MOPS_ISS_SRCREG(esr);
> > int sizereg = ESR_ELx_MOPS_ISS_SIZEREG(esr);
> > - unsigned long dst, src, size;
> > + unsigned long dst, size;
> >
> > dst = regs->regs[dstreg];
> > - src = regs->regs[srcreg];
> > size = regs->regs[sizereg];
> >
> > /*
> > @@ -129,6 +128,7 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> > }
> > } else {
> > /* CPY* instruction */
> > + unsigned long src = regs->regs[srcreg];
> > if (!(option_a ^ wrong_option)) {
> > /* Format is from Option B */
> > if (regs->pstate & PSR_N_BIT) {
> > --
> > 2.49.0.395.g12beb8f557-goog
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64: mops: Do not dereference src reg for a set operation
@ 2025-03-26 11:00 Keir Fraser
2025-03-26 11:05 ` Mark Rutland
0 siblings, 1 reply; 6+ messages in thread
From: Keir Fraser @ 2025-03-26 11:00 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Keir Fraser, Kristina Martsenko, Catalin Marinas, Mark Rutland,
Will Deacon, stable, Marc Zyngier
The source register is not used for SET* and reading it can result in
a UBSAN out-of-bounds array access error, specifically when the MOPS
exception is taken from a SET* sequence with XZR (reg 31) as the
source. Architecturally this is the only case where a src/dst/size
field in the ESR can be reported as 31.
Prior to 2de451a329cf662b the code in do_el0_mops() was benign as the
use of pt_regs_read_reg() prevented the out-of-bounds access.
Fixes: 2de451a329cf662b ("KVM: arm64: Add handler for MOPS exceptions")
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Keir Fraser <keirf@google.com>
---
arch/arm64/include/asm/traps.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index d780d1bd2eac..82cf1f879c61 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -109,10 +109,9 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
int dstreg = ESR_ELx_MOPS_ISS_DESTREG(esr);
int srcreg = ESR_ELx_MOPS_ISS_SRCREG(esr);
int sizereg = ESR_ELx_MOPS_ISS_SIZEREG(esr);
- unsigned long dst, src, size;
+ unsigned long dst, size;
dst = regs->regs[dstreg];
- src = regs->regs[srcreg];
size = regs->regs[sizereg];
/*
@@ -129,6 +128,7 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
}
} else {
/* CPY* instruction */
+ unsigned long src = regs->regs[srcreg];
if (!(option_a ^ wrong_option)) {
/* Format is from Option B */
if (regs->pstate & PSR_N_BIT) {
--
2.49.0.395.g12beb8f557-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] arm64: mops: Do not dereference src reg for a set operation
2025-03-26 11:00 Keir Fraser
@ 2025-03-26 11:05 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2025-03-26 11:05 UTC (permalink / raw)
To: Keir Fraser
Cc: linux-arm-kernel, linux-kernel, Kristina Martsenko,
Catalin Marinas, Will Deacon, stable, Marc Zyngier
On Wed, Mar 26, 2025 at 11:00:58AM +0000, Keir Fraser wrote:
> The source register is not used for SET* and reading it can result in
> a UBSAN out-of-bounds array access error, specifically when the MOPS
> exception is taken from a SET* sequence with XZR (reg 31) as the
> source. Architecturally this is the only case where a src/dst/size
> field in the ESR can be reported as 31.
>
> Prior to 2de451a329cf662b the code in do_el0_mops() was benign as the
> use of pt_regs_read_reg() prevented the out-of-bounds access.
>
> Fixes: 2de451a329cf662b ("KVM: arm64: Add handler for MOPS exceptions")
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Keir Fraser <keirf@google.com>
Thanks!
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/traps.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> index d780d1bd2eac..82cf1f879c61 100644
> --- a/arch/arm64/include/asm/traps.h
> +++ b/arch/arm64/include/asm/traps.h
> @@ -109,10 +109,9 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> int dstreg = ESR_ELx_MOPS_ISS_DESTREG(esr);
> int srcreg = ESR_ELx_MOPS_ISS_SRCREG(esr);
> int sizereg = ESR_ELx_MOPS_ISS_SIZEREG(esr);
> - unsigned long dst, src, size;
> + unsigned long dst, size;
>
> dst = regs->regs[dstreg];
> - src = regs->regs[srcreg];
> size = regs->regs[sizereg];
>
> /*
> @@ -129,6 +128,7 @@ static inline void arm64_mops_reset_regs(struct user_pt_regs *regs, unsigned lon
> }
> } else {
> /* CPY* instruction */
> + unsigned long src = regs->regs[srcreg];
> if (!(option_a ^ wrong_option)) {
> /* Format is from Option B */
> if (regs->pstate & PSR_N_BIT) {
> --
> 2.49.0.395.g12beb8f557-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-26 11:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-26 7:02 [PATCH] arm64: mops: Do not dereference src reg for a set operation Keir Fraser
2025-03-26 9:39 ` Marc Zyngier
2025-03-26 10:26 ` Mark Rutland
2025-03-26 10:45 ` Keir Fraser
-- strict thread matches above, loose matches on Subject: below --
2025-03-26 11:00 Keir Fraser
2025-03-26 11:05 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).