| Message ID | 20250326070255.2567981-1-keirf@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: mops: Do not dereference src reg for a set operation | expand |
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.
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 >
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 > >
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) {
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(-)