[v2] linux: open and openat ignore 'mode' with O_TMPFILE in flags

  Both open and openat load their last argument 'mode' lazily, using
va_arg() only if O_CREAT is found in oflag. This is wrong, mode is also
necessary if O_TMPFILE is in oflag.

By chance on x86_64, the problem wasn't evident when using O_TMPFILE
with open, as the 3rd argument of open, even when not loaded with
va_arg, is left untouched in RDX, where the syscall expects it.

However, openat was not so lucky, and O_TMPFILE couldn't be used: mode
is the 4th argument, in RCX, but the syscall expects its 4th argument in
a different register than the glibc wrapper, in R10.

Introduce a macro __OPEN_NEEDS_MODE (oflag) to test if either O_CREAT or
O_TMPFILE is set in oflag.

Tested on Linux x86_64.

	[BZ #17523]
	* io/fcntl.h: Define __OPEN_NEEDS_MODE.
	* io/bits/fcntl2.h: Use __OPEN_NEEDS_MODE.
	* io/open.c: Likewise.
	* io/open64.c: Likewise.
	* io/open64_2.c: Likewise.
	* io/open_2.c: Likewise.
	* io/openat.c: Likewise.
	* io/openat64.c: Likewise.
	* io/openat64_2.c: Likewise.
	* io/openat_2.c: Likewise.
	* sysdeps/mach/hurd/open.c: Likewise.
	* sysdeps/mach/hurd/openat.c: Likewise.
	* sysdeps/posix/open64.c: Likewise.
	* sysdeps/unix/sysv/linux/dl-openat64.c: Likewise.
	* sysdeps/unix/sysv/linux/generic/open.c: Likewise.
	* sysdeps/unix/sysv/linux/generic/open64.c: Likewise.
	* sysdeps/unix/sysv/linux/open64.c: Likewise.
	* sysdeps/unix/sysv/linux/openat.c: Likewise.
---
 ChangeLog                                | 22 ++++++++++++++++++++++
 io/bits/fcntl2.h                         | 18 +++++++++---------
 io/fcntl.h                               | 14 ++++++++++++--
 io/open.c                                |  4 ++--
 io/open64.c                              |  4 ++--
 io/open64_2.c                            |  4 ++--
 io/open_2.c                              |  4 ++--
 io/openat.c                              |  4 ++--
 io/openat64.c                            |  4 ++--
 io/openat64_2.c                          |  4 ++--
 io/openat_2.c                            |  4 ++--
 sysdeps/mach/hurd/open.c                 |  4 ++--
 sysdeps/mach/hurd/openat.c               |  4 ++--
 sysdeps/posix/open64.c                   |  4 ++--
 sysdeps/unix/sysv/linux/dl-openat64.c    |  2 +-
 sysdeps/unix/sysv/linux/generic/open.c   |  6 +++---
 sysdeps/unix/sysv/linux/generic/open64.c |  4 ++--
 sysdeps/unix/sysv/linux/open64.c         |  4 ++--
 sysdeps/unix/sysv/linux/openat.c         |  6 +++---
 19 files changed, 76 insertions(+), 44 deletions(-)
  

This patch above is a straightforward fix, but Linus asks: why not
invoke va_arg() unconditionally? The 'mode' argument thus read could
not be used by the wrapper, as it could contain garbage, but it could
be passed untouched to the kernel.

I looked at various obscure architectures (but that wasn't an
exhaustive survey), and it does not look like it would ever be a
problem to read one argument past what the caller provided for 2 out
of 3 and 3 out of 4 arguments (which is Linus' contention). It would
make the wrappers simpler and lower the risk of a similar bug in the
future.

The wrappers for fortify would likely continue to check for O_CREAT
and O_TMPFILE (see io/bits/fcntl2.h).

Thoughts?

On Thu, Oct 30, 2014 at 5:21 PM, Eric Rannaud <e@nanocritical.com> wrote:
> Both open and openat load their last argument 'mode' lazily, using
> va_arg() only if O_CREAT is found in oflag. This is wrong, mode is also
> necessary if O_TMPFILE is in oflag.
>
> By chance on x86_64, the problem wasn't evident when using O_TMPFILE
> with open, as the 3rd argument of open, even when not loaded with
> va_arg, is left untouched in RDX, where the syscall expects it.
>
> However, openat was not so lucky, and O_TMPFILE couldn't be used: mode
> is the 4th argument, in RCX, but the syscall expects its 4th argument in
> a different register than the glibc wrapper, in R10.
>
> Introduce a macro __OPEN_NEEDS_MODE (oflag) to test if either O_CREAT or
> O_TMPFILE is set in oflag.
>
> Tested on Linux x86_64.
>
>         [BZ #17523]
>         * io/fcntl.h: Define __OPEN_NEEDS_MODE.
>         * io/bits/fcntl2.h: Use __OPEN_NEEDS_MODE.
>         * io/open.c: Likewise.
>         * io/open64.c: Likewise.
>         * io/open64_2.c: Likewise.
>         * io/open_2.c: Likewise.
>         * io/openat.c: Likewise.
>         * io/openat64.c: Likewise.
>         * io/openat64_2.c: Likewise.
>         * io/openat_2.c: Likewise.
>         * sysdeps/mach/hurd/open.c: Likewise.
>         * sysdeps/mach/hurd/openat.c: Likewise.
>         * sysdeps/posix/open64.c: Likewise.
>         * sysdeps/unix/sysv/linux/dl-openat64.c: Likewise.
>         * sysdeps/unix/sysv/linux/generic/open.c: Likewise.
>         * sysdeps/unix/sysv/linux/generic/open64.c: Likewise.
>         * sysdeps/unix/sysv/linux/open64.c: Likewise.
>         * sysdeps/unix/sysv/linux/openat.c: Likewise.
> ---
>  ChangeLog                                | 22 ++++++++++++++++++++++
>  io/bits/fcntl2.h                         | 18 +++++++++---------
>  io/fcntl.h                               | 14 ++++++++++++--
>  io/open.c                                |  4 ++--
>  io/open64.c                              |  4 ++--
>  io/open64_2.c                            |  4 ++--
>  io/open_2.c                              |  4 ++--
>  io/openat.c                              |  4 ++--
>  io/openat64.c                            |  4 ++--
>  io/openat64_2.c                          |  4 ++--
>  io/openat_2.c                            |  4 ++--
>  sysdeps/mach/hurd/open.c                 |  4 ++--
>  sysdeps/mach/hurd/openat.c               |  4 ++--
>  sysdeps/posix/open64.c                   |  4 ++--
>  sysdeps/unix/sysv/linux/dl-openat64.c    |  2 +-
>  sysdeps/unix/sysv/linux/generic/open.c   |  6 +++---
>  sysdeps/unix/sysv/linux/generic/open64.c |  4 ++--
>  sysdeps/unix/sysv/linux/open64.c         |  4 ++--
>  sysdeps/unix/sysv/linux/openat.c         |  6 +++---
>  19 files changed, 76 insertions(+), 44 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index a28bb3b3452f..b1787fbdceab 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,25 @@
> +2014-10-30  Eric Rannaud  <e@nanocritical.com>
> +
> +       [BZ #17523]
> +       * io/fcntl.h: Define __OPEN_NEEDS_MODE.
> +       * io/bits/fcntl2.h: Use __OPEN_NEEDS_MODE.
> +       * io/open.c: Likewise.
> +       * io/open64.c: Likewise.
> +       * io/open64_2.c: Likewise.
> +       * io/open_2.c: Likewise.
> +       * io/openat.c: Likewise.
> +       * io/openat64.c: Likewise.
> +       * io/openat64_2.c: Likewise.
> +       * io/openat_2.c: Likewise.
> +       * sysdeps/mach/hurd/open.c: Likewise.
> +       * sysdeps/mach/hurd/openat.c: Likewise.
> +       * sysdeps/posix/open64.c: Likewise.
> +       * sysdeps/unix/sysv/linux/dl-openat64.c: Likewise.
> +       * sysdeps/unix/sysv/linux/generic/open.c: Likewise.
> +       * sysdeps/unix/sysv/linux/generic/open64.c: Likewise.
> +       * sysdeps/unix/sysv/linux/open64.c: Likewise.
> +       * sysdeps/unix/sysv/linux/openat.c: Likewise.
> +
>  2014-10-29  Carlos O'Donell  <carlos@redhat.com>
>
>         * manual/llio.texi: Add comments discussing why write() may be
> diff --git a/io/bits/fcntl2.h b/io/bits/fcntl2.h
> index 4f13b1070673..bb8d233b01da 100644
> --- a/io/bits/fcntl2.h
> +++ b/io/bits/fcntl2.h
> @@ -20,7 +20,7 @@
>  # error "Never include <bits/fcntl2.h> directly; use <fcntl.h> instead."
>  #endif
>
> -/* Check that calls to open and openat with O_CREAT set have an
> +/* Check that calls to open and openat with O_CREAT or O_TMPFILE set have an
>     appropriate third/fourth parameter.  */
>  #ifndef __USE_FILE_OFFSET64
>  extern int __open_2 (const char *__path, int __oflag) __nonnull ((1));
> @@ -35,7 +35,7 @@ extern int __REDIRECT (__open_alias, (const char *__path, int __oflag, ...),
>  __errordecl (__open_too_many_args,
>              "open can be called either with 2 or 3 arguments, not more");
>  __errordecl (__open_missing_mode,
> -            "open with O_CREAT in second argument needs 3 arguments");
> +            "open with O_CREAT or O_TMPFILE in second argument needs 3 arguments");
>
>  __fortify_function int
>  open (const char *__path, int __oflag, ...)
> @@ -45,7 +45,7 @@ open (const char *__path, int __oflag, ...)
>
>    if (__builtin_constant_p (__oflag))
>      {
> -      if ((__oflag & O_CREAT) != 0 && __va_arg_pack_len () < 1)
> +      if (__OPEN_NEEDS_MODE (__oflag) && __va_arg_pack_len () < 1)
>         {
>           __open_missing_mode ();
>           return __open_2 (__path, __oflag);
> @@ -67,7 +67,7 @@ extern int __REDIRECT (__open64_alias, (const char *__path, int __oflag,
>  __errordecl (__open64_too_many_args,
>              "open64 can be called either with 2 or 3 arguments, not more");
>  __errordecl (__open64_missing_mode,
> -            "open64 with O_CREAT in second argument needs 3 arguments");
> +            "open64 with O_CREAT or O_TMPFILE in second argument needs 3 arguments");
>
>  __fortify_function int
>  open64 (const char *__path, int __oflag, ...)
> @@ -77,7 +77,7 @@ open64 (const char *__path, int __oflag, ...)
>
>    if (__builtin_constant_p (__oflag))
>      {
> -      if ((__oflag & O_CREAT) != 0 && __va_arg_pack_len () < 1)
> +      if (__OPEN_NEEDS_MODE (__oflag) && __va_arg_pack_len () < 1)
>         {
>           __open64_missing_mode ();
>           return __open64_2 (__path, __oflag);
> @@ -111,7 +111,7 @@ extern int __REDIRECT (__openat_alias, (int __fd, const char *__path,
>  __errordecl (__openat_too_many_args,
>              "openat can be called either with 3 or 4 arguments, not more");
>  __errordecl (__openat_missing_mode,
> -            "openat with O_CREAT in third argument needs 4 arguments");
> +            "openat with O_CREAT or O_TMPFILE in third argument needs 4 arguments");
>
>  __fortify_function int
>  openat (int __fd, const char *__path, int __oflag, ...)
> @@ -121,7 +121,7 @@ openat (int __fd, const char *__path, int __oflag, ...)
>
>    if (__builtin_constant_p (__oflag))
>      {
> -      if ((__oflag & O_CREAT) != 0 && __va_arg_pack_len () < 1)
> +      if (__OPEN_NEEDS_MODE (__oflag) && __va_arg_pack_len () < 1)
>         {
>           __openat_missing_mode ();
>           return __openat_2 (__fd, __path, __oflag);
> @@ -145,7 +145,7 @@ extern int __REDIRECT (__openat64_alias, (int __fd, const char *__path,
>  __errordecl (__openat64_too_many_args,
>              "openat64 can be called either with 3 or 4 arguments, not more");
>  __errordecl (__openat64_missing_mode,
> -            "openat64 with O_CREAT in third argument needs 4 arguments");
> +            "openat64 with O_CREAT or O_TMPFILE in third argument needs 4 arguments");
>
>  __fortify_function int
>  openat64 (int __fd, const char *__path, int __oflag, ...)
> @@ -155,7 +155,7 @@ openat64 (int __fd, const char *__path, int __oflag, ...)
>
>    if (__builtin_constant_p (__oflag))
>      {
> -      if ((__oflag & O_CREAT) != 0 && __va_arg_pack_len () < 1)
> +      if (__OPEN_NEEDS_MODE (__oflag) && __va_arg_pack_len () < 1)
>         {
>           __openat64_missing_mode ();
>           return __openat64_2 (__fd, __path, __oflag);
> diff --git a/io/fcntl.h b/io/fcntl.h
> index cf512dd27f8c..43e2a63343f1 100644
> --- a/io/fcntl.h
> +++ b/io/fcntl.h
> @@ -34,6 +34,15 @@ __BEGIN_DECLS
>     numbers and flag bits for `open', `fcntl', et al.  */
>  #include <bits/fcntl.h>
>
> +/* Detect if open needs mode as a third argument (or for openat as a fourth
> +   argument).  */
> +#ifdef __O_TMPFILE
> +# define __OPEN_NEEDS_MODE(oflag) \
> +  ( ((oflag) & O_CREAT) != 0 || ((oflag) & __O_TMPFILE) == __O_TMPFILE )
> +#else
> +# define __OPEN_NEEDS_MODE(oflag) ( ((oflag) & O_CREAT) != 0 )
> +#endif
> +
>  /* POSIX.1-2001 specifies that these types are defined by <fcntl.h>.
>     Earlier POSIX standards permitted any type ending in `_t' to be defined
>     by any POSIX header, so we don't conditionalize the definitions here.  */
> @@ -160,8 +169,9 @@ typedef __pid_t pid_t;
>  extern int fcntl (int __fd, int __cmd, ...);
>
>  /* Open FILE and return a new file descriptor for it, or -1 on error.
> -   OFLAG determines the type of access used.  If O_CREAT is on OFLAG,
> -   the third argument is taken as a `mode_t', the mode of the created file.
> +   OFLAG determines the type of access used.  If O_CREAT or O_TMPFILE is on
> +   OFLAG, the third argument is taken as a `mode_t', the mode of the created
> +   file.
>
>     This function is a cancellation point and therefore not marked with
>     __THROW.  */
> diff --git a/io/open.c b/io/open.c
> index 24aa38033984..d1df5c38d3fb 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -23,7 +23,7 @@
>  #include <stdio.h>
>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open (file, oflag)
> @@ -38,7 +38,7 @@ __libc_open (file, oflag)
>        return -1;
>      }
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start(arg, oflag);
> diff --git a/io/open64.c b/io/open64.c
> index 3f3d2e8bbd4b..def4e0b1b27d 100644
> --- a/io/open64.c
> +++ b/io/open64.c
> @@ -21,7 +21,7 @@
>  #include <stddef.h>
>  #include <stdio.h>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open64 (file, oflag)
> @@ -36,7 +36,7 @@ __libc_open64 (file, oflag)
>        return -1;
>      }
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/io/open64_2.c b/io/open64_2.c
> index 7cafbba4fcc4..dced8abbe7af 100644
> --- a/io/open64_2.c
> +++ b/io/open64_2.c
> @@ -22,8 +22,8 @@
>  int
>  __open64_2 (const char *file, int oflag)
>  {
> -  if (oflag & O_CREAT)
> -    __fortify_fail ("invalid open64 call: O_CREAT without mode");
> +  if (__OPEN_NEEDS_MODE (oflag))
> +    __fortify_fail ("invalid open64 call: O_CREAT or O_TMPFILE without mode");
>
>    return __open64 (file, oflag);
>  }
> diff --git a/io/open_2.c b/io/open_2.c
> index 65d2c1c845bc..d5b3afe53013 100644
> --- a/io/open_2.c
> +++ b/io/open_2.c
> @@ -22,8 +22,8 @@
>  int
>  __open_2 (const char *file, int oflag)
>  {
> -  if (oflag & O_CREAT)
> -    __fortify_fail ("invalid open call: O_CREAT without mode");
> +  if (__OPEN_NEEDS_MODE (oflag))
> +    __fortify_fail ("invalid open call: O_CREAT or O_TMPFILE without mode");
>
>    return __open (file, oflag);
>  }
> diff --git a/io/openat.c b/io/openat.c
> index 2d822702af43..f3ac8a7532fd 100644
> --- a/io/openat.c
> +++ b/io/openat.c
> @@ -30,7 +30,7 @@ int __have_atfcts;
>  #endif
>
>  /* Open FILE with access OFLAG.  Interpret relative paths relative to
> -   the directory associated with FD.  If OFLAG includes O_CREAT, a
> +   the directory associated with FD.  If O_CREAT or O_TMPFILE is in OFLAG, a
>     third argument is the file protection.  */
>  int
>  __openat (fd, file, oflag)
> @@ -60,7 +60,7 @@ __openat (fd, file, oflag)
>         }
>      }
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/io/openat64.c b/io/openat64.c
> index c0c4e19589bc..d104bc123bb3 100644
> --- a/io/openat64.c
> +++ b/io/openat64.c
> @@ -23,7 +23,7 @@
>  #include <sys/stat.h>
>
>  /* Open FILE with access OFLAG.  Interpret relative paths relative to
> -   the directory associated with FD.  If OFLAG includes O_CREAT, a
> +   the directory associated with FD.  If O_CREAT or O_TMPFILE is in OFLAG, a
>     third argument is the file protection.  */
>  int
>  __openat64 (fd, file, oflag)
> @@ -53,7 +53,7 @@ __openat64 (fd, file, oflag)
>         }
>      }
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/io/openat64_2.c b/io/openat64_2.c
> index 6cfea6a9aace..9c22a28cadc6 100644
> --- a/io/openat64_2.c
> +++ b/io/openat64_2.c
> @@ -22,8 +22,8 @@
>  int
>  __openat64_2 (int fd, const char *file, int oflag)
>  {
> -  if (oflag & O_CREAT)
> -    __fortify_fail ("invalid openat64 call: O_CREAT without mode");
> +  if (__OPEN_NEEDS_MODE (oflag))
> +    __fortify_fail ("invalid openat64 call: O_CREAT or O_TMPFILE without mode");
>
>    return __openat64 (fd, file, oflag);
>  }
> diff --git a/io/openat_2.c b/io/openat_2.c
> index 9e38c142671c..d15d1e922d76 100644
> --- a/io/openat_2.c
> +++ b/io/openat_2.c
> @@ -22,8 +22,8 @@
>  int
>  __openat_2 (int fd, const char *file, int oflag)
>  {
> -  if (oflag & O_CREAT)
> -    __fortify_fail ("invalid openat call: O_CREAT without mode");
> +  if (__OPEN_NEEDS_MODE (oflag))
> +    __fortify_fail ("invalid openat call: O_CREAT or O_TMPFILE without mode");
>
>    return __openat (fd, file, oflag);
>  }
> diff --git a/sysdeps/mach/hurd/open.c b/sysdeps/mach/hurd/open.c
> index 7d9b2de70c38..f003d032e46e 100644
> --- a/sysdeps/mach/hurd/open.c
> +++ b/sysdeps/mach/hurd/open.c
> @@ -22,7 +22,7 @@
>  #include <hurd.h>
>  #include <hurd/fd.h>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open (const char *file, int oflag, ...)
> @@ -30,7 +30,7 @@ __libc_open (const char *file, int oflag, ...)
>    mode_t mode;
>    io_t port;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/sysdeps/mach/hurd/openat.c b/sysdeps/mach/hurd/openat.c
> index 318cb229ef64..83ffe13e5adb 100644
> --- a/sysdeps/mach/hurd/openat.c
> +++ b/sysdeps/mach/hurd/openat.c
> @@ -26,7 +26,7 @@
>  #include <hurd/fd.h>
>
>  /* Open FILE with access OFLAG.  Interpret relative paths relative to
> -   the directory associated with FD.  If OFLAG includes O_CREAT, a
> +   the directory associated with FD.  If O_CREAT or O_TMPFILE is in OFLAG, a
>     third argument is the file protection.  */
>  int
>  __openat (fd, file, oflag)
> @@ -37,7 +37,7 @@ __openat (fd, file, oflag)
>    mode_t mode;
>    io_t port;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/sysdeps/posix/open64.c b/sysdeps/posix/open64.c
> index 64d192af9793..4b7ec362be80 100644
> --- a/sysdeps/posix/open64.c
> +++ b/sysdeps/posix/open64.c
> @@ -19,14 +19,14 @@
>  #include <stdarg.h>
>  #include <sysdep-cancel.h>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open64 (const char *file, int oflag, ...)
>  {
>    int mode = 0;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/sysdeps/unix/sysv/linux/dl-openat64.c b/sysdeps/unix/sysv/linux/dl-openat64.c
> index 9d00b459a60d..5ac16a221315 100644
> --- a/sysdeps/unix/sysv/linux/dl-openat64.c
> +++ b/sysdeps/unix/sysv/linux/dl-openat64.c
> @@ -28,7 +28,7 @@ openat64 (dfd, file, oflag)
>       const char *file;
>       int oflag;
>  {
> -  assert ((oflag & O_CREAT) == 0);
> +  assert (!__OPEN_NEEDS_MODE (oflag));
>
>  #ifdef __NR_openat
>    return INLINE_SYSCALL (openat, 3, dfd, file, oflag | O_LARGEFILE);
> diff --git a/sysdeps/unix/sysv/linux/generic/open.c b/sysdeps/unix/sysv/linux/generic/open.c
> index 4f73fa019cd8..b4c68344c2eb 100644
> --- a/sysdeps/unix/sysv/linux/generic/open.c
> +++ b/sysdeps/unix/sysv/linux/generic/open.c
> @@ -22,14 +22,14 @@
>  #include <stdio.h>
>  #include <sysdep-cancel.h>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open (const char *file, int oflag, ...)
>  {
>    int mode = 0;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> @@ -59,7 +59,7 @@ __open_nocancel (const char *file, int oflag, ...)
>  {
>    int mode = 0;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/sysdeps/unix/sysv/linux/generic/open64.c b/sysdeps/unix/sysv/linux/generic/open64.c
> index 93d79e381fab..faea4df9511b 100644
> --- a/sysdeps/unix/sysv/linux/generic/open64.c
> +++ b/sysdeps/unix/sysv/linux/generic/open64.c
> @@ -22,14 +22,14 @@
>  #include <stdio.h>
>  #include <sysdep-cancel.h>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open64 (const char *file, int oflag, ...)
>  {
>    int mode = 0;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/sysdeps/unix/sysv/linux/open64.c b/sysdeps/unix/sysv/linux/open64.c
> index 0d63806d04ae..6d91b21c3c4f 100644
> --- a/sysdeps/unix/sysv/linux/open64.c
> +++ b/sysdeps/unix/sysv/linux/open64.c
> @@ -21,14 +21,14 @@
>  #include <stdio.h>
>  #include <sysdep-cancel.h>
>
> -/* Open FILE with access OFLAG.  If OFLAG includes O_CREAT,
> +/* Open FILE with access OFLAG.  If O_CREAT or O_TMPFILE is in OFLAG,
>     a third argument is the file protection.  */
>  int
>  __libc_open64 (const char *file, int oflag, ...)
>  {
>    int mode = 0;
>
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> diff --git a/sysdeps/unix/sysv/linux/openat.c b/sysdeps/unix/sysv/linux/openat.c
> index 36555b958fdd..99462dcef0d8 100644
> --- a/sysdeps/unix/sysv/linux/openat.c
> +++ b/sysdeps/unix/sysv/linux/openat.c
> @@ -58,8 +58,8 @@ OPENAT_NOT_CANCEL (fd, file, oflag, mode)
>
>
>  /* Open FILE with access OFLAG.  Interpret relative paths relative to
> -   the directory associated with FD.  If OFLAG includes O_CREAT, a
> -   third argument is the file protection.  */
> +   the directory associated with FD.  If OFLAG includes O_CREAT or
> +   O_TMPFILE, a fourth argument is the file protection.  */
>  int
>  __OPENAT (fd, file, oflag)
>       int fd;
> @@ -67,7 +67,7 @@ __OPENAT (fd, file, oflag)
>       int oflag;
>  {
>    mode_t mode = 0;
> -  if (oflag & O_CREAT)
> +  if (__OPEN_NEEDS_MODE (oflag))
>      {
>        va_list arg;
>        va_start (arg, oflag);
> --
> 2.1.2
>
  

On Thu, Oct 30, 2014 at 08:06:26PM -0700, Eric Rannaud wrote:
> This patch above is a straightforward fix, but Linus asks: why not
> invoke va_arg() unconditionally? The 'mode' argument thus read could
> not be used by the wrapper, as it could contain garbage, but it could
> be passed untouched to the kernel.

This is valid only if you treat translation units as black boxes. If
they're not (think: LTO on glibc, which perhaps isn't supported now
but could be in the future) then all sorts of things could go wrong at
the compiler level that have nothing to do with whether this would
conceptually work at the ABI level. The most likely error seems to be
a compiler warning/error meant to catch this exact problem: calling
va_arg without a matching argument.

In principle the same could also happen at the hardware/ABI level on
some new arch; I'm kinda surprised if it doesn't already happen on
IA64.

Rich
  

* Eric Rannaud:

> This patch above is a straightforward fix, but Linus asks: why not
> invoke va_arg() unconditionally? The 'mode' argument thus read could
> not be used by the wrapper, as it could contain garbage, but it could
> be passed untouched to the kernel.

If technically undefined behavior which just happens to work is
acceptable, you could also add the optional argument to the argument
list.  This would address the performance concern which may have been
the reason to make the va_arg call conditional.

Anyway, I think the definition of __OPEN_NEEDS_MODE is still
incorrect.  It should check for any unknown flags in addition to
O_CREAT and O_TMPFILE.  Otherwise, we'll run into the same problem
again.
  

Florian Weimer <fw@deneb.enyo.de> writes:

> Anyway, I think the definition of __OPEN_NEEDS_MODE is still
> incorrect.  It should check for any unknown flags in addition to
> O_CREAT and O_TMPFILE.  Otherwise, we'll run into the same problem
> again.

That will be incompatible with the fortify definition in
<bits/fcntl2.h>.

Andreas.
  

On Sun, Nov 02, 2014 at 04:59:35PM +0100, Florian Weimer wrote:
> * Eric Rannaud:
> 
> > This patch above is a straightforward fix, but Linus asks: why not
> > invoke va_arg() unconditionally? The 'mode' argument thus read could
> > not be used by the wrapper, as it could contain garbage, but it could
> > be passed untouched to the kernel.
> 
> If technically undefined behavior which just happens to work is
> acceptable, you could also add the optional argument to the argument
> list.  This would address the performance concern which may have been
> the reason to make the va_arg call conditional.

I don't think it's acceptable.

> Anyway, I think the definition of __OPEN_NEEDS_MODE is still
> incorrect.  It should check for any unknown flags in addition to
> O_CREAT and O_TMPFILE.  Otherwise, we'll run into the same problem
> again.

It's unlikely that such a flag will ever be added again, and it really
should not have happened with O_TMPFILE. O_TMPFILE is fundamentally a
"create" operation and should have either requried O_CREAT with it, or
had O_CREAT built into the bits of its definition. This was an
oversight on the part of the kernel folks when they added it.

Rich
  

On Nov 2, 2014 7:59 AM, "Florian Weimer" <fw@deneb.enyo.de> wrote:
> Anyway, I think the definition of __OPEN_NEEDS_MODE is still
> incorrect.  It should check for any unknown flags in addition to
> O_CREAT and O_TMPFILE.  Otherwise, we'll run into the same problem
> again.

Checking for any unknown bit, not having any idea whether it means 'mode' is actually needed, seems like a weird middle ground between the current code and unconditionally reading the mode argument.

That is, if unknown bits are found, read mode without proper cause and invoke undefined behavior.

Could we have the kernel define __OPEN_NEEDS_MODE() for us? Do we have examples of the kernel headers defining that kind of macro? We could either use it directly, or check that the glibc definition is up-to-date in a test.

The kernel actually had a very similar bug in the early history of O_TMPFILE: some code was only checking for O_CREAT. So maybe such a macro would be useful for them too, and they could fairly easily make sure they keep that macro up to date as well.
  

On Sun, Nov 2, 2014 at 8:44 AM, Rich Felker <dalias@libc.org> wrote:
> It's unlikely that such a flag will ever be added again, and it really
> should not have happened with O_TMPFILE. O_TMPFILE is fundamentally a
> "create" operation and should have either requried O_CREAT with it, or
> had O_CREAT built into the bits of its definition. This was an
> oversight on the part of the kernel folks when they added it.

For reference, that was actually entirely deliberate (original
discussion at http://thread.gmane.org/gmane.linux.file-systems/76261).
They were looking for a flag combination that would guarantee that
O_TMPFILE would fail on older kernels.

When using O_TMPFILE to implement a secure temporary file facility,
you don't want
        fd = open("$TMPDIR", O_TPMFILE, 0600);
to succeed as a regular O_CREAT on an older kernel that doesn't know
about O_TMPFILE, yet finds the O_CREAT bit in flags, and creates a
visible file, when TMPDIR doesn't exist (any manual check beforehand
would necessarily be racy). open() has traditionally never checked for
unknown bits set in flags.

Some have argued (then and since) that this should have been a hint
that open() was the wrong place to add such a facility, and that a
tmpfile() syscall should have been added instead, with its own
separate semantics.
  

On 11/02/2014 05:44 PM, Rich Felker wrote:
> On Sun, Nov 02, 2014 at 04:59:35PM +0100, Florian Weimer wrote:
>> * Eric Rannaud:
>>
>>> This patch above is a straightforward fix, but Linus asks: why not
>>> invoke va_arg() unconditionally? The 'mode' argument thus read could
>>> not be used by the wrapper, as it could contain garbage, but it could
>>> be passed untouched to the kernel.
>>
>> If technically undefined behavior which just happens to work is
>> acceptable, you could also add the optional argument to the argument
>> list.  This would address the performance concern which may have been
>> the reason to make the va_arg call conditional.
>
> I don't think it's acceptable.

I've asked on gcc-help:

   <http://gcc.gnu.org/ml/gcc-help/2014-12/msg00075.html>
  

On Fri, Dec 12, 2014 at 2:50 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>>> This patch above is a straightforward fix, but Linus asks: why not
>>>> invoke va_arg() unconditionally? The 'mode' argument thus read could
>>>> not be used by the wrapper, as it could contain garbage, but it could
>>>> be passed untouched to the kernel.
>>>
>>> If technically undefined behavior which just happens to work is
>>> acceptable, you could also add the optional argument to the argument
>>> list.  This would address the performance concern which may have been
>>> the reason to make the va_arg call conditional.
>>
>> I don't think it's acceptable.
>
> I've asked on gcc-help:
>   <http://gcc.gnu.org/ml/gcc-help/2014-12/msg00075.html>

I read this thread to mean that their answer is no, there is no GCC
portable way to play fast and loose with the optional argument, and
that we should stick with the current proposed patch?

My FSF papers are finally in. I will resubmit the latest version of
the patch shortly.