[PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code

[PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code

[Tom Musta]

This patch series addresses bugs in the PowerPC single-step emulation
code (arch/powerpc/lib/sstep.c) pertaining to Little Endian.

The existing code has a chicken switch for little endian.  The first
patch softens the restriction so that only cross-endian modes are not
supported.

There is a general problem with unaligned little endian loads and stores.
This is addressed by the second patch.

Finally, there is a problem with unaligned single precision floating point
loads and stores which is addressed by the third patch.

Tom Musta (3):
  powerpc: Enable emulate_step In Little Endian Mode
  powerpc: Fix Unaligned Fixed Point Loads and Stores
  powerpc: Fix Unaligned LE Floating Point Loads and Stores

 arch/powerpc/lib/sstep.c |  109 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 94 insertions(+), 15 deletions(-)

[PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode

[Tom Musta]

This patch modifies the endian chicken switch in the single step
emulation code (emulate_step()).  The old (big endian) code bailed
early if a load or store instruction was to be emulated in little
endian mode.

The new code modifies the check and only bails in a cross-endian
situation (LE mode in a kernel compiled for BE and vice verse).

Signed-off-by: Tom Musta <tmusta@gmail.com>
---
 arch/powerpc/lib/sstep.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index b1faa15..5e0d0e9 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1222,12 +1222,18 @@ int __kprobes emulate_step(struct pt_regs *regs,
unsigned int instr)
 	}
 
 	/*
-	 * Following cases are for loads and stores, so bail out
-	 * if we're in little-endian mode.
+	 * Following cases are for loads and stores and this
+	 * implementation does not support cross-endian.  So
+	 * bail out if this is the case.
 	 */
+#ifdef __BIG_ENDIAN__
 	if (regs->msr & MSR_LE)
 		return 0;
-
+#endif
+#ifdef __LITTLE_ENDIAN__
+	if (!regs->msr & MSR_LE)
+		return 0;
+#endif
 	/*
 	 * Save register RA in case it's an update form load or store
 	 * and the access faults.
-- 
1.7.1

[PATCH 2/3] powerpc: Fix Unaligned Loads and Stores

[Tom Musta]

This patch modifies the unaligned access routines of the sstep.c
module so that it properly reverses the bytes of storage operands
in the little endian kernel kernel.   This is implemented by
breaking an unaligned little endian access into a combination of
single byte accesses plus an overal byte reversal operation.

Signed-off-by: Tom Musta <tmusta@gmail.com>
---
 arch/powerpc/lib/sstep.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 5e0d0e9..570f2af 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -212,11 +212,19 @@ static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea,
 {
 	int err;
 	unsigned long x, b, c;
+#ifdef __LITTLE_ENDIAN__
+	int len = nb; /* save a copy of the length for byte reversal */
+#endif
 
 	/* unaligned, do this in pieces */
 	x = 0;
 	for (; nb > 0; nb -= c) {
+#ifdef __LITTLE_ENDIAN__
+		c = 1;
+#endif
+#ifdef __BIG_ENDIAN__
 		c = max_align(ea);
+#endif
 		if (c > nb)
 			c = max_align(nb);
 		err = read_mem_aligned(&b, ea, c);
@@ -225,7 +233,24 @@ static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea,
 		x = (x << (8 * c)) + b;
 		ea += c;
 	}
+#ifdef __LITTLE_ENDIAN__
+	switch (len) {
+	case 2:
+		*dest = byterev_2(x);
+		break;
+	case 4:
+		*dest = byterev_4(x);
+		break;
+#ifdef __powerpc64__
+	case 8:
+		*dest = byterev_8(x);
+		break;
+#endif
+	}
+#endif
+#ifdef __BIG_ENDIAN__
 	*dest = x;
+#endif
 	return 0;
 }
 
@@ -273,9 +298,29 @@ static int __kprobes write_mem_unaligned(unsigned long val, unsigned long ea,
 	int err;
 	unsigned long c;
 
+#ifdef __LITTLE_ENDIAN__
+	switch (nb) {
+	case 2:
+		val = byterev_2(val);
+		break;
+	case 4:
+		val = byterev_4(val);
+		break;
+#ifdef __powerpc64__
+	case 8:
+		val = byterev_8(val);
+		break;
+#endif
+	}
+#endif
 	/* unaligned or little-endian, do this in pieces */
 	for (; nb > 0; nb -= c) {
+#ifdef __LITTLE_ENDIAN__
+		c = 1;
+#endif
+#ifdef __BIG_ENDIAN__
 		c = max_align(ea);
+#endif
 		if (c > nb)
 			c = max_align(nb);
 		err = write_mem_aligned(val >> (nb - c) * 8, ea, c);
-- 
1.7.1

[PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores

[Tom Musta]

This patch addresses unaligned single precision floating point loads
and stores in the single-step code.  The old implementation
improperly treated an 8 byte structure as an array of two 4 byte
words, which is a classic little endian bug.

Signed-off-by: Tom Musta <tmusta@gmail.com>
---
 arch/powerpc/lib/sstep.c |   52 +++++++++++++++++++++++++++++++++++----------
 1 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 570f2af..f6f17aa 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -355,22 +355,36 @@ static int __kprobes do_fp_load(int rn, int (*func)(int, unsigned long),
 				struct pt_regs *regs)
 {
 	int err;
-	unsigned long val[sizeof(double) / sizeof(long)];
+	union {
+		double dbl;
+		unsigned long ul[2];
+		struct {
+#ifdef __BIG_ENDIAN__
+			unsigned _pad_;
+			unsigned word;
+#endif
+#ifdef __LITTLE_ENDIAN__
+			unsigned word;
+			unsigned _pad_;
+#endif
+		} single;
+	} data;
 	unsigned long ptr;
 
 	if (!address_ok(regs, ea, nb))
 		return -EFAULT;
 	if ((ea & 3) == 0)
 		return (*func)(rn, ea);
-	ptr = (unsigned long) &val[0];
+	ptr = (unsigned long) &data.ul;
 	if (sizeof(unsigned long) == 8 || nb == 4) {
-		err = read_mem_unaligned(&val[0], ea, nb, regs);
-		ptr += sizeof(unsigned long) - nb;
+		err = read_mem_unaligned(&data.ul[0], ea, nb, regs);
+		if (nb == 4)
+			ptr = (unsigned long)&(data.single.word);
 	} else {
 		/* reading a double on 32-bit */
-		err = read_mem_unaligned(&val[0], ea, 4, regs);
+		err = read_mem_unaligned(&data.ul[0], ea, 4, regs);
 		if (!err)
-			err = read_mem_unaligned(&val[1], ea + 4, 4, regs);
+			err = read_mem_unaligned(&data.ul[1], ea + 4, 4, regs);
 	}
 	if (err)
 		return err;
@@ -382,28 +396,42 @@ static int __kprobes do_fp_store(int rn, int (*func)(int, unsigned long),
 				 struct pt_regs *regs)
 {
 	int err;
-	unsigned long val[sizeof(double) / sizeof(long)];
+	union {
+		double dbl;
+		unsigned long ul[2];
+		struct {
+#ifdef __BIG_ENDIAN__
+			unsigned _pad_;
+			unsigned word;
+#endif
+#ifdef __LITTLE_ENDIAN__
+			unsigned word;
+			unsigned _pad_;
+#endif
+		} single;
+	} data;
 	unsigned long ptr;
 
 	if (!address_ok(regs, ea, nb))
 		return -EFAULT;
 	if ((ea & 3) == 0)
 		return (*func)(rn, ea);
-	ptr = (unsigned long) &val[0];
+	ptr = (unsigned long) &data.ul[0];
 	if (sizeof(unsigned long) == 8 || nb == 4) {
-		ptr += sizeof(unsigned long) - nb;
+		if (nb == 4)
+			ptr = (unsigned long)&(data.single.word);
 		err = (*func)(rn, ptr);
 		if (err)
 			return err;
-		err = write_mem_unaligned(val[0], ea, nb, regs);
+		err = write_mem_unaligned(data.ul[0], ea, nb, regs);
 	} else {
 		/* writing a double on 32-bit */
 		err = (*func)(rn, ptr);
 		if (err)
 			return err;
-		err = write_mem_unaligned(val[0], ea, 4, regs);
+		err = write_mem_unaligned(data.ul[0], ea, 4, regs);
 		if (!err)
-			err = write_mem_unaligned(val[1], ea + 4, 4, regs);
+			err = write_mem_unaligned(data.ul[1], ea + 4, 4, regs);
 	}
 	return err;
 }
-- 
1.7.1

Re: [PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode

[Benjamin Herrenschmidt]

On Fri, 2013-10-18 at 14:40 -0500, Tom Musta wrote:
> This patch modifies the endian chicken switch in the single step
> emulation code (emulate_step()).  The old (big endian) code bailed
> early if a load or store instruction was to be emulated in little
> endian mode.
> 
> The new code modifies the check and only bails in a cross-endian
> situation (LE mode in a kernel compiled for BE and vice verse).

I get a malformed patch error, looks like it got wrapped.

Cheers,
Ben.

> Signed-off-by: Tom Musta <tmusta@gmail.com>
> ---
>  arch/powerpc/lib/sstep.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index b1faa15..5e0d0e9 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1222,12 +1222,18 @@ int __kprobes emulate_step(struct pt_regs *regs,
> unsigned int instr)
>  	}
>  
>  	/*
> -	 * Following cases are for loads and stores, so bail out
> -	 * if we're in little-endian mode.
> +	 * Following cases are for loads and stores and this
> +	 * implementation does not support cross-endian.  So
> +	 * bail out if this is the case.
>  	 */
> +#ifdef __BIG_ENDIAN__
>  	if (regs->msr & MSR_LE)
>  		return 0;
> -
> +#endif
> +#ifdef __LITTLE_ENDIAN__
> +	if (!regs->msr & MSR_LE)
> +		return 0;
> +#endif
>  	/*
>  	 * Save register RA in case it's an update form load or store
>  	 * and the access faults.

Re: [PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode

[Andreas Schwab]

Tom Musta <tommusta@gmail.com> writes:

> +#ifdef __LITTLE_ENDIAN__
> +	if (!regs->msr & MSR_LE)

That won't work.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode

[Tom Musta]

On 10/30/2013 12:43 PM, Andreas Schwab wrote:
> Tom Musta <tommusta@gmail.com> writes:
>
>> +#ifdef __LITTLE_ENDIAN__
>> +	if (!regs->msr & MSR_LE)
>
> That won't work.
>
> Andreas.
>

Please elaborate.

Re: [PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode

[Geert Uytterhoeven]

On Wed, Oct 30, 2013 at 8:35 PM, Tom Musta <tommusta@gmail.com> wrote:
> On 10/30/2013 12:43 PM, Andreas Schwab wrote:
>>
>> Tom Musta <tommusta@gmail.com> writes:
>>
>>> +#ifdef __LITTLE_ENDIAN__
>>> +       if (!regs->msr & MSR_LE)
>>
>>
>> That won't work.
>>
>> Andreas.
>>
>
> Please elaborate.

You want to test for "!(regs & MSR_LE)".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode

[Tom Musta]

On 10/30/2013 2:43 PM, Geert Uytterhoeven wrote:
> On Wed, Oct 30, 2013 at 8:35 PM, Tom Musta <tommusta@gmail.com> wrote:
>> On 10/30/2013 12:43 PM, Andreas Schwab wrote:
>>>
>>> Tom Musta <tommusta@gmail.com> writes:
>>>
>>>> +#ifdef __LITTLE_ENDIAN__
>>>> +       if (!regs->msr & MSR_LE)
>>>
>>>
>>> That won't work.
>>>
>>> Andreas.
>>>
>>
>> Please elaborate.
>
> You want to test for "!(regs & MSR_LE)".
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
>

Thanks Adnreas and Geert.  I will fix and resubmit.