Attachment 509875 Details for Bug 716489 – read instead of mmap to load modules

[patch] read instead of mmap to load modules

rhbz716489.patch (text/plain), 3.03 KB, created by Josh Stone on 2011-06-25 05:25:16 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josh Stone

Created: 2011-06-25 05:25:16 UTC

Size: 3.03 KB

patch

obsolete

>commit fa6b56faaa56c98203dcc3fbdda5eab3d91ec62d
>Author: Josh Stone <jistone@redhat.com>
>Date:   Fri Jun 24 15:00:41 2011 -0700
>
>    rhbz716489: read instead of mmap to load modules
>    
>    As staprun is preparing to load a kernel module, we first mmap the whole
>    module as MAP_PRIVATE. Then we proceed with our security checks,
>    including a trusted-signature validation on the mapped region, and if
>    all checks out, we'll call init_module() with that same mapped region.
>    
>    However, MMAP(2) says of MAP_PRIVATE, "It is unspecified whether changes
>    made to the file after the mmap() call are visible in the mapped
>    region."  From my testing, it appears that file changes do indeed show
>    up in our mapped memory.  This means we have a TOCTOU race between
>    verifying the signature of that memory and then calling init_module().
>    
>    By using read() instead of mmap(), we ensure that we have a fully
>    private copy of the module to verify and load, without fear of change.
>
>diff --git a/runtime/staprun/staprun_funcs.c b/runtime/staprun/staprun_funcs.c
>index 74eef9c..e0a5a46 100644
>--- a/runtime/staprun/staprun_funcs.c
>+++ b/runtime/staprun/staprun_funcs.c
>@@ -49,7 +49,7 @@ int insert_module(
>   assert_permissions_func assert_permissions
> ) {
> 	int i;
>-	long ret;
>+	long ret, module_read;
> 	void *module_file;
> 	char *opts;
> 	int saved_errno;
>@@ -109,17 +109,39 @@ int insert_module(
> 		return -1;
> 	}
> 
>-	/* mmap in the entire module. Work with the memory mapped data from this
>-	   point on to avoid a TOCTOU race between path and signature checking
>-	   below and module loading.  */
>-	module_file = mmap(NULL, sbuf.st_size, PROT_READ, MAP_PRIVATE, module_fd, 0);
>-	if (module_file == MAP_FAILED) {
>-		_perr("Error mapping '%s'", module_realpath);
>+	/* Allocate memory for the entire module. */
>+	module_file = calloc(1, sbuf.st_size);
>+	if (module_file == NULL) {
>+		_perr("Error allocating memory to read '%s'", module_realpath);
> 		close(module_fd);
> 		free(opts);
> 		return -1;
> 	}
> 
>+	/* read in the entire module.  Work with this copy of the data from this
>+	   point on to avoid a TOCTOU race between path and signature checking
>+	   below and module loading.  */
>+	module_read = 0;
>+	while (module_read < sbuf.st_size) {
>+		ret = read(module_fd, module_file + module_read,
>+				sbuf.st_size - module_read);
>+		if (ret > 0)
>+			module_read += ret;
>+		else if (ret == 0) {
>+			_err("Unexpected EOF reading '%s'", module_realpath);
>+			free(module_file);
>+			close(module_fd);
>+			free(opts);
>+			return -1;
>+		} else if (errno != EINTR) {
>+			_perr("Error reading '%s'", module_realpath);
>+			free(module_file);
>+			close(module_fd);
>+			free(opts);
>+			return -1;
>+		}
>+	}
>+
> 	/* Check whether this module can be loaded by the current user.
> 	 * check_permissions will exit(-1) if permissions are insufficient*/
> 	assert_permissions (module_realpath, module_fd, module_file, sbuf.st_size);
>@@ -131,7 +153,7 @@ int insert_module(
> 
> 	/* Cleanup. */
> 	free(opts);
>-	munmap(module_file, sbuf.st_size);
>+	free(module_file);
> 	close(module_fd);
> 
> 	if (ret != 0) {

Actions: View | Diff

Attachments on bug 716489: 509875