[proposal] use mprotect() in code generation

Sam_Hocevar · March 1, 2006, 11:01pm

This patch uses mprotect() on the page containing the dynamically
generated code from SDL_stretch.c. It is useful on systems that enforce
a security policy such as forbidding writable pages to be executed.

Ideally this patch should check for various headers on the system at
the configure step, but I just hardcoded a #ifdef linux test for
now. Let me know if there is interest in it and I will send a polished
patch.

Regards,–
Sam.
-------------- next part --------------
diff -puriN SDL-1.2.9.orig/src/video/SDL_stretch.c SDL-1.2.9/src/video/SDL_stretch.c
— SDL-1.2.9.orig/src/video/SDL_stretch.c 2004-05-16 23:08:55 +0200
+++ SDL-1.2.9/src/video/SDL_stretch.c 2006-03-01 13:28:46 +0100
@@ -43,10 +43,23 @@ static char rcsid =
!defined(WATCOMC) && !defined(LCC) && !defined(FREEBCC)) ||
(defined(i386) && defined(GNUC) && defined(USE_ASMBLIT))
#define USE_ASM_STRETCH
+#if defined(linux)
+#define USE_MPROTECT
+#endif
#endif

#ifdef USE_ASM_STRETCH

+#ifdef USE_MPROTECT
+#include <stdlib.h>
+#include <stdint.h>
+#include <limits.h>
+#ifndef PAGESIZE
+#define PAGESIZE 4096
+#endif
+#include <sys/mman.h>
+#endif
+
#if defined(WIN32) || defined(i386)
#define PREFIX16 0x66
#define STORE_BYTE 0xAA
@@ -58,10 +71,16 @@ static char rcsid =
#error Need assembly opcodes for this architecture
#endif

+#define MAX_CODE_LENGTH 4096
+
#if defined(ELF) && defined(GNUC)
-extern unsigned char _copy_row[4096] attribute ((alias (“copy_row”)));
+extern unsigned char *_copy_row attribute ((alias (“copy_row”)));
+#endif
+#ifdef USE_MPROTECT
+static unsigned char *copy_row;
+#else
+static unsigned char copy_row[MAX_CODE_LENGTH];
#endif
-static unsigned char copy_row[4096];

static int generate_rowbytes(int src_w, int dst_w, int bpp)
{
@@ -70,12 +89,25 @@ static int generate_rowbytes(int src_w,
int src_w;
int dst_w;
} last;
+#ifdef USE_MPROTECT

static unsigned char *code_buffer;
+#endif

int i;
int pos, inc;
unsigned char *eip;
unsigned char load, store;

+#ifdef USE_MPROTECT

if ( ! copy_row ) {

  code_buffer = malloc(MAX_CODE_LENGTH + PAGESIZE - 1);

```
  if ( ! code_buffer ) {
```
```
  	return(-1);
```
```
  }
```

  copy_row = (unsigned char *)(((uintptr_t)code_buffer + PAGESIZE - 1) & ~(uintptr_t)(PAGESIZE - 1));

}
+#endif
/* See if we need to regenerate the copy buffer */
if ( (src_w == last.src_w) &&
(dst_w == last.dst_w) && (bpp == last.bpp) ) {
@@ -99,6 +131,9 @@ static int generate_rowbytes(int src_w,
SDL_SetError(“ASM stretch of %d bytes isn’t supported\n”, bpp);
return(-1);
}
+#ifdef USE_MPROTECT
mprotect(copy_row, MAX_CODE_LENGTH, PROT_READ|PROT_WRITE);
+#endif
pos = 0x10000;
inc = (src_w << 16) / dst_w;
eip = copy_row;
@@ -119,10 +154,13 @@ static int generate_rowbytes(int src_w,
*eip++ = RETURN;

/* Verify that we didn’t overflow (too late) */

if ( eip > (copy_row+sizeof(copy_row)) ) {

if ( eip > (copy_row+MAX_CODE_LENGTH) ) {
SDL_SetError(“Copy buffer overflow”);
return(-1);
}
+#ifdef USE_MPROTECT
mprotect(copy_row, MAX_CODE_LENGTH, PROT_READ|PROT_EXEC);
+#endif
return(0);
}

@@ -284,15 +322,20 @@ int SDL_SoftStretch(SDL_Surface *src, SD
copy_row3(srcp, srcrect->w, dstp, dstrect->w);
break;
default:

```
  {
```

+#ifdef USE_MPROTECT

```
  	void *code = copy_row;
```

+#else

```
  	void *code = &copy_row;
```

+#endif
#ifdef GNUC
asm volatile (

```
  	"call _copy_row"
```

  	"call %4"
  	: "=&D" (u1), "=&S" (u2)

```
  	: "0" (dstp), "1" (srcp)
```

  	: "0" (dstp), "1" (srcp), "r" (code)
  	: "memory" );

#else
#ifdef WIN32

  { void *code = &copy_row;
  	__asm {
  		push edi
  		push esi

@@ -304,11 +347,11 @@ int SDL_SoftStretch(SDL_Surface *src, SD
pop esi
pop edi
}

```
  }
```

#else
#error Need inline assembly for this compiler
#endif
#endif /* GNUC */

```
  }
  	break;
  }
```

#else

Mike_Frysinger · March 1, 2006, 11:15pm

This patch uses mprotect() on the page containing the dynamically
generated code from SDL_stretch.c. It is useful on systems that enforce
a security policy such as forbidding writable pages to be executed.

some notes:

you should redo your patch against cvs as your changes conflict with at
least one patch ive gotten integrated so far (the inline asm call)
shouldnt you also pass PROT_EXEC to mprotect() ?
you assume copy_row is initialized to NULL for you … seems like a bad
assumption to make … should rather do i think:
+static unsigned char *copy_row = NULL;

Ideally this patch should check for various headers on the system at
the configure step, but I just hardcoded a #ifdef linux test for
now. Let me know if there is interest in it and I will send a polished
patch.

i started to write something similar but ended up not bothering since
SDL_Stretch is hardly used in projects … but Gentoo would be interested in
such a patch done properly (i.e. use configure to look for mprotect()) …
-mikeOn Wednesday 01 March 2006 18:01, Sam Hocevar wrote:

Sam_Hocevar · March 2, 2006, 7:03pm

some notes:

you should redo your patch against cvs as your changes conflict with at
least one patch ive gotten integrated so far (the inline asm call)

You’re right. Sorry about the other ones that did not apply well, I
will try to fix everything.

shouldnt you also pass PROT_EXEC to mprotect() ?

Well yes, and that’s exactly what the patch does

you assume copy_row is initialized to NULL for you … seems like a bad
assumption to make … should rather do i think:
+static unsigned char *copy_row = NULL;

It seems to me like a perfectly valid assumption to make since the
C standard guarantees that static variables are initialised to all
zeroes. I do agree though that for the sake of clarity and to please the
nitpickers claiming that NULL can have non-zero bits, your proposal is
better.

Regards,On Wed, Mar 01, 2006, Mike Frysinger wrote:

Sam.

Mike_Frysinger · March 3, 2006, 1:04am

shouldnt you also pass PROT_EXEC to mprotect() ?

Well yes, and that’s exactly what the patch does

indeed … i saw the first mprotect which doesnt use PROT_EXEC but missed the
second mprotect which does use PROT_EXEC

you assume copy_row is initialized to NULL for you … seems like a
bad assumption to make … should rather do i think:
+static unsigned char *copy_row = NULL;

It seems to me like a perfectly valid assumption to make since the
C standard guarantees that static variables are initialised to all
zeroes. I do agree though that for the sake of clarity and to please the
nitpickers claiming that NULL can have non-zero bits, your proposal is
better.

fair enough on both points … guess final call is up to Sam or Ryan (or
whoever commits the code)
-mikeOn Thursday 02 March 2006 14:03, Sam Hocevar wrote:

On Wed, Mar 01, 2006, Mike Frysinger wrote: