Re: [PATCH] fopencookie: Mangle function pointers stored on the heap [BZ #20222]

[Carlos O'Donell <carlos at redhat dot com>]

On 06/10/2016 06:49 AM, Florian Weimer wrote:
> 2016-06-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #20222]
> 	* libio/iofopncook.c (_IO_cookie_read): Demangle callback pointer.
> 	(_IO_cookie_write): Likewise.
> 	(_IO_cookie_seek): Likewise.
> 	(_IO_cookie_close): Likewise.
> 	(_IO_old_cookie_seek): Likewise.
> 	(set_callbacks): New function.
> 	(_IO_cookie_init): Call set_callbacks to copy callbacks.

A couple of questions...
 
> diff --git a/libio/iofopncook.c b/libio/iofopncook.c
> index 9eda7c1..487297c 100644
> --- a/libio/iofopncook.c
> +++ b/libio/iofopncook.c
> @@ -43,25 +43,29 @@ static _IO_ssize_t
>  _IO_cookie_read (_IO_FILE *fp, void *buf, _IO_ssize_t size)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_read_function_t *read_cb = cfile->__io_functions.read;
> +  PTR_DEMANGLE (read_cb);
>  
> -  if (cfile->__io_functions.read == NULL)
> +  if (read_cb == NULL)
>      return -1;
>  
> -  return cfile->__io_functions.read (cfile->__cookie, buf, size);
> +  return read_cb (cfile->__cookie, buf, size);
>  }
>  
>  static _IO_ssize_t
>  _IO_cookie_write (_IO_FILE *fp, const void *buf, _IO_ssize_t size)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_write_function_t *write_cb = cfile->__io_functions.write;
> +  PTR_DEMANGLE (write_cb);
>  
> -  if (cfile->__io_functions.write == NULL)
> +  if (write_cb == NULL)
>      {
>        fp->_flags |= _IO_ERR_SEEN;
>        return 0;
>      }
>  
> -  _IO_ssize_t n = cfile->__io_functions.write (cfile->__cookie, buf, size);
> +  _IO_ssize_t n = write_cb (cfile->__cookie, buf, size);
>    if (n < size)
>      fp->_flags |= _IO_ERR_SEEN;
>  
> @@ -72,9 +76,11 @@ static _IO_off64_t
>  _IO_cookie_seek (_IO_FILE *fp, _IO_off64_t offset, int dir)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_seek_function_t *seek_cb = cfile->__io_functions.seek;
> +  PTR_DEMANGLE (seek_cb);
>  
> -  return ((cfile->__io_functions.seek == NULL
> -	   || (cfile->__io_functions.seek (cfile->__cookie, &offset, dir)
> +  return ((seek_cb == NULL
> +	   || (seek_cb (cfile->__cookie, &offset, dir)
>  	       == -1)
>  	   || offset == (_IO_off64_t) -1)
>  	  ? _IO_pos_BAD : offset);
> @@ -84,11 +90,13 @@ static int
>  _IO_cookie_close (_IO_FILE *fp)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> +  cookie_close_function_t *close_cb = cfile->__io_functions.close;
> +  PTR_DEMANGLE (close_cb);
>  
> -  if (cfile->__io_functions.close == NULL)
> +  if (close_cb == NULL)
>      return 0;
>  
> -  return cfile->__io_functions.close (cfile->__cookie);
> +  return close_cb (cfile->__cookie);
>  }
>  
>  
> @@ -126,6 +134,19 @@ static const struct _IO_jump_t _IO_cookie_jumps = {
>  };
>  
>  
> +/* Copy the callbacks from SOURCE to *TARGET, with pointer
> +   mangling.  */
> +static void
> +set_callbacks (_IO_cookie_io_functions_t *target,
> +	       _IO_cookie_io_functions_t source)
> +{
> +  PTR_MANGLE (source.read);
> +  PTR_MANGLE (source.write);
> +  PTR_MANGLE (source.seek);
> +  PTR_MANGLE (source.close);

Isn't this modifying the user's copy of the callbacks
passed in during the call to fopencookie?

If it is, then that might be unexpected. I expect that we
need to copy the function pointers and then encrypt them.
Leaving the users copy untouched.

In fact I'd go as far to say that stdio-common/tst-cookie.c
should be modified to verify that 'cookie_io_functions_t fcts'
is unmodified after the call to fopencookie.

> +  *target = source;
> +}
> +
>  void
>  _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write,
>  		 void *cookie, _IO_cookie_io_functions_t io_functions)
> @@ -134,7 +155,7 @@ _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write,
>    _IO_JUMPS (&cfile->__fp) = &_IO_cookie_jumps;
>  
>    cfile->__cookie = cookie;
> -  cfile->__io_functions = io_functions;
> +  set_callbacks (&cfile->__io_functions, io_functions);
>  
>    _IO_file_init (&cfile->__fp);
>  
> @@ -205,14 +226,14 @@ attribute_compat_text_section
>  _IO_old_cookie_seek (_IO_FILE *fp, _IO_off64_t offset, int dir)
>  {
>    struct _IO_cookie_file *cfile = (struct _IO_cookie_file *) fp;
> -  int (*seek) (_IO_FILE *, _IO_off_t, int);
> -  int ret;
> +  int (*seek_cb) (_IO_FILE *, _IO_off_t, int)
> +    = (int (*)(_IO_FILE *, _IO_off_t, int)) cfile->__io_functions.seek;;
> +  PTR_DEMANGLE (seek_cb);
>  
> -  seek = (int (*)(_IO_FILE *, _IO_off_t, int)) cfile->__io_functions.seek;
> -  if (seek == NULL)
> +  if (seek_cb == NULL)
>      return _IO_pos_BAD;
>  
> -  ret = seek (cfile->__cookie, offset, dir);
> +  int ret = seek_cb (cfile->__cookie, offset, dir);
>  
>    return (ret == -1) ? _IO_pos_BAD : ret;
>  }
> 


-- 
Cheers,
Carlos.