diff mbox series

[U-Boot] tools: mkimage: Ensure munmap unmaps the same length that was mapped

Message ID 20180828225114.30005-1-judge.packham@gmail.com
State Accepted
Commit 8961c8ad252b8af887439e4e5c6c1bc0c912f2de
Delegated to: Tom Rini
Headers show
Series [U-Boot] tools: mkimage: Ensure munmap unmaps the same length that was mapped | expand

Commit Message

Chris Packham Aug. 28, 2018, 10:51 p.m. UTC 
From: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>

The set_header call in kwbimage.c adds a checksum to the end of the
image in addition to setting up the header. It 'helpfully' updates the
st_size to match the fact that the file is now longer. However, mkimage
uses this length in the munmap call. This can lead to unmapping an extra
page, of perhaps required data. When this happens, a SEGV can occur.

To prevent this from happening, the munmap call now uses the same length
that was passed to mmap. This could also have been fixed by not changing
the length in kwbimage.c, however changing it in the main file means
that other plugins will also not fall for the same trap.

Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
Signed-off-by: Chris Packham <judge.packham@gmail.com>
[cp: resolve checkpatch complaints]
Tested-by: Chris Packham <judge.packham@gmail.com>
---

 tools/mkimage.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Tom Rini Sept. 11, 2018, 12:26 p.m. UTC | #1 
On Wed, Aug 29, 2018 at 10:51:14AM +1200, Chris Packham wrote:

> From: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> 
> The set_header call in kwbimage.c adds a checksum to the end of the
> image in addition to setting up the header. It 'helpfully' updates the
> st_size to match the fact that the file is now longer. However, mkimage
> uses this length in the munmap call. This can lead to unmapping an extra
> page, of perhaps required data. When this happens, a SEGV can occur.
> 
> To prevent this from happening, the munmap call now uses the same length
> that was passed to mmap. This could also have been fixed by not changing
> the length in kwbimage.c, however changing it in the main file means
> that other plugins will also not fall for the same trap.
> 
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> [cp: resolve checkpatch complaints]
> Tested-by: Chris Packham <judge.packham@gmail.com>

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/tools/mkimage.c b/tools/mkimage.c
index e0d4d20be499..6abd4d6a8b22 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -318,6 +318,7 @@  int main(int argc, char **argv)
 	struct image_type_params *tparams = NULL;
 	int pad_len = 0;
 	int dfd;
+	size_t map_len;
 
 	params.cmdname = *argv;
 	params.addr = 0;
@@ -576,7 +577,8 @@  int main(int argc, char **argv)
 	}
 	params.file_size = sbuf.st_size;
 
-	ptr = mmap(0, sbuf.st_size, PROT_READ|PROT_WRITE, MAP_SHARED, ifd, 0);
+	map_len = sbuf.st_size;
+	ptr = mmap(0, map_len, PROT_READ | PROT_WRITE, MAP_SHARED, ifd, 0);
 	if (ptr == MAP_FAILED) {
 		fprintf (stderr, "%s: Can't map %s: %s\n",
 			params.cmdname, params.imagefile, strerror(errno));
@@ -600,7 +602,7 @@  int main(int argc, char **argv)
 			params.cmdname, tparams->name);
 	}
 
-	(void) munmap((void *)ptr, sbuf.st_size);
+	(void)munmap((void *)ptr, map_len);
 
 	/* We're a bit of paranoid */
 #if defined(_POSIX_SYNCHRONIZED_IO) && \