[Mesa-dev] [wip 3/9] glsl: memory_map helper class for data deserialization

[Paul Berry]

On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com> wrote:

> Class will be used by the shader binary cache implementation.
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/glsl/memory_map.h | 174
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100644 src/glsl/memory_map.h
>
> diff --git a/src/glsl/memory_map.h b/src/glsl/memory_map.h
> new file mode 100644
> index 0000000..1b68b72
> --- /dev/null
> +++ b/src/glsl/memory_map.h
> @@ -0,0 +1,174 @@
> +/* -*- c++ -*- */
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#pragma once
> +#ifndef MEMORY_MAP_H
> +#define MEMORY_MAP_H
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +
> +#ifdef __cplusplus
> +
> +/**
> + * Helper class to read data
> + *
> + * Class can read either from user given memory or from a file. On Linux
> + * file reading wraps around the Posix functions for mapping a file into
> + * the process's address space. Other OS may need different
> implementation.
> + */
> +class memory_map
> +{
> +public:
> +   memory_map() :
> +      mode(memory_map::READ_MEM),
> +      fd(0),
> +      cache_size(0),
> +      cache_mmap(NULL),
> +      cache_mmap_p(NULL)
> +   {
> +      /* only used by read_string() */
> +      mem_ctx = ralloc_context(NULL);
> +   }
> +
> +   /* read from disk */
> +   int map(const char *path)
> +   {
> +      struct stat stat_info;
> +      if (stat(path, &stat_info) != 0)
> +         return -1;
>

As before, I'm not thrilled with the use of -1 to mean failure and 0 to
mean success, because it forces the caller to use counterintuitive if
statements.  I'd prefer for map() to return a bool with true meaning
success and false meaning failure.


> +
> +      mode = memory_map::READ_MAP;
> +      cache_size = stat_info.st_size;
> +
> +      fd = open(path, O_RDONLY);
> +      if (fd) {
> +         cache_mmap_p = cache_mmap = (char *)
> +            mmap(NULL, cache_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +         return (cache_mmap == MAP_FAILED) ? -1 : 0;
>

MAP_FAILED is a nonzero value, so if this error condition ever occurs, the
destructor will errneously try to call munmap().

What I'd recommend doing instead is:

void *mmap_result = mmap(...);
if (mmap_result == MAP_FAILED) {
   close(fd);
   return -1;
}
cache_mmap_p = cache_mmap = (char *) mmap_result;
return 0;

>
> +      }
> +      return -1;
> +   }
> +
> +   /* read from memory */
> +   int map(const void *memory, size_t size)
> +   {
> +      cache_mmap_p = cache_mmap = (char *) memory;
> +      cache_size = size;
> +      return 0;
> +   }
>

IMHO, functions that cannot fail should return void.


> +
> +   /* wrap a portion from another map */
> +   int map(memory_map &map, size_t size)
> +   {
> +      cache_mmap_p = cache_mmap = map.cache_mmap_p;
> +      cache_size = size;
> +      map.ffwd(size);
> +      return 0;
> +   }
> +
> +   ~memory_map() {
> +      if (cache_mmap && mode == READ_MAP) {
> +         munmap(cache_mmap, cache_size);
> +         close(fd);
> +      }
> +      ralloc_free(mem_ctx);
> +   }
> +
> +   /* move read pointer forward */
> +   inline void ffwd(int len)
> +   {
> +      cache_mmap_p += len;
> +   }
> +
> +   inline void jump(unsigned pos)
> +   {
> +      cache_mmap_p = cache_mmap + pos;
> +   }
> +
> +
> +   /* position of read pointer */
> +   inline uint32_t position()
> +   {
> +      return cache_mmap_p - cache_mmap;
> +   }
> +
> +   inline char *read_string()
> +   {
> +      char *str = ralloc_strdup(mem_ctx, cache_mmap_p);
> +      ffwd(strlen(str)+1);
> +      return str;
>

This is problematic from a security perspective.  If the client provides
corrupted data that ends in a truncated string (lacking a null terminator)
that could cause ralloc_strdup() to try to read beyond the end of the
file.  We need to make sure the code doesn't try to read beyond the end of
file, even if the data is corrupted or truncated.

My recommendation would be:

- Have a boolean in the memory_map class to tell whether an error has
occurred.
- If we ever try to read beyond the end of the file, set the boolean and
return ralloc_strdup("").
- At the end of deserialization, check the boolean to see if an error
occurred.


> +   }
> +
> +/**
> + * read functions per type
> + */
> +#define DECL_READER(type) type read_ ##type () {\
> +   ffwd(sizeof(type));\
> +   return *(type *) (cache_mmap_p - sizeof(type));\
>

This is a similar security issue if the input is truncated.  If
cache_mmap_p points beyond the end of the file after the call to ffwd(), we
should set the error boolean and return 0.


> +}
> +
> +   DECL_READER(int32_t);
> +   DECL_READER(int64_t);
> +   DECL_READER(uint8_t);
> +   DECL_READER(uint32_t);
> +
> +   inline uint8_t read_bool()
> +   {
> +      return read_uint8_t();
> +   }
> +
> +   inline void read(void *dst, size_t size)
> +   {
> +      memcpy(dst, cache_mmap_p, size);
> +      ffwd(size);
>

Similar security issue here.


> +   }
> +
> +   /* total size of mapped memory */
> +   inline int32_t size()
> +   {
> +      return cache_size;
> +   }
> +
> +private:
> +
> +   void *mem_ctx;
> +
> +   /* specifies if we are reading mapped memory or user passed mem */
> +   enum read_mode {
> +      READ_MEM = 0,
> +      READ_MAP
> +   };
> +
> +   int32_t mode;
> +   int32_t fd;
> +   int32_t cache_size;
> +   char *cache_mmap;
> +   char *cache_mmap_p;
> +};
> +#endif /* ifdef __cplusplus */
> +
> +#endif /* MEMORY_MAP_H */
> --
> 1.8.3.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140113/7494639c/attachment-0001.html>