compat: fix compat types affecting struct compat_elf_prpsinfo

The compat_elf_prpsinfo structure does not match the arch/arm struct elf_pspsinfo definition. As result NT_PRPSINFO note in core file created by arm64 kernel for aarch32 (compat) process has wrong size. So gdb cannot display command that caused process crash.

Fix is to change size of __compat_uid_t, __compat_gid_t so it would match size of similar fields in arch/arm case.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/include/asm/compat.h +0 -0

index 253e33b..56de5aa 100644

Quoted 23 lines · Victor Kamensky

The compat_elf_prpsinfo structure does not match the arch/arm struct elf_pspsinfo definition. As result NT_PRPSINFO note in core file created by arm64 kernel for aarch32 (compat) process has wrong size. So gdb cannot display command that caused process crash.

Fix is to change size of __compat_uid_t, __compat_gid_t so it would match size of similar fields in arch/arm case.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/include/asm/compat.h +0 -2

index 253e33b..56de5aa 100644
@@ -37,8 +37,8 @@ typedef s32		compat_ssize_t;
 typedef s32		compat_time_t;
 typedef s32		compat_clock_t;
 typedef s32		compat_pid_t;
-typedef u32		__compat_uid_t;
-typedef u32		__compat_gid_t;

arch/arm64/include/asm/compat.h typedef s32 compat_ssize_t; · line 44

Quoted 5 lines

arch/arm64/include/asm/compat.h +2 -0

+typedef u16		__compat_uid_t;
+typedef u16		__compat_gid_t;
 typedef u16		__compat_uid16_t;
 typedef u16		__compat_gid16_t;
 typedef u32		__compat_uid32_t;

__compat_uid_t is defined to match the arm32 uid_t and that would be __kernel_uid32_t (or __compat_uid32_t). So this is the correct fix.

Quoted 23 lines · Victor Kamensky

The compat_elf_prpsinfo structure does not match the arch/arm struct elf_pspsinfo definition. As result NT_PRPSINFO note in core file created by arm64 kernel for aarch32 (compat) process has wrong size. So gdb cannot display command that caused process crash.

Fix is to change size of __compat_uid_t, __compat_gid_t so it would match size of similar fields in arch/arm case.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/include/asm/compat.h +0 -2

index 253e33b..56de5aa 100644
@@ -37,8 +37,8 @@ typedef s32         compat_ssize_t;
 typedef s32          compat_time_t;
 typedef s32          compat_clock_t;
 typedef s32          compat_pid_t;
-typedef u32          __compat_uid_t;
-typedef u32          __compat_gid_t;

arch/arm64/include/asm/compat.h typedef s32 compat_ssize_t; · line 44

Quoted 5 lines

arch/arm64/include/asm/compat.h +2 -0

+typedef u16          __compat_uid_t;
+typedef u16          __compat_gid_t;
 typedef u16          __compat_uid16_t;
 typedef u16          __compat_gid16_t;
 typedef u32          __compat_uid32_t;

__compat_uid_t is defined to match the arm32 uid_t and that would be __kernel_uid32_t (or __compat_uid32_t). So this is not the correct fix.

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which is 32-bit. In reality compat_uid_t is different from the arm32 kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

Quoted 24 lines · On Tuesday 14 October 2014 09:51:25 Catalin Marinas wrote:

Quoted 23 lines · Victor Kamensky

The compat_elf_prpsinfo structure does not match the arch/arm struct elf_pspsinfo definition. As result NT_PRPSINFO note in core file created by arm64 kernel for aarch32 (compat) process has wrong size. So gdb cannot display command that caused process crash.

Fix is to change size of __compat_uid_t, __compat_gid_t so it would match size of similar fields in arch/arm case.

Signed-off-by Victor Kamensky <[email protected]>

---
 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/include/asm/compat.h +0 -2

index 253e33b..56de5aa 100644
@@ -37,8 +37,8 @@ typedef s32         compat_ssize_t;
 typedef s32          compat_time_t;
 typedef s32          compat_clock_t;
 typedef s32          compat_pid_t;
-typedef u32          __compat_uid_t;
-typedef u32          __compat_gid_t;

Quoted 8 lines

arch/arm64/include/asm/compat.h typedef s32 compat_ssize_t; · line 44

Quoted 5 lines

arch/arm64/include/asm/compat.h +2 -0

+typedef u16          __compat_uid_t;
+typedef u16          __compat_gid_t;
 typedef u16          __compat_uid16_t;
 typedef u16          __compat_gid16_t;
 typedef u32          __compat_uid32_t;

__compat_uid_t is defined to match the arm32 uid_t and that would be __kernel_uid32_t (or __compat_uid32_t). So this is not the correct fix.

No, I think Victor is right: __compat_uid_t should match the arm32 __kernel_uid_t, not the arm32 uid_t, which is just a kernel-internal definition, while the __kernel_uid_t is the one used in all user visible interfaces.

The definition in your asm/compat.h file seems to be a mistake.

Quoted 4 lines

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which is 32-bit. In reality compat_uid_t is different from the arm32 kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

compat_uid_t should match the __kernel_uid_t for all 32-bit architectures that are emulated on a 64-bit architecture, that is the definition.

Quoted 7 lines

Quoted 4 lines

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which is 32-bit. In reality compat_uid_t is different from the arm32 kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

compat_uid_t should match the __kernel_uid_t for all 32-bit architectures that are emulated on a 64-bit architecture, that is the definition.

I guess you meant __compat_uid_t here. The compat_uid_t type is u32 already.

So that patch is fine, I'll take it for 3.17 (and cc stable all the way back to 3.7).

Quoted 8 lines · On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:

Quoted 7 lines

Quoted 4 lines

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which is 32-bit. In reality compat_uid_t is different from the arm32 kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

compat_uid_t should match the __kernel_uid_t for all 32-bit architectures that are emulated on a 64-bit architecture, that is the definition.

Quoted 2 lines

I guess you meant __compat_uid_t here. The compat_uid_t type is u32 already.

Ah, that's weird: compat_uid_t is not used anywhere in the kernel, and the definition is odd. Apparently it was intentional back in 2005 when Stephen Rothwell introduced it as part of 202e5979af4d9 ("compat: be more consistent about [ug]id_t"), but I have trouble understanding the intention.

Quoted 2 lines

So that patch is fine, I'll take it for 3.17 (and cc stable all the way back to 3.7).

Ok. It might be worth checking if there are any uses of __compat_uid_t in arm64 that should have been __compat_uid32_t. Currently they are the same, but after Victor's patch, they would be different, which could cause regressions.

Quoted 12 lines · Arnd Bergmann

Quoted 10 lines · On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:

Quoted 7 lines

Quoted 4 lines

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which is 32-bit. In reality compat_uid_t is different from the arm32 kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

compat_uid_t should match the __kernel_uid_t for all 32-bit architectures that are emulated on a 64-bit architecture, that is the definition.

I guess you meant __compat_uid_t here. The compat_uid_t type is u32 already.

Quoted 5 lines

Ah, that's weird: compat_uid_t is not used anywhere in the kernel, and the definition is odd. Apparently it was intentional back in 2005 when Stephen Rothwell introduced it as part of 202e5979af4d9 ("compat: be more consistent about [ug]id_t"), but I have trouble understanding the intention.

It may be worth removing to avoid confusion.

Quoted 2 lines

Quoted 2 lines

So that patch is fine, I'll take it for 3.17 (and cc stable all the way back to 3.7).

Quoted 5 lines

Ok. It might be worth checking if there are any uses of __compat_uid_t in arm64 that should have been __compat_uid32_t. Currently they are the same, but after Victor's patch, they would be different, which could cause regressions.

A quick grep shows __compat_uid_t being used for:

struct compat_ncp_mount_data
struct compat_elf_prpsinfo
struct compat_ipc_perm

In all these cases, the native structures on arm32 would use __kernel_uid_t. The arch/arm64 code doesn't make any use of __compat_uid_t, apart from defining it.

But I'll run some LTP again to make sure (though I don't have many hopes of it being useful since this bug wasn't previously detected).

--
Catalin

Quoted 19 lines · Catalin Marinas

Quoted 17 lines · Arnd Bergmann

Quoted 10 lines · On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:

Quoted 7 lines

Quoted 4 lines

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which is 32-bit. In reality compat_uid_t is different from the arm32 kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

compat_uid_t should match the __kernel_uid_t for all 32-bit architectures that are emulated on a 64-bit architecture, that is the definition.

I guess you meant __compat_uid_t here. The compat_uid_t type is u32 already.

Ah, that's weird: compat_uid_t is not used anywhere in the kernel, and the definition is odd. Apparently it was intentional back in 2005 when Stephen Rothwell introduced it as part of 202e5979af4d9 ("compat: be more consistent about [ug]id_t"), but I have trouble understanding the intention.

Quoted 1 line

It may be worth removing to avoid confusion.

Do I need to do that? Or it can be done latter? I think, if anyone will do that, it should be separate commit anyway.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So that patch is fine, I'll take it for 3.17 (and cc stable all the way back to 3.7).

Catalin, Arnd, do I have permission to use your Acked-by with next post of the patch (where I would "cc stable")?

Quoted 7 lines

Quoted 4 lines

Ok. It might be worth checking if there are any uses of __compat_uid_t in arm64 that should have been __compat_uid32_t. Currently they are the same, but after Victor's patch, they would be different, which could cause regressions.

A quick grep shows __compat_uid_t being used for:

Quoted 7 lines

struct compat_ncp_mount_data
struct compat_elf_prpsinfo
struct compat_ipc_perm

In all these cases, the native structures on arm32 would use __kernel_uid_t. The arch/arm64 code doesn't make any use of __compat_uid_t, apart from defining it.

When I run into the issue, I've tried to look for similar mismatch issues in other places. I wrote quick awk script that would parse 'readelf --debug-dump vmlinux'
output and dump names and sizes of all arm64 structs that starts with compat_ and then compared them with corresponding structures sizes in TC2 image. I saw that compat_ncp_mount_data, compat_elf_prpsinfo, compat_ipc_perm and three other that use compat_ipc_perm sizes changed. But after the fix applied they match arch/arm sizes, and they were not matching before.

Besides those in all other cases arm64 compat and corresponding arch/arm struct sizes match each other (modulo missing features in TC2 image that were not checked; like cdrom, floppy, video related, and few others).