Bug 18231 - ipc_perm struct's mode member has wrong type in sys/ipc.h
Summary: ipc_perm struct's mode member has wrong type in sys/ipc.h
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: 2.31
Assignee: Adhemerval Zanella
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-10 10:02 UTC by Szabolcs Nagy
Modified: 2019-10-10 20:38 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Szabolcs Nagy 2015-04-10 10:02:12 UTC 
the ipc_perm mode member must have type mode_t, this works on some targets (eg aarch64, linux) but broken on others (eg x86_64, linux)

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html

the following code fails on x86_64 linux:

#define _XOPEN_SOURCE 700
#include <sys/ipc.h>
void f()
{
struct ipc_perm x;
mode_t *p = &x.mode; // warning: initialization from incompatible pointer type [enabled by default]
}
Comment 1 Rich Felker 2015-06-19 07:27:28 UTC 
The following generic file is the one that's wrong:

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/bits/ipc.h;h=5c496eb39eebd5ae50d7eb4ff3e93d5ac9844491;hb=HEAD

Thanks to padding, this is easy to fix on little-endian archs, but it's a mess on big-endian, and has even led to regressions in the kernel to match the wrong glibc definition:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=2f3a499e6b803802880aea1fb8d3b46f1959494f
Comment 2 Rich Felker 2015-10-16 01:36:14 UTC 
Ping.
Comment 3 Andreas Schwab 2019-08-27 12:37:35 UTC 
RISC-V has inherited the same bug.
Comment 4 Rich Felker 2019-08-27 13:58:19 UTC 
Can glibc *please* fix the generic file to have the right type, and put the wrong-type bits files in local copies for the archs with the bug, so that this bug doesn't keep getting copied to new archs??

Ideally the wrong-type ones should also be fixed for all little-endian archs just by removing the pad field and changing the type to mode_t. For big-endian this can't be fixed without a new symbol version or similar, though...
Comment 6 Sourceware Commits 2019-10-10 20:38:07 UTC 
The master branch has been updated by Adhemerval Zanella <azanella@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=2f959dfe849e0646e27403f2e4091536496ac0f0

commit 2f959dfe849e0646e27403f2e4091536496ac0f0
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Thu Oct 10 18:13:11 2019 +0000

    sysvipc: Set ipc_perm mode as mode_t (BZ#18231)
    
    This patch sets the mode field in ipc_perm as mode_t for all architectures,
    as POSIX specification [1].  The changes required are as follow:
    
      1. It moves the ipc_perm definition out of ipc.h to its own header
         ipc_perm.h.  It also allows consolidate the IPC_* definition on
         only one header.
    
      2. The generic implementation follow the kernel ipc64_perm size so the
         syscall can be made directly without temporary buffer copy.  However,
         since glibc defines the MODE field as mode_t, it omits the __PAD1 field
         (since glibc does not export mode_t as 16-bit for any architecture).
    
         It is a two-fold improvement:
    
         2.1. New implementation which follow Linux UAPI will not need to
    	  provide an arch-specific ipc-perm.h header neither wrongly
              use the wrong 16-bit definition from previous default ipc.h
    	  (as csky did).
    
         2.1. It allows consolidate ipc_perm definition for architectures that
              already provide mode_t as 32-bit.
    
      3. All kernel ABIs for the supported architectures already provides the
         expected padding for mode type extension to 32-bit.  However, some
         architectures the padding has the wrong placement, so it requires
         the ipc control routines (msgctl, semctl, and shmctl) to adjust the
         mode field accordingly.  Currently they are armeb, microblaze, m68k,
         s390, and sheb.
    
         A new assume is added, __ASSUME_SYSVIPC_BROKEN_MODE_T, which the
         required ABIs define.
    
      4. For the ABIs that define __ASSUME_SYSVIPC_BROKEN_MODE_T, it also
         require compat symbols that do not adjust the mode field.
    
    Checked on arm-linux-gnueabihf, aarch64-linux-gnu, powerpc64le-linux-gnu,
    and x86_64-linux-gnu. I also checked the sysvipc tests on hppa-linux-gnu,
    sh4-linux-gnu, s390x-linux-gnu, and s390-linux-gnu.
    
    I also did a sanity test against armeb qemu usermode for the sysvipc
    tests.
    
    	[BZ #18231]
    	* sysdeps/unix/sysv/linux/Makefile (sysdep_headers): Add
    	bits/ipc-perm.h.
    	* sysdeps/unix/sysv/linux/aarch64/bits/ipc.h: Remove file.
    	* sysdeps/unix/sysv/linux/alpha/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/hppa/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/ia64/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/mips/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/powerpc/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/s390/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/sparc/bits/ipc.h: Likewise.
    	* sysdeps/unix/sysv/linux/arm/kernel-features.h
    	[__BYTE_ORDER == __BIG_ENDIAN] (__ASSUME_SYSVIPC_BROKEN_MODE_T):
    	Define.
    	* sysdeps/sysv/linux/microblaze/kernel-features.h: Likewise.
    	* sysdeps/unix/sysv/linux/s390/kernel-features.h
    	[!__s390x__] (__ASSUME_SYSVIPC_BROKEN_MODE_T): Define.
    	* sysdeps/unix/sysv/linux/sh/kernel-features.h
    	(__ASSUME_SYSVIPC_BROKEN_MODE_T): Define.
    	* sysdeps/unix/sysv/linux/m68k/kernel-features.h: Likewise.
    	* sysdeps/unix/sysv/linux/bits/ipc-perm.h: New file.
    	* sysdeps/unix/sysv/linux/powerpc/bits/ipc-perm.h: Likewise.
    	* sysdeps/unix/sysv/linux/sparc/bits/ipc-perm.h: Likewise.
    	* sysdeps/unix/sysv/linux/bits/ipc.h (ipc_perm): Move to
    	bits/ipc-perm.h.
    	* sysdeps/unix/sysv/linux/hppa/bits/ipc-perm.h: New file.
    	* sysdeps/unix/sysv/linux/kernel-features.h: Add comment about
    	__ASSUME_SYSVIPC_BROKEN_MODE_T semantic.
    	* sysdeps/unix/sysv/linux/msgctl.c (DEFAULT_VERSION): Define as
    	2.31 if __ASSUME_SYSVIPC_BROKEN_MODE_T is defined.
    	(msgctl_syscall, __msgctl_mode16): New symbol.
    	(__new_msgctl): Add bits for __ASSUME_SYSVIPC_BROKEN_MODE_T.
    	* sysdeps/unix/sysv/linux/semctl.c: Likewise.
    	* sysdeps/unix/sysv/linux/shmctl.c: Likewise.
    	* sysdeps/unix/sysv/linux/arm/be/libc.abilist (GLIBC_2.31): Add
    	msgctl, semctl, and shmctl.
    	* sysdeps/sysv/linux/microblaze/be/libc.abilist: Likewise.
    	* sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist: Likewise.
    	* sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist: Likewise.
    	* sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist: Likewise.
    	* sysdeps/unix/sysv/linux/sh/be/libc.abilist: Likewise.
    	* conform/data/sys/ipc.h-data: Only xfail {struct ipc_perm} mode_t
    	mode for Hurd.
    	* sysdeps/unix/sysv/linux/m68k/Versions (libc) [GLIBC_2.31]: Add
    	msgctl, semctl, and shmctl.
    	* sysdeps/unix/sysv/linux/arm/be/Versions: New file.
    	* sysdeps/unix/sysv/linux/microblaze/be/Versions: Likewise.
    	* sysdeps/unix/sysv/linux/sh/be/Versions: Likewise.
    
    [1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html
Comment 7 Adhemerval Zanella 2019-10-10 20:38:37 UTC 
Fixed on 2.31 (2f959dfe849e0646e27403f2e4091536496ac0f0).