[PATCH] x86, efi: Fix unaligned access and endian issues

[PATCH] x86, efi: Fix unaligned access and endian issues

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

We need to read from and write to 'buf' a byte at a time otherwise
it's possible we'll perform an unaligned access, which can lead to a
segfault when cross-building an x86 kernel on risc architectures.

Also, we may need to convert the endianness of the data we read
from/write to buf, so let's add some helper functions to do that.

Stephen Rothwell noticed this bug when he hit a segfault while
cross-building an x86_64 allmodconfig kernel on PowerPC.

Cc: H. Peter Anvin <hpa@zytor.com>
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/x86/boot/tools/build.c |   44 ++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 4e9bd6b..4030cb7 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -133,6 +133,26 @@ static void usage(void)
 	die("Usage: build setup system [> image]");
 }
 
+static inline u32 read32_le(u8 *src)
+{
+	u32 data;
+
+	data = *src++;
+	data |= *src++ << 8;
+	data |= *src++ << 16;
+	data |= *src++ << 24;
+
+	return data;
+}
+
+static inline void write32_le(u8 *dst, u32 data)
+{
+	*dst++ = data;
+	*dst++ = data >> 8;
+	*dst++ = data >> 16;
+	*dst++ = data >> 24;
+}
+
 int main(int argc, char ** argv)
 {
 #ifdef CONFIG_EFI_STUB
@@ -192,44 +212,42 @@ int main(int argc, char ** argv)
 
 	/* Patch the setup code with the appropriate size parameters */
 	buf[0x1f1] = setup_sectors-1;
-	buf[0x1f4] = sys_size;
-	buf[0x1f5] = sys_size >> 8;
-	buf[0x1f6] = sys_size >> 16;
-	buf[0x1f7] = sys_size >> 24;
+	write32_le(&buf[0x1f4], sys_size);
 
 #ifdef CONFIG_EFI_STUB
 	file_sz = sz + i + ((sys_size * 16) - sz);
 
-	pe_header = *(unsigned int *)&buf[0x3c];
+	pe_header = read32_le(&buf[0x3c]);
 
 	/* Size of code */
-	*(unsigned int *)&buf[pe_header + 0x1c] = file_sz;
+	write32_le(&buf[pe_header + 0x1c], file_sz);
 
 	/* Size of image */
-	*(unsigned int *)&buf[pe_header + 0x50] = file_sz;
+	write32_le(&buf[pe_header + 0x50], file_sz);
 
 #ifdef CONFIG_X86_32
 	/* Address of entry point */
-	*(unsigned int *)&buf[pe_header + 0x28] = i;
+	write32_le(&buf[pe_header + 0x28], i);
 
 	/* .text size */
-	*(unsigned int *)&buf[pe_header + 0xb0] = file_sz;
+	write32_le(&buf[pe_header + 0xb0], file_sz);
 
 	/* .text size of initialised data */
-	*(unsigned int *)&buf[pe_header + 0xb8] = file_sz;
+	write32_le(&buf[pe_header + 0xb8], file_sz);
 #else
 	/*
 	 * Address of entry point. startup_32 is at the beginning and
 	 * the 64-bit entry point (startup_64) is always 512 bytes
 	 * after.
 	 */
-	*(unsigned int *)&buf[pe_header + 0x28] = i + 512;
+	write32_le(&buf[pe_header + 0x28], i + 512);
 
 	/* .text size */
-	*(unsigned int *)&buf[pe_header + 0xc0] = file_sz;
+	write32_le(&buf[pe_header + 0xc0], file_sz);
 
 	/* .text size of initialised data */
-	*(unsigned int *)&buf[pe_header + 0xc8] = file_sz;
+	write32_le(&buf[pe_header + 0xc8], file_sz);
+
 #endif /* CONFIG_X86_32 */
 #endif /* CONFIG_EFI_STUB */
 
-- 
1.7.4.4

Re: [PATCH] x86, efi: Fix unaligned access and endian issues

[Nick Bowler]

On 2012-02-22 14:00 +0000, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> We need to read from and write to 'buf' a byte at a time otherwise
> it's possible we'll perform an unaligned access, which can lead to a
> segfault when cross-building an x86 kernel on risc architectures.
> 
> Also, we may need to convert the endianness of the data we read
> from/write to buf, so let's add some helper functions to do that.
[...]
> +static inline u32 read32_le(u8 *src)
> +{
> +	u32 data;
> +
> +	data = *src++;
> +	data |= *src++ << 8;
> +	data |= *src++ << 16;
> +	data |= *src++ << 24;
> +
> +	return data;
> +}

We already have get_unaligned_le32 in <asm/unaligned.h> for this.

> +
> +static inline void write32_le(u8 *dst, u32 data)
> +{
> +	*dst++ = data;
> +	*dst++ = data >> 8;
> +	*dst++ = data >> 16;
> +	*dst++ = data >> 24;
> +}

Similarly, put_unaligned_le32.

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [PATCH] x86, efi: Fix unaligned access and endian issues

[H. Peter Anvin]

On 02/22/2012 08:44 AM, Nick Bowler wrote:
> On 2012-02-22 14:00 +0000, Matt Fleming wrote:
>> From: Matt Fleming<matt.fleming@intel.com>
>>
>> We need to read from and write to 'buf' a byte at a time otherwise
>> it's possible we'll perform an unaligned access, which can lead to a
>> segfault when cross-building an x86 kernel on risc architectures.
>>
>> Also, we may need to convert the endianness of the data we read
>> from/write to buf, so let's add some helper functions to do that.
> [...]
>> +static inline u32 read32_le(u8 *src)
>> +{
>> +	u32 data;
>> +
>> +	data = *src++;
>> +	data |= *src++<<  8;
>> +	data |= *src++<<  16;
>> +	data |= *src++<<  24;
>> +
>> +	return data;
>> +}
>
> We already have get_unaligned_le32 in<asm/unaligned.h>  for this.
>
>> +
>> +static inline void write32_le(u8 *dst, u32 data)
>> +{
>> +	*dst++ = data;
>> +	*dst++ = data>>  8;
>> +	*dst++ = data>>  16;
>> +	*dst++ = data>>  24;
>> +}
>
> Similarly, put_unaligned_le32.
>

This is user space; those headers are not exported.  However, sticking 
to the same name would be good.

I'm wondering if this is the kind of things that should be put in 
usr/include?

	-hpa

Re: [PATCH] x86, efi: Fix unaligned access and endian issues

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi Matt,

On Wed, 22 Feb 2012 14:00:08 +0000 Matt Fleming <matt@console-pimps.org> wrote:
>
> From: Matt Fleming <matt.fleming@intel.com>
> 
> We need to read from and write to 'buf' a byte at a time otherwise
> it's possible we'll perform an unaligned access, which can lead to a
> segfault when cross-building an x86 kernel on risc architectures.
> 
> Also, we may need to convert the endianness of the data we read
> from/write to buf, so let's add some helper functions to do that.
> 
> Stephen Rothwell noticed this bug when he hit a segfault while
> cross-building an x86_64 allmodconfig kernel on PowerPC.
> 
> Cc: H. Peter Anvin <hpa@zytor.com>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

Tested-by: Stephen Rothwell <sfr@canb.auug.org.au> (cross build only)

The bzImage from a ARCH=x86_64 defconfig+CONFIG_EFI_STUB cross build is
in http://ozlabs.org/~sfr/bzImage if you want to attempt to boot it.

One little thing is that those two new functions may we warned about as
unused if CONFIG_EFI_STUB is not set (I have not done that build yet).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] x86, efi: Fix unaligned access and endian issues

[Matt Fleming]

On Thu, 2012-02-23 at 09:31 +1100, Stephen Rothwell wrote:
>
> Tested-by: Stephen Rothwell <sfr@canb.auug.org.au> (cross build only)
> 
> The bzImage from a ARCH=x86_64 defconfig+CONFIG_EFI_STUB cross build is
> in http://ozlabs.org/~sfr/bzImage if you want to attempt to boot it.

Thanks Stephen, this bzImage works fine on my setup.

> One little thing is that those two new functions may we warned about as
> unused if CONFIG_EFI_STUB is not set (I have not done that build yet).

Should be OK because they're static inline (it at least doesn't warn
here).

Peter was talking about exporting unaligned.h for host tools so this
patch will probably go through another iteration anyway.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Fix unaligned access and endian issues

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

Hi Matt,

On Wed, 22 Feb 2012 22:58:28 +0000 Matt Fleming <matt@console-pimps.org> wrote:
>
> Thanks Stephen, this bzImage works fine on my setup.

Great.

> > One little thing is that those two new functions may we warned about as
> > unused if CONFIG_EFI_STUB is not set (I have not done that build yet).
> 
> Should be OK because they're static inline (it at least doesn't warn
> here).

And I did the build and they don;t warn here either, so good.

> Peter was talking about exporting unaligned.h for host tools so this
> patch will probably go through another iteration anyway.

OK.  In that case, I will add this version to my fixes tree in linux-next
just so that the allmodconfig builds work again.  I will remove it when
it turns up (presumably in the tip tree).

Just to be clear, it wasn't the unaligned accesses that were causing me a
problem (PowerPC copes), it was the endian issues.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] x86, efi: Fix unaligned access and endian issues

[Matt Fleming]

On Thu, 2012-02-23 at 10:54 +1100, Stephen Rothwell wrote:
> > Peter was talking about exporting unaligned.h for host tools so this
> > patch will probably go through another iteration anyway.
> 
> OK.  In that case, I will add this version to my fixes tree in linux-next
> just so that the allmodconfig builds work again.  I will remove it when
> it turns up (presumably in the tip tree).

OK, thanks.

> Just to be clear, it wasn't the unaligned accesses that were causing me a
> problem (PowerPC copes), it was the endian issues.

Right. I'll fix up the commit message to be a bit clearer.

-- 
Matt Fleming, Intel Open Source Technology Center