[PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Nicolin Chen]

CONFIG_COMPAT allows ARM64 machine to run 32-bit instructions.
Since the ARCH_TIMER_USR_VCT_ACCESS_EN might be disabled if a
timer workaround is detected, accessing cntvct via mrrc will
also trigger a trap.

So this patch adds support to handle this situation.

Tested with a user program generated by 32-bit compiler:
  int main()
  {
      unsigned long long cval;
      asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
      return 0;
  }

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

[ I also added cntfrq here for safety as theoretically it could
  trigger the trap as well. However, my another test case (with
  mrc insturction) doesn't seem to trigger a trap. So I would
  drop it in the next version if someone can confirm it's not
  required. Thanks -- Nicolin ]

 arch/arm64/include/asm/esr.h    | 25 +++++++++++++++++++
 arch/arm64/include/asm/ptrace.h | 15 ++++++++++++
 arch/arm64/kernel/entry.S       |  4 ++--
 arch/arm64/kernel/traps.c       | 53 +++++++++++++++++++++++++++++++++++++++--
 4 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 014d7d8..55dea62 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -220,6 +220,31 @@
 		(((e) & ESR_ELx_SYS64_ISS_OP2_MASK) >>		\
 		 ESR_ELx_SYS64_ISS_OP2_SHIFT))
 
+/* ISS fields for MRC and MRS are very similar, so reuse SYS64 macros */
+#define ESR_ELx_CP15_32_ISS_MRC_MASK	ESR_ELx_SYS64_ISS_SYS_MASK
+#define ESR_ELx_CP15_32_ISS_MRC_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(0, 0, 0, 14, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
+/* ISS field definitions for CP15 MRRC/MCRR instructions (same for CP14) */
+#define ESR_ELx_CP15_64_ISS_OPC1_SHIFT	16
+#define ESR_ELx_CP15_64_ISS_OPC1_MASK	(UL(0xf) << ESR_ELx_CP15_64_ISS_OPC1_SHIFT)
+#define ESR_ELx_CP15_64_ISS_RT2_SHIFT	10
+#define ESR_ELx_CP15_64_ISS_RT2_MASK	(UL(0x1f) << ESR_ELx_CP15_64_ISS_RT2_SHIFT)
+#define ESR_ELx_CP15_64_ISS_RT_SHIFT	5
+#define ESR_ELx_CP15_64_ISS_RT_MASK	(UL(0x1f) << ESR_ELx_CP15_64_ISS_RT_SHIFT)
+#define ESR_ELx_CP15_64_ISS_CRM_SHIFT	1
+#define ESR_ELx_CP15_64_ISS_CRM_MASK	(UL(0xf) << ESR_ELx_CP15_64_ISS_CRM_SHIFT)
+#define ESR_ELx_CP15_64_ISS_MRRC_MASK	(ESR_ELx_CP15_64_ISS_OPC1_MASK | \
+					 ESR_ELx_CP15_64_ISS_CRM_MASK| \
+					 ESR_ELx_SYS64_ISS_DIR_MASK)
+
+#define ESR_ELx_CP15_64_ISS_MRRC_VAL(opc1, crm) \
+					(((opc1) << ESR_ELx_CP15_64_ISS_OPC1_SHIFT) | \
+					 ((crm) << ESR_ELx_CP15_64_ISS_CRM_SHIFT))
+
+#define ESR_ELx_CP15_64_ISS_MRRC_CNTVCT	(ESR_ELx_CP15_64_ISS_MRRC_VAL(1, 14) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 6069d66..50caf11 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -243,6 +243,21 @@ static inline void pt_regs_write_reg(struct pt_regs *regs, int r,
 		regs->regs[r] = val;
 }
 
+/*
+ * Write two registers given architectural register index r and r2.
+ * Used by 32-bit MRRC and MCRR instrutions for 64-bit results
+ */
+static inline void pt_regs_write_regs(struct pt_regs *regs, int r, int r2,
+				      unsigned long val)
+{
+	if (r != 31 && r2 != 31) {
+		/* Save lower 32 bits to register r */
+		regs->regs[r] = val & 0xffffffff;
+		/* Save higher 32 bits to register r2 */
+		regs->regs[r2] = val >> 32;
+	}
+}
+
 /* Valid only for Kernel mode traps. */
 static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
 {
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 6d14b8f..9d6cd95 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -636,9 +636,9 @@ el0_sync_compat:
 	cmp	x24, #ESR_ELx_EC_UNKNOWN	// unknown exception in EL0
 	b.eq	el0_undef
 	cmp	x24, #ESR_ELx_EC_CP15_32	// CP15 MRC/MCR trap
-	b.eq	el0_undef
+	b.eq	el0_sys
 	cmp	x24, #ESR_ELx_EC_CP15_64	// CP15 MRRC/MCRR trap
-	b.eq	el0_undef
+	b.eq	el0_sys
 	cmp	x24, #ESR_ELx_EC_CP14_MR	// CP14 MRC/MCR trap
 	b.eq	el0_undef
 	cmp	x24, #ESR_ELx_EC_CP14_LS	// CP14 LDC/STC trap
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 3d3588f..211cce7 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -454,6 +454,17 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 	arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
 }
 
+#ifdef CONFIG_COMPAT
+static void cntvct_read_32_handler(unsigned int esr, struct pt_regs *regs)
+{
+	int rt2 = (esr & ESR_ELx_CP15_64_ISS_RT2_MASK) >> ESR_ELx_CP15_64_ISS_RT2_SHIFT;
+	int rt = (esr & ESR_ELx_CP15_64_ISS_RT_MASK) >> ESR_ELx_CP15_64_ISS_RT_SHIFT;
+
+	pt_regs_write_regs(regs, rt, rt2, arch_counter_get_cntvct());
+	arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
+}
+#endif
+
 static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
 {
 	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
@@ -495,11 +506,49 @@ static struct sys64_hook sys64_hooks[] = {
 	{},
 };
 
+#ifdef CONFIG_COMPAT
+static struct sys64_hook cp15_32_hooks[] = {
+	{
+		/* Trap read access to CNTFRQ via 32-bit insturction MRC */
+		.esr_mask = ESR_ELx_CP15_32_ISS_MRC_MASK,
+		.esr_val = ESR_ELx_CP15_32_ISS_MRC_CNTFRQ,
+		.handler = cntfrq_read_handler,
+	},
+	{},
+};
+
+static struct sys64_hook cp15_64_hooks[] = {
+	{
+		/* Trap read access to CNTVCT via 32-bit insturction MRRC */
+		.esr_mask = ESR_ELx_CP15_64_ISS_MRRC_MASK,
+		.esr_val = ESR_ELx_CP15_64_ISS_MRRC_CNTVCT,
+		.handler = cntvct_read_32_handler,
+	},
+	{},
+};
+#endif
+
 asmlinkage void __exception do_sysinstr(unsigned int esr, struct pt_regs *regs)
 {
-	struct sys64_hook *hook;
+	struct sys64_hook *hook = NULL;
+
+	switch (ESR_ELx_EC(esr)) {
+#ifdef CONFIG_COMPAT
+	case ESR_ELx_EC_CP15_32:
+		hook = cp15_32_hooks;
+		break;
+	case ESR_ELx_EC_CP15_64:
+		hook = cp15_64_hooks;
+		break;
+#endif
+	case ESR_ELx_EC_SYS64:
+		hook = sys64_hooks;
+		break;
+	default:
+		break;
+	}
 
-	for (hook = sys64_hooks; hook->handler; hook++)
+	for (; hook && hook->handler; hook++)
 		if ((hook->esr_mask & esr) == hook->esr_val) {
 			hook->handler(esr, regs);
 			return;
-- 
2.1.4

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Marc Zyngier]

On 11/01/18 04:43, Nicolin Chen wrote:
> CONFIG_COMPAT allows ARM64 machine to run 32-bit instructions.
> Since the ARCH_TIMER_USR_VCT_ACCESS_EN might be disabled if a
> timer workaround is detected, accessing cntvct via mrrc will
> also trigger a trap.
> 
> So this patch adds support to handle this situation.
> 
> Tested with a user program generated by 32-bit compiler:
>   int main()
>   {
>       unsigned long long cval;
>       asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
>       return 0;
>   }
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> 
> [ I also added cntfrq here for safety as theoretically it could
>   trigger the trap as well. However, my another test case (with
>   mrc insturction) doesn't seem to trigger a trap. So I would
>   drop it in the next version if someone can confirm it's not
>   required. Thanks -- Nicolin ]

See my previous series on this very subject[1] as well as Will's reply.

> 
>  arch/arm64/include/asm/esr.h    | 25 +++++++++++++++++++
>  arch/arm64/include/asm/ptrace.h | 15 ++++++++++++
>  arch/arm64/kernel/entry.S       |  4 ++--
>  arch/arm64/kernel/traps.c       | 53 +++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 014d7d8..55dea62 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -220,6 +220,31 @@
>  		(((e) & ESR_ELx_SYS64_ISS_OP2_MASK) >>		\
>  		 ESR_ELx_SYS64_ISS_OP2_SHIFT))
>  
> +/* ISS fields for MRC and MRS are very similar, so reuse SYS64 macros */
> +#define ESR_ELx_CP15_32_ISS_MRC_MASK	ESR_ELx_SYS64_ISS_SYS_MASK
> +#define ESR_ELx_CP15_32_ISS_MRC_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(0, 0, 0, 14, 0) | \
> +					 ESR_ELx_SYS64_ISS_DIR_READ)
> +
> +/* ISS field definitions for CP15 MRRC/MCRR instructions (same for CP14) */
> +#define ESR_ELx_CP15_64_ISS_OPC1_SHIFT	16
> +#define ESR_ELx_CP15_64_ISS_OPC1_MASK	(UL(0xf) << ESR_ELx_CP15_64_ISS_OPC1_SHIFT)
> +#define ESR_ELx_CP15_64_ISS_RT2_SHIFT	10
> +#define ESR_ELx_CP15_64_ISS_RT2_MASK	(UL(0x1f) << ESR_ELx_CP15_64_ISS_RT2_SHIFT)
> +#define ESR_ELx_CP15_64_ISS_RT_SHIFT	5
> +#define ESR_ELx_CP15_64_ISS_RT_MASK	(UL(0x1f) << ESR_ELx_CP15_64_ISS_RT_SHIFT)
> +#define ESR_ELx_CP15_64_ISS_CRM_SHIFT	1
> +#define ESR_ELx_CP15_64_ISS_CRM_MASK	(UL(0xf) << ESR_ELx_CP15_64_ISS_CRM_SHIFT)
> +#define ESR_ELx_CP15_64_ISS_MRRC_MASK	(ESR_ELx_CP15_64_ISS_OPC1_MASK | \
> +					 ESR_ELx_CP15_64_ISS_CRM_MASK| \
> +					 ESR_ELx_SYS64_ISS_DIR_MASK)
> +
> +#define ESR_ELx_CP15_64_ISS_MRRC_VAL(opc1, crm) \
> +					(((opc1) << ESR_ELx_CP15_64_ISS_OPC1_SHIFT) | \
> +					 ((crm) << ESR_ELx_CP15_64_ISS_CRM_SHIFT))
> +
> +#define ESR_ELx_CP15_64_ISS_MRRC_CNTVCT	(ESR_ELx_CP15_64_ISS_MRRC_VAL(1, 14) | \
> +					 ESR_ELx_SYS64_ISS_DIR_READ)
> +
>  #ifndef __ASSEMBLY__
>  #include <asm/types.h>
>  
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 6069d66..50caf11 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -243,6 +243,21 @@ static inline void pt_regs_write_reg(struct pt_regs *regs, int r,
>  		regs->regs[r] = val;
>  }
>  
> +/*
> + * Write two registers given architectural register index r and r2.
> + * Used by 32-bit MRRC and MCRR instrutions for 64-bit results
> + */
> +static inline void pt_regs_write_regs(struct pt_regs *regs, int r, int r2,
> +				      unsigned long val)
> +{
> +	if (r != 31 && r2 != 31) {
> +		/* Save lower 32 bits to register r */
> +		regs->regs[r] = val & 0xffffffff;
> +		/* Save higher 32 bits to register r2 */
> +		regs->regs[r2] = val >> 32;
> +	}
> +}
> +
>  /* Valid only for Kernel mode traps. */
>  static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
>  {
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 6d14b8f..9d6cd95 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -636,9 +636,9 @@ el0_sync_compat:
>  	cmp	x24, #ESR_ELx_EC_UNKNOWN	// unknown exception in EL0
>  	b.eq	el0_undef
>  	cmp	x24, #ESR_ELx_EC_CP15_32	// CP15 MRC/MCR trap
> -	b.eq	el0_undef
> +	b.eq	el0_sys
>  	cmp	x24, #ESR_ELx_EC_CP15_64	// CP15 MRRC/MCRR trap
> -	b.eq	el0_undef
> +	b.eq	el0_sys
>  	cmp	x24, #ESR_ELx_EC_CP14_MR	// CP14 MRC/MCR trap
>  	b.eq	el0_undef
>  	cmp	x24, #ESR_ELx_EC_CP14_LS	// CP14 LDC/STC trap
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 3d3588f..211cce7 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -454,6 +454,17 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
>  	arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
>  }
>  
> +#ifdef CONFIG_COMPAT
> +static void cntvct_read_32_handler(unsigned int esr, struct pt_regs *regs)
> +{
> +	int rt2 = (esr & ESR_ELx_CP15_64_ISS_RT2_MASK) >> ESR_ELx_CP15_64_ISS_RT2_SHIFT;
> +	int rt = (esr & ESR_ELx_CP15_64_ISS_RT_MASK) >> ESR_ELx_CP15_64_ISS_RT_SHIFT;
> +
> +	pt_regs_write_regs(regs, rt, rt2, arch_counter_get_cntvct());
> +	arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
> +}
> +#endif
> +
>  static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
>  {
>  	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
> @@ -495,11 +506,49 @@ static struct sys64_hook sys64_hooks[] = {
>  	{},
>  };
>  
> +#ifdef CONFIG_COMPAT
> +static struct sys64_hook cp15_32_hooks[] = {
> +	{
> +		/* Trap read access to CNTFRQ via 32-bit insturction MRC */
> +		.esr_mask = ESR_ELx_CP15_32_ISS_MRC_MASK,
> +		.esr_val = ESR_ELx_CP15_32_ISS_MRC_CNTFRQ,
> +		.handler = cntfrq_read_handler,
> +	},
> +	{},
> +};
> +
> +static struct sys64_hook cp15_64_hooks[] = {
> +	{
> +		/* Trap read access to CNTVCT via 32-bit insturction MRRC */
> +		.esr_mask = ESR_ELx_CP15_64_ISS_MRRC_MASK,
> +		.esr_val = ESR_ELx_CP15_64_ISS_MRRC_CNTVCT,
> +		.handler = cntvct_read_32_handler,
> +	},
> +	{},
> +};
> +#endif
> +
>  asmlinkage void __exception do_sysinstr(unsigned int esr, struct pt_regs *regs)
>  {
> -	struct sys64_hook *hook;
> +	struct sys64_hook *hook = NULL;
> +
> +	switch (ESR_ELx_EC(esr)) {
> +#ifdef CONFIG_COMPAT
> +	case ESR_ELx_EC_CP15_32:
> +		hook = cp15_32_hooks;
> +		break;
> +	case ESR_ELx_EC_CP15_64:
> +		hook = cp15_64_hooks;
> +		break;
> +#endif
> +	case ESR_ELx_EC_SYS64:
> +		hook = sys64_hooks;
> +		break;
> +	default:
> +		break;
> +	}
>  
> -	for (hook = sys64_hooks; hook->handler; hook++)
> +	for (; hook && hook->handler; hook++)
>  		if ((hook->esr_mask & esr) == hook->esr_val) {
>  			hook->handler(esr, regs);
>  			return;
> 

Also, this code is fairly broken in its handling of conditional
instructions.

Thanks,

	M.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/520426.html
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Nicolin Chen]

Hello Marc,

On Thu, Jan 11, 2018 at 08:51:37AM +0000, Marc Zyngier wrote:
> > [ I also added cntfrq here for safety as theoretically it could
> >   trigger the trap as well. However, my another test case (with
> >   mrc insturction) doesn't seem to trigger a trap. So I would
> >   drop it in the next version if someone can confirm it's not
> >   required. Thanks -- Nicolin ]
> 
> See my previous series on this very subject[1] as well as Will's reply.

Thanks for the background.

> > -	for (hook = sys64_hooks; hook->handler; hook++)
> > +	for (; hook && hook->handler; hook++)
> >  		if ((hook->esr_mask & esr) == hook->esr_val) {
> >  			hook->handler(esr, regs);
> >  			return;
> > 
> 
> Also, this code is fairly broken in its handling of conditional
> instructions.

I understand that it should take care of the condition field as
a general instruction handler. Just for curiosity: If we confine
the topic to read access of CNTVCT/CNTFRQ, what'd be the penalty
by ignoring the condition field and executing it anyway?

Thank you
Nicolin

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Marc Zyngier]

On Tue, 16 Jan 2018 20:32:19 +0000,
Nicolin Chen wrote:
> 
> Hello Marc,
> 
> On Thu, Jan 11, 2018 at 08:51:37AM +0000, Marc Zyngier wrote:
> > > [ I also added cntfrq here for safety as theoretically it could
> > >   trigger the trap as well. However, my another test case (with
> > >   mrc insturction) doesn't seem to trigger a trap. So I would
> > >   drop it in the next version if someone can confirm it's not
> > >   required. Thanks -- Nicolin ]
> > 
> > See my previous series on this very subject[1] as well as Will's reply.
> 
> Thanks for the background.
> 
> > > -	for (hook = sys64_hooks; hook->handler; hook++)
> > > +	for (; hook && hook->handler; hook++)
> > >  		if ((hook->esr_mask & esr) == hook->esr_val) {
> > >  			hook->handler(esr, regs);
> > >  			return;
> > > 
> > 
> > Also, this code is fairly broken in its handling of conditional
> > instructions.
> 
> I understand that it should take care of the condition field as
> a general instruction handler. Just for curiosity: If we confine
> the topic to read access of CNTVCT/CNTFRQ, what'd be the penalty
> by ignoring the condition field and executing it anyway?

Do you mean, apart from severely corrupting userspace execution?
That's a rhetorical question, right?

	M.
-- 
Jazz is not dead, it just smell funny.

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Nicolin Chen]

On Tue, Jan 16, 2018 at 09:19:13PM +0000, Marc Zyngier wrote:

> > I understand that it should take care of the condition field as
> > a general instruction handler. Just for curiosity: If we confine
> > the topic to read access of CNTVCT/CNTFRQ, what'd be the penalty
> > by ignoring the condition field and executing it anyway?
> 
> Do you mean, apart from severely corrupting userspace execution?
> That's a rhetorical question, right?

I don't quite understand the corrupting userspace execution part.
What I see for a conditional CNTVCT read is more likely:
	if (condition) {	// in this case, if (true)
		r1 = lower32(cntvct);
		r2 = higher32(cntvct);
	}

Could you please elaborate a bit? Thank you.

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Nicolin Chen]

On Tue, Jan 16, 2018 at 01:37:46PM -0800, Nicolin Chen wrote:
> On Tue, Jan 16, 2018 at 09:19:13PM +0000, Marc Zyngier wrote:
> 
> > > I understand that it should take care of the condition field as
> > > a general instruction handler. Just for curiosity: If we confine
> > > the topic to read access of CNTVCT/CNTFRQ, what'd be the penalty
> > > by ignoring the condition field and executing it anyway?
> > 
> > Do you mean, apart from severely corrupting userspace execution?
> > That's a rhetorical question, right?
> 
> I don't quite understand the corrupting userspace execution part.
> What I see for a conditional CNTVCT read is more likely:
> 	if (condition) {	// in this case, if (true)
> 		r1 = lower32(cntvct);
> 		r2 = higher32(cntvct);
> 	}
> 
> Could you please elaborate a bit? Thank you.

I guess I got it now. The concern seems to be Thumb instructions.
So ignoring a condition for a Thumb instruction may cause its IT
scope shifting. For ARM mode, the only penalty could be two Rts
getting written -- which shouldn't corrupt userspace execution.

Please correct me if I am wrong or not thorough.

Thanks
Nicolin

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Marc Zyngier]

On 17/01/18 02:13, Nicolin Chen wrote:
> On Tue, Jan 16, 2018 at 01:37:46PM -0800, Nicolin Chen wrote:
>> On Tue, Jan 16, 2018 at 09:19:13PM +0000, Marc Zyngier wrote:
>>
>>>> I understand that it should take care of the condition field as
>>>> a general instruction handler. Just for curiosity: If we confine
>>>> the topic to read access of CNTVCT/CNTFRQ, what'd be the penalty
>>>> by ignoring the condition field and executing it anyway?
>>>
>>> Do you mean, apart from severely corrupting userspace execution?
>>> That's a rhetorical question, right?
>>
>> I don't quite understand the corrupting userspace execution part.
>> What I see for a conditional CNTVCT read is more likely:
>> 	if (condition) {	// in this case, if (true)
>> 		r1 = lower32(cntvct);
>> 		r2 = higher32(cntvct);
>> 	}
>>
>> Could you please elaborate a bit? Thank you.
> 
> I guess I got it now. The concern seems to be Thumb instructions.

Not only.

> So ignoring a condition for a Thumb instruction may cause its IT
> scope shifting. For ARM mode, the only penalty could be two Rts
> getting written -- which shouldn't corrupt userspace execution.
> 
> Please correct me if I am wrong or not thorough.

Consider the following:
	
	mov	r0, #0
	mov	r1, #0
	cmp	r1, #3
	mrrceq	r0, r1, cntvct // simplified version

Oh look, you've corrupted r0 and r1, which should never have be changed.
Whatever uses the content r0 and r1 after the mrrc will misbehave. How
is that an acceptable behaviour? How do you expect userspace to cope
with such a brain damage?

If you intend to emulate the CPU, you must emulate it fully, to the
letter of the architecture. No ifs, no buts.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Nicolin Chen]

On Wed, Jan 17, 2018 at 09:03:48AM +0000, Marc Zyngier wrote:

> > So ignoring a condition for a Thumb instruction may cause its IT
> > scope shifting. For ARM mode, the only penalty could be two Rts
> > getting written -- which shouldn't corrupt userspace execution.
> > 
> > Please correct me if I am wrong or not thorough.
> 
> Consider the following:
> 	
> 	mov	r0, #0
> 	mov	r1, #0
> 	cmp	r1, #3
> 	mrrceq	r0, r1, cntvct // simplified version
> 
> Oh look, you've corrupted r0 and r1, which should never have be changed.
> Whatever uses the content r0 and r1 after the mrrc will misbehave. How
> is that an acceptable behaviour? How do you expect userspace to cope
> with such a brain damage?
> 
> If you intend to emulate the CPU, you must emulate it fully, to the
> letter of the architecture. No ifs, no buts.

Thanks for the explain. I see the point here.

I saw your version for arm64 compat doesn't check if (rt != 31)
as MRS handler does. Is there any reason for that?

Thank you
Nicolin

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Robin Murphy]

On Wed, 17 Jan 2018 12:41:56 -0800
Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> On Wed, Jan 17, 2018 at 09:03:48AM +0000, Marc Zyngier wrote:
> 
> > > So ignoring a condition for a Thumb instruction may cause its IT
> > > scope shifting. For ARM mode, the only penalty could be two Rts
> > > getting written -- which shouldn't corrupt userspace execution.
> > > 
> > > Please correct me if I am wrong or not thorough.  
> > 
> > Consider the following:
> > 	
> > 	mov	r0, #0
> > 	mov	r1, #0
> > 	cmp	r1, #3
> > 	mrrceq	r0, r1, cntvct // simplified version
> > 
> > Oh look, you've corrupted r0 and r1, which should never have be
> > changed. Whatever uses the content r0 and r1 after the mrrc will
> > misbehave. How is that an acceptable behaviour? How do you expect
> > userspace to cope with such a brain damage?
> > 
> > If you intend to emulate the CPU, you must emulate it fully, to the
> > letter of the architecture. No ifs, no buts.  
> 
> Thanks for the explain. I see the point here.
> 
> I saw your version for arm64 compat doesn't check if (rt != 31)
> as MRS handler does. Is there any reason for that?

Um, perhaps because it's *compat*? How do you envision an AArch32
MR{R}C instruction targeting r31, exactly? ;)

Robin.

Re: [PATCH RFC v1] arm64: Handle traps from accessing CNTVCT/CNTFRQ for CONFIG_COMPAT

[Nicolin Chen]

On Wed, Jan 17, 2018 at 11:35:08PM +0000, Robin Murphy wrote:
> On Wed, 17 Jan 2018 12:41:56 -0800
> Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> 
> > On Wed, Jan 17, 2018 at 09:03:48AM +0000, Marc Zyngier wrote:
> > 
> > > > So ignoring a condition for a Thumb instruction may cause its IT
> > > > scope shifting. For ARM mode, the only penalty could be two Rts
> > > > getting written -- which shouldn't corrupt userspace execution.
> > > > 
> > > > Please correct me if I am wrong or not thorough.  
> > > 
> > > Consider the following:
> > > 	
> > > 	mov	r0, #0
> > > 	mov	r1, #0
> > > 	cmp	r1, #3
> > > 	mrrceq	r0, r1, cntvct // simplified version
> > > 
> > > Oh look, you've corrupted r0 and r1, which should never have be
> > > changed. Whatever uses the content r0 and r1 after the mrrc will
> > > misbehave. How is that an acceptable behaviour? How do you expect
> > > userspace to cope with such a brain damage?
> > > 
> > > If you intend to emulate the CPU, you must emulate it fully, to the
> > > letter of the architecture. No ifs, no buts.  
> > 
> > Thanks for the explain. I see the point here.
> > 
> > I saw your version for arm64 compat doesn't check if (rt != 31)
> > as MRS handler does. Is there any reason for that?
> 
> Um, perhaps because it's *compat*? How do you envision an AArch32
> MR{R}C instruction targeting r31, exactly? ;)

I see. Thank you, Robin.