LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] powerpc: Add build-time check of ptrace PT_xx defines
@ 2019-10-30 11:12 Michael Ellerman
  2019-10-30 11:17 ` Christophe Leroy
  2019-11-07  3:45 ` Michael Ellerman
  0 siblings, 2 replies; 3+ messages in thread
From: Michael Ellerman @ 2019-10-30 11:12 UTC (permalink / raw)
  To: linuxppc-dev

As part of the uapi we export a lot of PT_xx defines for each register
in struct pt_regs. These are expressed as an index from gpr[0], in
units of unsigned long.

Currently there's nothing tying the values of those defines to the
actual layout of the struct.

But we *don't* want to change the uapi defines to derive the PT_xx
values based on the layout of the struct, those values are ABI and
must never change.

Instead we want to do the reverse, make sure that the layout of the
struct never changes vs the PT_xx defines. So add build time checks of
that.

This probably seems paranoid, but at least once in the past someone
has sent a patch that would have broken the ABI if it hadn't been
spotted. Although it probably would have been detected via testing,
it's preferable to just quash any issues at the source.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/ptrace.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 8c92febf5f44..abd888bada03 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3398,4 +3398,67 @@ void __init pt_regs_check(void)
 		     offsetof(struct user_pt_regs, result));
 
 	BUILD_BUG_ON(sizeof(struct user_pt_regs) > sizeof(struct pt_regs));
+
+	// Now check that the pt_regs offsets match the uapi #defines
+	#define CHECK_REG(_pt, _reg) \
+		BUILD_BUG_ON(_pt != (offsetof(struct user_pt_regs, _reg) / \
+				     sizeof(unsigned long)));
+
+	CHECK_REG(PT_R0,  gpr[0]);
+	CHECK_REG(PT_R1,  gpr[1]);
+	CHECK_REG(PT_R2,  gpr[2]);
+	CHECK_REG(PT_R3,  gpr[3]);
+	CHECK_REG(PT_R4,  gpr[4]);
+	CHECK_REG(PT_R5,  gpr[5]);
+	CHECK_REG(PT_R6,  gpr[6]);
+	CHECK_REG(PT_R7,  gpr[7]);
+	CHECK_REG(PT_R8,  gpr[8]);
+	CHECK_REG(PT_R9,  gpr[9]);
+	CHECK_REG(PT_R10, gpr[10]);
+	CHECK_REG(PT_R11, gpr[11]);
+	CHECK_REG(PT_R12, gpr[12]);
+	CHECK_REG(PT_R13, gpr[13]);
+	CHECK_REG(PT_R14, gpr[14]);
+	CHECK_REG(PT_R15, gpr[15]);
+	CHECK_REG(PT_R16, gpr[16]);
+	CHECK_REG(PT_R17, gpr[17]);
+	CHECK_REG(PT_R18, gpr[18]);
+	CHECK_REG(PT_R19, gpr[19]);
+	CHECK_REG(PT_R20, gpr[20]);
+	CHECK_REG(PT_R21, gpr[21]);
+	CHECK_REG(PT_R22, gpr[22]);
+	CHECK_REG(PT_R23, gpr[23]);
+	CHECK_REG(PT_R24, gpr[24]);
+	CHECK_REG(PT_R25, gpr[25]);
+	CHECK_REG(PT_R26, gpr[26]);
+	CHECK_REG(PT_R27, gpr[27]);
+	CHECK_REG(PT_R28, gpr[28]);
+	CHECK_REG(PT_R29, gpr[29]);
+	CHECK_REG(PT_R30, gpr[30]);
+	CHECK_REG(PT_R31, gpr[31]);
+	CHECK_REG(PT_NIP, nip);
+	CHECK_REG(PT_MSR, msr);
+	CHECK_REG(PT_ORIG_R3, orig_gpr3);
+	CHECK_REG(PT_CTR, ctr);
+	CHECK_REG(PT_LNK, link);
+	CHECK_REG(PT_XER, xer);
+	CHECK_REG(PT_CCR, ccr);
+#ifdef CONFIG_PPC64
+	CHECK_REG(PT_SOFTE, softe);
+#else
+	CHECK_REG(PT_MQ, mq);
+#endif
+	CHECK_REG(PT_TRAP, trap);
+	CHECK_REG(PT_DAR, dar);
+	CHECK_REG(PT_DSISR, dsisr);
+	CHECK_REG(PT_RESULT, result);
+	#undef CHECK_REG
+
+	BUILD_BUG_ON(PT_REGS_COUNT != sizeof(struct user_pt_regs) / sizeof(unsigned long));
+
+	/*
+	 * PT_DSCR isn't a real reg, but it's important that it doesn't overlap the
+	 * real registers.
+	 */
+	BUILD_BUG_ON(PT_DSCR < sizeof(struct user_pt_regs) / sizeof(unsigned long));
 }
-- 
2.21.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc: Add build-time check of ptrace PT_xx defines
  2019-10-30 11:12 [PATCH] powerpc: Add build-time check of ptrace PT_xx defines Michael Ellerman
@ 2019-10-30 11:17 ` Christophe Leroy
  2019-11-07  3:45 ` Michael Ellerman
  1 sibling, 0 replies; 3+ messages in thread
From: Christophe Leroy @ 2019-10-30 11:17 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev



Le 30/10/2019 à 12:12, Michael Ellerman a écrit :
> As part of the uapi we export a lot of PT_xx defines for each register
> in struct pt_regs. These are expressed as an index from gpr[0], in
> units of unsigned long.
> 
> Currently there's nothing tying the values of those defines to the
> actual layout of the struct.
> 
> But we *don't* want to change the uapi defines to derive the PT_xx
> values based on the layout of the struct, those values are ABI and
> must never change.
> 
> Instead we want to do the reverse, make sure that the layout of the
> struct never changes vs the PT_xx defines. So add build time checks of
> that.
> 
> This probably seems paranoid, but at least once in the past someone
> has sent a patch that would have broken the ABI if it hadn't been
> spotted. Although it probably would have been detected via testing,
> it's preferable to just quash any issues at the source.

While you are playing with pt_regs_check(), could you also take the 
patch from Mathieu https://patchwork.ozlabs.org/patch/1009816/ ?

Christophe

> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/kernel/ptrace.c | 63 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 8c92febf5f44..abd888bada03 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -3398,4 +3398,67 @@ void __init pt_regs_check(void)
>   		     offsetof(struct user_pt_regs, result));
>   
>   	BUILD_BUG_ON(sizeof(struct user_pt_regs) > sizeof(struct pt_regs));
> +
> +	// Now check that the pt_regs offsets match the uapi #defines
> +	#define CHECK_REG(_pt, _reg) \
> +		BUILD_BUG_ON(_pt != (offsetof(struct user_pt_regs, _reg) / \
> +				     sizeof(unsigned long)));
> +
> +	CHECK_REG(PT_R0,  gpr[0]);
> +	CHECK_REG(PT_R1,  gpr[1]);
> +	CHECK_REG(PT_R2,  gpr[2]);
> +	CHECK_REG(PT_R3,  gpr[3]);
> +	CHECK_REG(PT_R4,  gpr[4]);
> +	CHECK_REG(PT_R5,  gpr[5]);
> +	CHECK_REG(PT_R6,  gpr[6]);
> +	CHECK_REG(PT_R7,  gpr[7]);
> +	CHECK_REG(PT_R8,  gpr[8]);
> +	CHECK_REG(PT_R9,  gpr[9]);
> +	CHECK_REG(PT_R10, gpr[10]);
> +	CHECK_REG(PT_R11, gpr[11]);
> +	CHECK_REG(PT_R12, gpr[12]);
> +	CHECK_REG(PT_R13, gpr[13]);
> +	CHECK_REG(PT_R14, gpr[14]);
> +	CHECK_REG(PT_R15, gpr[15]);
> +	CHECK_REG(PT_R16, gpr[16]);
> +	CHECK_REG(PT_R17, gpr[17]);
> +	CHECK_REG(PT_R18, gpr[18]);
> +	CHECK_REG(PT_R19, gpr[19]);
> +	CHECK_REG(PT_R20, gpr[20]);
> +	CHECK_REG(PT_R21, gpr[21]);
> +	CHECK_REG(PT_R22, gpr[22]);
> +	CHECK_REG(PT_R23, gpr[23]);
> +	CHECK_REG(PT_R24, gpr[24]);
> +	CHECK_REG(PT_R25, gpr[25]);
> +	CHECK_REG(PT_R26, gpr[26]);
> +	CHECK_REG(PT_R27, gpr[27]);
> +	CHECK_REG(PT_R28, gpr[28]);
> +	CHECK_REG(PT_R29, gpr[29]);
> +	CHECK_REG(PT_R30, gpr[30]);
> +	CHECK_REG(PT_R31, gpr[31]);
> +	CHECK_REG(PT_NIP, nip);
> +	CHECK_REG(PT_MSR, msr);
> +	CHECK_REG(PT_ORIG_R3, orig_gpr3);
> +	CHECK_REG(PT_CTR, ctr);
> +	CHECK_REG(PT_LNK, link);
> +	CHECK_REG(PT_XER, xer);
> +	CHECK_REG(PT_CCR, ccr);
> +#ifdef CONFIG_PPC64
> +	CHECK_REG(PT_SOFTE, softe);
> +#else
> +	CHECK_REG(PT_MQ, mq);
> +#endif
> +	CHECK_REG(PT_TRAP, trap);
> +	CHECK_REG(PT_DAR, dar);
> +	CHECK_REG(PT_DSISR, dsisr);
> +	CHECK_REG(PT_RESULT, result);
> +	#undef CHECK_REG
> +
> +	BUILD_BUG_ON(PT_REGS_COUNT != sizeof(struct user_pt_regs) / sizeof(unsigned long));
> +
> +	/*
> +	 * PT_DSCR isn't a real reg, but it's important that it doesn't overlap the
> +	 * real registers.
> +	 */
> +	BUILD_BUG_ON(PT_DSCR < sizeof(struct user_pt_regs) / sizeof(unsigned long));
>   }
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] powerpc: Add build-time check of ptrace PT_xx defines
  2019-10-30 11:12 [PATCH] powerpc: Add build-time check of ptrace PT_xx defines Michael Ellerman
  2019-10-30 11:17 ` Christophe Leroy
@ 2019-11-07  3:45 ` Michael Ellerman
  1 sibling, 0 replies; 3+ messages in thread
From: Michael Ellerman @ 2019-11-07  3:45 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev

On Wed, 2019-10-30 at 11:12:31 UTC, Michael Ellerman wrote:
> As part of the uapi we export a lot of PT_xx defines for each register
> in struct pt_regs. These are expressed as an index from gpr[0], in
> units of unsigned long.
> 
> Currently there's nothing tying the values of those defines to the
> actual layout of the struct.
> 
> But we *don't* want to change the uapi defines to derive the PT_xx
> values based on the layout of the struct, those values are ABI and
> must never change.
> 
> Instead we want to do the reverse, make sure that the layout of the
> struct never changes vs the PT_xx defines. So add build time checks of
> that.
> 
> This probably seems paranoid, but at least once in the past someone
> has sent a patch that would have broken the ABI if it hadn't been
> spotted. Although it probably would have been detected via testing,
> it's preferable to just quash any issues at the source.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Applied to powerpc next.

https://git.kernel.org/powerpc/c/b9e0805abf2e92fc275ac5fbd8c1c9a92b00413d

cheers

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-30 11:12 [PATCH] powerpc: Add build-time check of ptrace PT_xx defines Michael Ellerman
2019-10-30 11:17 ` Christophe Leroy
2019-11-07  3:45 ` Michael Ellerman

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git