Move __cleanup into mprotect'ed page to prevent unintentional modifications

similar to the atexit handlers. Idea and help deraadt@, ok deraadt@

9 files changed, 102 insertions, 38 deletions

diff --git a/lib/libc/stdio/findfp.c b/lib/libc/stdio/findfp.c
index 0702efad985..3aa66965367 100644
--- a/lib/libc/stdio/findfp.c
+++ b/lib/libc/stdio/findfp.c
@@ -35,7 +35,7 @@
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
-static char rcsid[] = "$OpenBSD: findfp.c,v 1.2 1996/08/19 08:32:36 tholo Exp $";
+static char rcsid[] = "$OpenBSD: findfp.c,v 1.3 2002/09/14 22:03:14 dhartmei Exp $";
 #endif /* LIBC_SCCS and not lint */
 
 #include <sys/param.h>
@@ -141,9 +141,10 @@ f_prealloc()
 }
 
 /*
- * exit() calls _cleanup() through *__cleanup, set whenever we
- * open or buffer a file.  This chicanery is done so that programs
- * that do not use stdio need not link it all in.
+ * exit() and abort() call _cleanup() through the callback registered
+ * with __atexit_register_cleanup(), set whenever we open or buffer a
+ * file. This chicanery is done so that programs that do not use stdio
+ * need not link it all in.
  *
  * The name `_cleanup' is, alas, fairly well known outside stdio.
  */
@@ -161,6 +162,6 @@ void
 __sinit()
 {
 	/* make sure we clean up on exit */
-	__cleanup = _cleanup;		/* conservative */
+	__atexit_register_cleanup(_cleanup); /* conservative */
 	__sdidinit = 1;
 }
diff --git a/lib/libc/stdio/local.h b/lib/libc/stdio/local.h
index 70d5c564471..05060408f82 100644
--- a/lib/libc/stdio/local.h
+++ b/lib/libc/stdio/local.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: local.h,v 1.4 2002/02/17 19:42:23 millert Exp $	*/
+/*	$OpenBSD: local.h,v 1.5 2002/09/14 22:03:14 dhartmei Exp $	*/
 
 /*-
  * Copyright (c) 1990, 1993
@@ -50,13 +50,13 @@ fpos_t	__sseek(void *, fpos_t, int);
 int	__sclose(void *);
 void	__sinit(void);
 void	_cleanup(void);
-void	(*__cleanup)(void);
 void	__smakebuf(FILE *);
 int	__swhatbuf(FILE *, size_t *, int *);
 int	_fwalk(int (*)(FILE *));
 int	__swsetup(FILE *);
 int	__sflags(const char *, int *);
 
+extern void __atexit_register_cleanup(void (*)());
 extern int __sdidinit;
 
 /*
diff --git a/lib/libc/stdio/makebuf.c b/lib/libc/stdio/makebuf.c
index 4ebc3e93362..da9b86ca4ce 100644
--- a/lib/libc/stdio/makebuf.c
+++ b/lib/libc/stdio/makebuf.c
@@ -35,7 +35,7 @@
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
-static char rcsid[] = "$OpenBSD: makebuf.c,v 1.3 1998/11/25 05:18:49 millert Exp $";
+static char rcsid[] = "$OpenBSD: makebuf.c,v 1.4 2002/09/14 22:03:14 dhartmei Exp $";
 #endif /* LIBC_SCCS and not lint */
 
 #include <sys/types.h>
@@ -73,7 +73,7 @@ __smakebuf(fp)
 		fp->_bf._size = 1;
 		return;
 	}
-	__cleanup = _cleanup;
+	__atexit_register_cleanup(_cleanup);
 	flags |= __SMBF;
 	fp->_bf._base = fp->_p = p;
 	fp->_bf._size = size;
diff --git a/lib/libc/stdio/setvbuf.c b/lib/libc/stdio/setvbuf.c
index 65501fce9d9..0c6fd8d1623 100644
--- a/lib/libc/stdio/setvbuf.c
+++ b/lib/libc/stdio/setvbuf.c
@@ -35,7 +35,7 @@
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
-static char rcsid[] = "$OpenBSD: setvbuf.c,v 1.3 2001/07/09 06:57:44 deraadt Exp $";
+static char rcsid[] = "$OpenBSD: setvbuf.c,v 1.4 2002/09/14 22:03:14 dhartmei Exp $";
 #endif /* LIBC_SCCS and not lint */
 
 #include <stdio.h>
@@ -155,7 +155,7 @@ nbf:
 		/* begin/continue reading, or stay in intermediate state */
 		fp->_w = 0;
 	}
-	__cleanup = _cleanup;
+	__atexit_register_cleanup(_cleanup);
 
 	return (ret);
 }
diff --git a/lib/libc/stdlib/abort.c b/lib/libc/stdlib/abort.c
index 41a9f0f48bb..7057f9b1ad0 100644
--- a/lib/libc/stdlib/abort.c
+++ b/lib/libc/stdlib/abort.c
@@ -32,19 +32,19 @@
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
-static char *rcsid = "$OpenBSD: abort.c,v 1.7 2001/08/12 12:03:01 heko Exp $";
+static char *rcsid = "$OpenBSD: abort.c,v 1.8 2002/09/14 22:03:14 dhartmei Exp $";
 #endif /* LIBC_SCCS and not lint */
 
 #include <signal.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include "thread_private.h"
-
-void (*__cleanup)();
+#include "atexit.h"
 
 void
 abort()
 {
+	struct atexit *p = __atexit;
 	static int cleanup_called = 0;
 	sigset_t mask;
 
@@ -64,9 +64,13 @@ abort()
 	/*
 	 * POSIX requires we flush stdio buffers on abort
 	 */
-	if (cleanup_called == 0 && __cleanup != NULL) {
-		cleanup_called = 1;
-		(*__cleanup)();
+	if (cleanup_called == 0) {
+		while (p != NULL && p->next != NULL)
+			p = p->next;
+		if (p != NULL && p->fns[0] != NULL) {
+			cleanup_called = 1;
+			(*p->fns[0])();
+		}
 	}
 
 	(void)kill(getpid(), SIGABRT);
diff --git a/lib/libc/stdlib/atexit.c b/lib/libc/stdlib/atexit.c
index da5a0ddda03..98564d0dd3a 100644
--- a/lib/libc/stdlib/atexit.c
+++ b/lib/libc/stdlib/atexit.c
@@ -29,7 +29,7 @@
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
-static char *rcsid = "$OpenBSD: atexit.c,v 1.6 2002/09/06 22:48:34 henning Exp $";
+static char *rcsid = "$OpenBSD: atexit.c,v 1.7 2002/09/14 22:03:14 dhartmei Exp $";
 #endif /* LIBC_SCCS and not lint */
 
 #include <sys/types.h>
@@ -42,6 +42,20 @@ int __atexit_invalid = 1;
 struct atexit *__atexit;
 
 /*
+ * Function pointers are stored in a linked list of pages. The list
+ * is initially empty, and pages are allocated on demand. The first
+ * function pointer in the first allocated page (the last one in
+ * the linked list) is reserved for the cleanup function.
+ *
+ * Outside the following two functions, all pages are mprotect()'ed
+ * to prevent unintentional/malicious corruption.
+ *
+ * The free(malloc(1)) is a workaround causing malloc_init() to
+ * ensure that malloc.c gets the first mmap() call for its sbrk()
+ * games.
+ */
+
+/*
  * Register a function to be performed at exit.
  */
 int
@@ -61,9 +75,6 @@ atexit(fn)
 	}
 	if (p == NULL) {
 		if (__atexit_invalid) {
-			/* malloc.c wants the first mmap() for sbrk()
-			   games ('nice hack'), so enforce
-			   malloc_init() with a dummy call.  */
 			free(malloc(1));
 			__atexit_invalid = 0;
 		}
@@ -71,7 +82,11 @@ atexit(fn)
 		    MAP_ANON | MAP_PRIVATE, -1, 0);
 		if (p == MAP_FAILED)
 			return (-1);
-		p->ind = 0;
+		if (__atexit == NULL) {
+			p->fns[0] = NULL;
+			p->ind = 1;
+		} else
+			p->ind = 0;
 		p->max = (pgsize - ((char *)&p->fns[0] - (char *)p)) /
 		    sizeof(p->fns[0]);
 		p->next = __atexit;
@@ -82,3 +97,39 @@ atexit(fn)
 		return (-1);
 	return (0);
 }
+
+/*
+ * Register the cleanup function
+ */
+void
+__atexit_register_cleanup(fn)
+	void (*fn)();
+{
+	register struct atexit *p = __atexit;
+	register int pgsize = getpagesize();
+
+	if (pgsize < sizeof(*p))
+		return;
+	while (p != NULL && p->next != NULL)
+		p = p->next;
+	if (p == NULL) {
+		if (__atexit_invalid) {
+			free(malloc(1));
+			__atexit_invalid = 0;
+		}
+		p = mmap(NULL, pgsize, PROT_READ | PROT_WRITE,
+		    MAP_ANON | MAP_PRIVATE, -1, 0);
+		if (p == MAP_FAILED)
+			return;
+		p->ind = 1;
+		p->max = (pgsize - ((char *)&p->fns[0] - (char *)p)) /
+		    sizeof(p->fns[0]);
+		p->next = NULL;
+		__atexit = p;
+	} else {
+		if (mprotect(p, pgsize, PROT_READ | PROT_WRITE))
+		    return;
+	}
+	p->fns[0] = fn;
+	mprotect(p, pgsize, PROT_READ);
+}
diff --git a/lib/libc/stdlib/exit.c b/lib/libc/stdlib/exit.c
index c69639125ef..e22bd5178e6 100644
--- a/lib/libc/stdlib/exit.c
+++ b/lib/libc/stdlib/exit.c
@@ -32,7 +32,7 @@
  */
 
 #if defined(LIBC_SCCS) && !defined(lint)
-static char *rcsid = "$OpenBSD: exit.c,v 1.7 2002/08/30 07:58:07 dhartmei Exp $";
+static char *rcsid = "$OpenBSD: exit.c,v 1.8 2002/09/14 22:03:14 dhartmei Exp $";
 #endif /* LIBC_SCCS and not lint */
 
 #include <sys/types.h>
@@ -42,8 +42,6 @@ static char *rcsid = "$OpenBSD: exit.c,v 1.7 2002/08/30 07:58:07 dhartmei Exp $"
 #include "atexit.h"
 #include "thread_private.h"
 
-void (*__cleanup)();
-
 /*
  * This variable is zero until a process has created a thread.
  * It is used to avoid calling locking functions in libc when they
@@ -67,13 +65,13 @@ exit(status)
 		p = __atexit;
 		while (p != NULL) {
 			for (n = p->ind; --n >= 0;)
-				(*p->fns[n])();
+				if (p->fns[n] != NULL)
+					(*p->fns[n])();
 			q = p;
 			p = p->next;
 			munmap(q, pgsize);
 		}
 	}
-	if (__cleanup)
-		(*__cleanup)();
+	/* cleanup, if registered, was called through fns[0] in the last page */
 	_exit(status);
 }
diff --git a/regress/lib/libc/atexit/Makefile b/regress/lib/libc/atexit/Makefile
index 89625e2a7b4..55c3f3981e6 100644
--- a/regress/lib/libc/atexit/Makefile
+++ b/regress/lib/libc/atexit/Makefile
@@ -1,4 +1,4 @@
-#	$OpenBSD: Makefile,v 1.2 2002/09/02 19:59:51 avsm Exp $
+#	$OpenBSD: Makefile,v 1.3 2002/09/14 22:03:14 dhartmei Exp $
 
 NOMAN=
 PROG=atexit_test
@@ -6,7 +6,9 @@ PROG=atexit_test
 run-regress-atexit_test: ${PROG}
 	./${PROG} -valid 2>${.OBJDIR}/valid.out
 	cmp -s ${.OBJDIR}/valid.out ${.CURDIR}/valid.ok
-	./${PROG} -invalid 2>${.OBJDIR}/invalid.out
+	./${PROG} -invalid-atexit 2>${.OBJDIR}/invalid.out
+	cmp -s ${.OBJDIR}/invalid.out ${.CURDIR}/invalid.ok
+	./${PROG} -invalid-cleanup 2>${.OBJDIR}/invalid.out
 	cmp -s ${.OBJDIR}/invalid.out ${.CURDIR}/invalid.ok
 
 .include <bsd.regress.mk>
diff --git a/regress/lib/libc/atexit/atexit_test.c b/regress/lib/libc/atexit/atexit_test.c
index fcfe95cb020..fea95832ab5 100644
--- a/regress/lib/libc/atexit/atexit_test.c
+++ b/regress/lib/libc/atexit/atexit_test.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: atexit_test.c,v 1.1 2002/07/29 19:51:41 dhartmei Exp $ */
+/*	$OpenBSD: atexit_test.c,v 1.2 2002/09/14 22:03:14 dhartmei Exp $ */
 
 /*
  * Copyright (c) 2002 Daniel Hartmeier
@@ -39,9 +39,6 @@
 #include <signal.h>
 #include "/usr/src/lib/libc/stdlib/atexit.h"
 
-extern struct atexit *__atexit;
-extern void (*__cleanup)();
-
 void	handle_first();
 void	handle_middle();
 void	handle_last();
@@ -57,8 +54,10 @@ main(int argc, char *argv[])
 	int i;
 
 	if (argc != 2 || (strcmp(argv[1], "-valid") &&
-	    strcmp(argv[1], "-invalid"))) {
-		fprintf(stderr, "%s -valid/-invalid\n", argv[0]);
+	    strcmp(argv[1], "-invalid-atexit") &&
+	    strcmp(argv[1], "-invalid-cleanup"))) {
+		fprintf(stderr, "%s -valid/-invalid-atexit/-invalid-cleanup\n",
+		    argv[0]);
 		return (1);
 	}
 	fprintf(stderr, "main()\n");
@@ -77,11 +76,20 @@ main(int argc, char *argv[])
 		return (1);
 	}
 	/* this is supposed to segfault */
-	if (strcmp(argv[1], "-valid")) {
+	if (!strcmp(argv[1], "-invalid-atexit")) {
 		signal(SIGSEGV, handle_signal);
 		__atexit->fns[0] = handle_invalid;
+	} else if (!strcmp(argv[1], "-invalid-cleanup")) {
+		struct atexit *p = __atexit;
+
+		signal(SIGSEGV, handle_signal);
+		while (p != NULL && p->next != NULL)
+			p = p->next;
+		if (p == NULL)
+			fprintf(stderr, "p == NULL, no page found\n");
+		p->fns[0] = handle_invalid;
 	}
-	__cleanup = handle_cleanup;
+	__atexit_register_cleanup(handle_cleanup);
 	counter = 0;
 	fprintf(stderr, "main() returns\n");
 	return (0);