On 23.01.2016 01:22, Nicolai Hähnle wrote:
On 22.01.2016 16:02, Kenneth Graunke wrote:
On Friday, January 22, 2016 12:09:18 PM PST Nicolai Hähnle wrote:
On 22.01.2016 02:53, Jordan Justen wrote:
Juha-Pekka found this back in May 2015:
<1430915727-28677-1-git-send-email-juhapekka.heikk...@gmail.com>
From the discussion, obviously it would be preferable to make
ralloc_size no longer return zeroed memory, but Juha-Pekka found that
it would break Mesa.
Hi, just to comment here. I did make set of patches which fixed all the
breaking places for Mesa. Tapani then pointed the problem I can only
test my patches on Intel HW, more so I have only SNB and IVB on my desk.
Making real fix here would be source of all kind of really nasty bugs on
different HW for some time to come thus I dropped the ball on the fixes.
In the end for Mesa on Intel HW there was not so much to fix but to find
all the correct places to fix require running lot of Valgrind.
I seem to have missed the patch where I was cc'd, sorry about that.
/Juha-Pekka
For now, let's point out the flaw, and stop doing the double zeroing
of rzalloc buffers.
Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Juha-Pekka Heikkila <juhapekka.heikk...@gmail.com>
Cc: Kenneth Graunke <kenn...@whitecape.org>
---
For a release build, I saw the code size shrink by 64 bytes.
src/util/ralloc.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/util/ralloc.c b/src/util/ralloc.c
index 6d4032b..24c1eee 100644
--- a/src/util/ralloc.c
+++ b/src/util/ralloc.c
@@ -49,6 +49,14 @@ _CRTIMP int _vscprintf(const char *format, va_list
argptr);
#endif
#endif
+/* ralloc_size has always used calloc to allocate memory. This has
allowed
+ * code using ralloc_size to depend on always receiving a cleared
buffer.
+ *
+ * FIXME: Clean up the code base to allow this to be set to false, and
then
+ * remove it altogether.
+ */
+static const bool always_allocate_zeroed_memory = true;
+
#define CANARY 0x5A1106
struct ralloc_header
@@ -110,7 +118,10 @@ ralloc_context(const void *ctx)
void *
ralloc_size(const void *ctx, size_t size)
{
- void *block = calloc(1, size + sizeof(ralloc_header));
+ void *block =
+ always_allocate_zeroed_memory ?
+ calloc(1, size + sizeof(ralloc_header)) :
+ malloc(size + sizeof(ralloc_header));
There's an integer overflow here which would be good to fix.
Please explain? ralloc_header is 40-44 bytes - the only way this will
overflow is if you asked for an absurd amount of memory (already near
the max value of size_t). And, if you did, I'm not sure what we're
supposed to do about it...
A common method of triggering buffer overflows leading to security
exploits is that the attacker sets an absurdly large buffer size
somewhere - so large that additional calculations that increase the
buffer size wrap around and result in a very small successful
allocation. The code will then write memory based on the original,
absurdly large buffer size. This means writing beyond the end of the
allocated buffer.
The people who look for security exploits for a living are really good
at finding ways in which such a situation can then be used to hijack the
control flow of the program somehow (for example, targeted overwriting
of the internal metadata of the heap... sounds crazy, but it's used) to
do more or less whatever they want.
I think a decent way to protect against it is something like:
/* Overflow of unsigned integer types is well defined */
if (size + sizeof(ralloc_header) < size)
return NULL;
or perhaps
if (size > SIZE_MAX - sizeof(ralloc_header))
return NULL;
Newer versions of gcc have nicer builtins for arithmetic with overflow
check, but I don't know if we want to depend on those being available.
The hope is that the calling code properly handles allocation failure.
Even if it doesn't, the result is much more likely to just be a segfault
before anything dangerous can happen.
One may say that all of this depends on an attacker gaining access to
Mesa, but WebGL is a thing, so...
Cheers,
Nicolai
Since it was there already in the older version, the patch is
Reviewed-by: Nicolai Hähnle <nicolai.haeh...@amd.com>
as is.
ralloc_header *info;
ralloc_header *parent;
@@ -132,7 +143,7 @@ void *
rzalloc_size(const void *ctx, size_t size)
{
void *ptr = ralloc_size(ctx, size);
- if (likely(ptr != NULL))
+ if (!always_allocate_zeroed_memory && likely(ptr != NULL))
memset(ptr, 0, size);
return ptr;
}
Dropping the memset seems reasonable. I would prefer it if we simply
moved the contents of ralloc_size into rzalloc_size, and made
ralloc_size call rzalloc_size with a comment.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
