Bug 64573

Summary: mmap with PROT_NONE, but still could be read
Product: Base System Reporter: gordon <hddai>
Component: kernAssignee: Alan Cox <alc>
Status: Closed FIXED    
Severity: Affects Only Me    
Priority: Normal    
Version: Unspecified   
Hardware: Any   
OS: Any   

Description gordon 2004-03-22 16:00:35 UTC
I have ported a test program for LSB test suites to test the compatibility of mmap system call. As to Open Group Technical Standard System Interfaces and Headers, Issue 5, when mmap with PROT_NONE, the  mapped pages should not be allowed to access. But as the program shows, the mapped page could still be read. Can anyone tell me how to solve this problem?
#include <stdio.h>
#include <unistd.h>
#include <setjmp.h>
#include <signal.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>

static int vsrt_mmap_setup(void);
static void vsrt_mmapsig_fn(int);
static void vsrt_invalid_sig_fn(int);
static int vsrt_signal(int , void (*handler)(int));
static int vsrt_open_file(char* s, int flag, mode_t mode, int npages);
typedef struct {
		char	name[10];
		int	signal;
		int	flags;
	} vsrt_siglst;
static vsrt_siglst	vsrt_signals[] = {
		{ "SIGFPE",	SIGFPE,		0 },
		{ "SIGILL",	SIGILL,		0 },
	};
#define	VSRT_NUM_SIGNALS	(sizeof(vsrt_signals) / sizeof(vsrt_siglst))
#define VSRT_PROT_ALL	(mode_t)(S_IRWXU|S_IRWXG|S_IRWXO)
static char* test_file = "./dhd";
static int vsrt_got_sigsegv = 0;
static int vsrt_got_sigbus = 0;
static int vsrt_got_sig = 0;
static int vsrt_setjmp_called = 0;
static unsigned long vsrt_pgsz, vsrt_ipgsz;
static sigjmp_buf vsrt_env;	
static struct sigaction Sigaction;

main()
{
	int	fd, err;
	pid_t	pid;
	int *addr;
	volatile int i;
	
	if (vsrt_mmap_setup() == -1){
		printf("Setup error!\n");
		return;
	}
	fd = vsrt_open_file(test_file, O_RDWR, VSRT_PROT_ALL, 2);
	if (fd == -1){
		printf("Error in open file\n");
		return;
	}
	if ((addr = (int *)mmap(0, vsrt_pgsz, PROT_NONE, MAP_SHARED, fd, (off_t)vsrt_pgsz)) == (int *) (-1)) {
		err = errno;
		printf("mmap failed, errno = %d \n", err);
		(void)close(fd);
		(void)unlink(test_file);
		return;
	}

	
	if ((pid = fork()) == 0){
		vsrt_setjmp_called = 1;

		if(sigsetjmp(vsrt_env,1) ==0)
			i = *addr;

		if (!(vsrt_got_sigsegv || vsrt_got_sigbus)) {
			printf("Mapped page could be accessed\n");
			exit(1);
		}
		else printf("Mapped page could not be accessed\n");
		exit (0);
	}
	else if (pid == -1){
		printf("fork failed\n");
		exit(0);
	}
	else	wait((int *)0);	
	close(fd);
	munmap((void*) addr, (size_t)vsrt_pgsz);
	unlink(test_file);
} 

int vsrt_mmap_setup(void) {
	int	i;
	
	if ((vsrt_pgsz = sysconf(_SC_PAGESIZE)) == -1) {
		printf("Error to get pagesize\n");
		return -1;
	}
	vsrt_ipgsz = vsrt_pgsz/sizeof(int);
	vsrt_got_sigsegv = vsrt_got_sigbus = vsrt_got_sig = 0;
	for (i = 0; i < VSRT_NUM_SIGNALS; i++) {
		if (vsrt_signal(vsrt_signals[i].signal, &vsrt_invalid_sig_fn) == -1)  return -1;
	}
	if (vsrt_signal(SIGSEGV, vsrt_mmapsig_fn) == -1) return -1;
	if (vsrt_signal(SIGBUS, vsrt_mmapsig_fn) == -1)	return -1;
	return 0;
}

int vsrt_signal(int sig, void (*handler)(int))
{
	int	rval;
	rval = sigemptyset(&(Sigaction.sa_mask));
	if (rval == -1) {
		printf("Error to sigemptyset\n");
		return -1;
	}
	Sigaction.sa_handler = handler;
	Sigaction.sa_flags = 0;
	rval = sigaction(sig,&Sigaction,(struct sigaction *)NULL);
	if (rval == -1) {
		printf("Error in sigaction\n");
		return -1;
	}
	return 0;
}

static void vsrt_invalid_sig_fn(int s)
{
	int i;
	for (i = 0; i < VSRT_NUM_SIGNALS; i++) {
		if (s == vsrt_signals[i].signal) {
			printf("Invalid signal: %d(%s) received\n", s, vsrt_signals[i].name);
			return;
		}
	}
}

static void vsrt_mmapsig_fn(int s)
{
	if (s == SIGSEGV)	vsrt_got_sigsegv++;
	else if (s == SIGBUS) vsrt_got_sigbus++;
	vsrt_got_sig++;
	if (vsrt_setjmp_called) {
		vsrt_setjmp_called = 0;
		siglongjmp(vsrt_env, 1);
	}
}

int vsrt_open_file(char* s, int flag, mode_t mode, int npages)
{
	int fd, *buf, i, j;
	unlink(s);
	if ((fd = open(s, (O_RDWR|O_CREAT|O_TRUNC), (mode_t)(S_IRWXU|S_IRWXG|S_IRWXO))) == -1) {
		printf("open() failed, errno = %d \n", errno);
		return -1;
	}
	if ((buf = (int *)malloc(sizeof(int) * vsrt_ipgsz * npages)) == NULL) {
		printf("malloc() failed, errno = %d \n", errno);
		return -1;
	}
	for (j = 0; j < vsrt_ipgsz*npages; j += sizeof(buf)/sizeof(buf[0])) {
		for (i = 0; i < sizeof(buf)/sizeof(buf[0]); i++)	buf[i] = i + j;
		if (write(fd, (void*)buf, sizeof(buf)) != sizeof(buf)) {
			printf("write() failed, errno = %d \n", errno );
			return -1;
		}
	}
	(void)close(fd);
	if (chmod(s, mode) == -1) {
		printf("chmod() failed, errno = %d \n", errno);
		return -1;
	}
	if ((fd = open(s, flag)) == -1) {
		printf("open() failed, errno = %d \n", errno);
		return -1;
	}
	return (fd);
}

How-To-Repeat: The ported program is attached in the Full Description part.
Comment 1 krentel 2004-04-14 02:35:38 UTC
First, there is a much simpler program to reproduce the bug.  This is
from 5.2-current as of April 6, 2004.

/*
 * Test mmap() and PROT_NONE.  The first printf() should fail with seg
 * fault or bus error because the segment has protection PROT_NONE,
 * but actually, it runs ok.
 */

#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/mman.h>

int main()
{
	int   fd;
	char *p;

	fd = open("myfile", O_RDWR);
	if (fd < 0) err(1, "open failed");

	p = mmap(NULL, 4096, PROT_NONE, MAP_SHARED, fd, 0);
	if (p == MAP_FAILED) err(1, "mmap failed");

	printf("Reading p[4] = %d\n", p[4]);
	printf("Success\n");

	return 0;
}

% yes abcdefg | dd of=myfile bs=1024 count=4
4+0 records in
4+0 records out
4096 bytes transferred in 0.000223 secs (18354561 bytes/sec)
% hd myfile 
00000000  61 62 63 64 65 66 67 0a  61 62 63 64 65 66 67 0a  |abcdefg.abcdefg.|
*
00001000
% ./a.out 
Reading p[4] = 101
Success
%

The problem is that mmap() calls vm_mmap() and the non-MAP_STACK
case calls vm_map_find() which calls vm_map_insert() which calls
vm_map_pmap_enter() which calls pmap_enter_quick() and
pmap_enter_quick() assumes that the segment always has read access.
See: pmap_enter_quick() in sys/i386/i386/pmap.c.

A similar bug happens with madvise(..., MADV_WILLNEED).  Change the
above program to call open() followed by mmap(..., PROT_READ, ...)
followed by mprotect(..., PROT_NONE).  At this point, reading from the
segment produces a bus error.  But now add madvise(..., MADV_WILLNEED)
and you can read the segment.  Again, the problem is that madvise()
calls vm_map_madvise() and the case for MADV_WILLNEED calls
vm_map_pmap_enter() as before.

I could only produce the bug with mmap() on a file and with read
access.  Write access and mmap() with MAP_ANON and MAP_STACK behaved
as expected.

The patch below adds a parameter "prot" to vm_map_pmap_enter() so that
vm_map_pmap_enter() will avoid calling pmap_enter_quick() when the
segment doesn't have read access.  It makes a bit more sense for
vm_map_pmap_enter() to make this decision rather than the calling
program, and prot may be useful for other things.

With much guidence from: alc

--Mark

----------------

--- vm_map.h.orig	Tue Apr  6 16:21:07 2004
+++ vm_map.h	Tue Apr 13 17:16:14 2004
@@ -337,7 +337,7 @@
     vm_pindex_t *, vm_prot_t *, boolean_t *);
 void vm_map_lookup_done (vm_map_t, vm_map_entry_t);
 boolean_t vm_map_lookup_entry (vm_map_t, vm_offset_t, vm_map_entry_t *);
-void vm_map_pmap_enter(vm_map_t map, vm_offset_t addr,
+void vm_map_pmap_enter(vm_map_t map, vm_offset_t addr, vm_prot_t prot,
     vm_object_t object, vm_pindex_t pindex, vm_size_t size, int flags);
 int vm_map_protect (vm_map_t, vm_offset_t, vm_offset_t, vm_prot_t, boolean_t);
 int vm_map_remove (vm_map_t, vm_offset_t, vm_offset_t);

--- vm_map.c.orig	Tue Apr  6 16:21:07 2004
+++ vm_map.c	Tue Apr 13 17:53:27 2004
@@ -881,7 +881,7 @@
 #endif
 
 	if (cow & (MAP_PREFAULT|MAP_PREFAULT_PARTIAL)) {
-		vm_map_pmap_enter(map, start,
+		vm_map_pmap_enter(map, start, prot,
 				    object, OFF_TO_IDX(offset), end - start,
 				    cow & MAP_PREFAULT_PARTIAL);
 	}
@@ -1252,14 +1252,14 @@
  *	immediately after an mmap(2).
  */
 void
-vm_map_pmap_enter(vm_map_t map, vm_offset_t addr,
+vm_map_pmap_enter(vm_map_t map, vm_offset_t addr, vm_prot_t prot,
     vm_object_t object, vm_pindex_t pindex, vm_size_t size, int flags)
 {
 	vm_offset_t tmpidx;
 	int psize;
 	vm_page_t p, mpte;
 
-	if (object == NULL)
+	if ((object == NULL) || ((prot & VM_PROT_READ) == 0))
 		return;
 	mtx_lock(&Giant);
 	VM_OBJECT_LOCK(object);
@@ -1551,6 +1551,7 @@
 			if (behav == MADV_WILLNEED) {
 				vm_map_pmap_enter(map,
 				    useStart,
+				    current->protection,
 				    current->object.vm_object,
 				    pindex,
 				    (count << PAGE_SHIFT),
Comment 2 Alan L. Cox 2004-04-24 05:07:06 UTC
Mark's patch with an additional comment and one minor change has been 
commited to current:

   Revision  Changes    Path
   1.333     +5 -4      src/sys/vm/vm_map.c
   1.109     +1 -1      src/sys/vm/vm_map.h

I've asked for a volunteer to port and commit this change to RELENG_4.
Comment 3 Alan Cox freebsd_committer freebsd_triage 2004-04-24 05:13:16 UTC
State Changed
From-To: open->patched

Patch applied to -CURRENT. 


Comment 4 Alan Cox freebsd_committer freebsd_triage 2004-04-24 05:13:16 UTC
Responsible Changed
From-To: freebsd-bugs->alc

Patch applied to -CURRENT.
Comment 5 Remko Lodder freebsd_committer freebsd_triage 2007-03-11 20:39:34 UTC
State Changed
From-To: patched->closed

Merged to relevant branches, closing the ticket (note no merging will 
occur for 4.x it is dead).