Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Szabolcs Nagy <szabolcs dot nagy at arm dot com>]

On 19/06/18 10:56, Florian Weimer wrote:
> 2018-06-19  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #23313]
> 	* libio/stdfiles.c (STDFILE_SECTION): Define macro.
> 	(DEF_STDFILE): Use it.
> 	* libio/vtables.c [SHARED] (__libc_IO_stdfiles): Declare as symbol
> 	set.
> 	[SHARED] (interposed_stdfiles_variable): New function.
> 	(stdfiles_interposed): Likewise.
> 	(_IO_vtable_check): Call stdfiles_interposed.
>
> diff --git a/libio/stdfiles.c b/libio/stdfiles.c
> index 18e1172ad0..2435f412f2 100644
> --- a/libio/stdfiles.c
> +++ b/libio/stdfiles.c
> @@ -33,11 +33,19 @@
>   
>   #include "libioP.h"
>   
> +#ifdef SHARED
> +/* Place the variables defined below in a separate section for the
> +   interposition check in vtables.c.  */
> +# define STDFILE_SECTION __attribute__ ((section ("__libc_IO_stdfiles")))
> +#else
> +# define STDFILE_SECTION
> +#endif
> +
>   #define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \
>     static _IO_lock_t _IO_stdfile_##FD##_lock = _IO_lock_initializer; \
>     static struct _IO_wide_data _IO_wide_data_##FD \
>       = { ._wide_vtable = &_IO_wfile_jumps }; \
> -  struct _IO_FILE_plus NAME \
> +  struct _IO_FILE_plus NAME STDFILE_SECTION \
>       = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, &_IO_wide_data_##FD), \
>          &_IO_file_jumps};
>   
> diff --git a/libio/vtables.c b/libio/vtables.c
> index 9fd4ccf642..aba414ef2d 100644
> --- a/libio/vtables.c
> +++ b/libio/vtables.c
> @@ -29,11 +29,44 @@ void (*IO_accept_foreign_vtables) (void) attribute_hidden;
>   extern struct dl_open_hook *_dl_open_hook;
>   libc_hidden_proto (_dl_open_hook);
>   
> +/* Used to detect interposition of _IO_2_1_stdin_, _IO_2_1_stdout_,
> +   _IO_2_1_stderr_.  See stdfiles.c.  */
> +symbol_set_declare (__libc_IO_stdfiles)
> +
> +/* Check whether the standard file variable at *PTR has been
> +   interposed, by comparing its address against the start and end of
> +   the __libc_IO_stdfiles section.  */
> +static inline bool
> +interposed_stdfiles_variable (void *ptr)
> +{
> +  uintptr_t uptr = (uintptr_t) ptr; /* Avoid ptrdiff_t overflow.  */
> +  uintptr_t length = __stop___libc_IO_stdfiles - __start___libc_IO_stdfiles;
> +  uintptr_t start = uptr - (uintptr_t) __start___libc_IO_stdfiles;
> +  return start >= length;
> +}
> +
> +/* Return true if any of the standard variable definitions for stdin,
> +   stdout, stderr have been interposed.  */
> +static inline bool
> +stdfiles_interposed (void)
> +{
> +  return interposed_stdfiles_variable (&_IO_2_1_stdin_)
> +    || interposed_stdfiles_variable (&_IO_2_1_stdout_)
> +    || interposed_stdfiles_variable (&_IO_2_1_stderr_);
> +}

the check looks ok to me
i think hidden symbol alias would work too

return &_IO_2_1_stdin == &_IO_2_1_stdin_internal_alias || ...;

and it may be a bit nicer than checking the section.. i'm not sure