[U-Boot] [PATCH] bootm.c: Correct the flush_len used in bootm_load_os()

[U-Boot] [PATCH] bootm.c: Correct the flush_len used in bootm_load_os()

[Tom Rini]

In do_bootm_states when doing BOOTM_STATE_LOADOS we use load_end
uninitialized and Coverity notes this now.  This however leads down
another interesting path.  We pass this pointer to bootm_load_os and
that in turn uses this uninitialized value immediately to calculate the
flush length, and is wrong.  We do not know what load_end will be until
after bootm_decomp_image is called, so we must only set flush_len after
that.  All of this also makes it clear that the only reason we pass a
pointer for load_end to bootm_load_os is so that we can call lmb_reserve
on success.  Rather than initialize load_end to 0 in do_bootm_states we
can just call lmb_reserve ourself.

Reported-by: Coverity (CID: 175572)
Cc: Simon Glass <sjg@chromium.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 common/bootm.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 36162917a1b7..7bdfc4b2b20e 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -425,17 +425,17 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
 }
 
 #ifndef USE_HOSTCC
-static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
-			 int boot_progress)
+static int bootm_load_os(bootm_headers_t *images, int boot_progress)
 {
 	image_info_t os = images->os;
 	ulong load = os.load;
+	ulong load_end;
 	ulong blob_start = os.start;
 	ulong blob_end = os.end;
 	ulong image_start = os.image_start;
 	ulong image_len = os.image_len;
 	ulong flush_start = ALIGN_DOWN(load, ARCH_DMA_MINALIGN);
-	ulong flush_len = *load_end - load;
+	ulong flush_len;
 	bool no_overlap;
 	void *load_buf, *image_buf;
 	int err;
@@ -444,27 +444,28 @@ static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
 	image_buf = map_sysmem(os.image_start, image_len);
 	err = bootm_decomp_image(os.comp, load, os.image_start, os.type,
 				 load_buf, image_buf, image_len,
-				 CONFIG_SYS_BOOTM_LEN, load_end);
+				 CONFIG_SYS_BOOTM_LEN, &load_end);
 	if (err) {
 		bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
 		return err;
 	}
 
+	flush_len = load_end - load;
 	if (flush_start < load)
 		flush_len += load - flush_start;
 
 	flush_cache(flush_start, ALIGN(flush_len, ARCH_DMA_MINALIGN));
 
-	debug("   kernel loaded at 0x%08lx, end = 0x%08lx\n", load, *load_end);
+	debug("   kernel loaded@0x%08lx, end = 0x%08lx\n", load, load_end);
 	bootstage_mark(BOOTSTAGE_ID_KERNEL_LOADED);
 
 	no_overlap = (os.comp == IH_COMP_NONE && load == image_start);
 
-	if (!no_overlap && (load < blob_end) && (*load_end > blob_start)) {
+	if (!no_overlap && (load < blob_end) && (load_end > blob_start)) {
 		debug("images.os.start = 0x%lX, images.os.end = 0x%lx\n",
 		      blob_start, blob_end);
 		debug("images.os.load = 0x%lx, load_end = 0x%lx\n", load,
-		      *load_end);
+		      load_end);
 
 		/* Check what type of image this is. */
 		if (images->legacy_hdr_valid) {
@@ -479,6 +480,8 @@ static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
 		}
 	}
 
+	lmb_reserve(&images->lmb, images->os.load, (load_end -
+				                    images->os.load));
 	return 0;
 }
 
@@ -630,14 +633,9 @@ int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 
 	/* Load the OS */
 	if (!ret && (states & BOOTM_STATE_LOADOS)) {
-		ulong load_end;
-
 		iflag = bootm_disable_interrupts();
-		ret = bootm_load_os(images, &load_end, 0);
-		if (ret == 0)
-			lmb_reserve(&images->lmb, images->os.load,
-				    (load_end - images->os.load));
-		else if (ret && ret != BOOTM_ERR_OVERLAP)
+		ret = bootm_load_os(images, 0);
+		if (ret && ret != BOOTM_ERR_OVERLAP)
 			goto err;
 		else if (ret == BOOTM_ERR_OVERLAP)
 			ret = 0;
-- 
2.7.4

[U-Boot] [PATCH] bootm.c: Correct the flush_len used in bootm_load_os()

[Simon Glass]

On 1 May 2018 at 10:32, Tom Rini <trini@konsulko.com> wrote:
> In do_bootm_states when doing BOOTM_STATE_LOADOS we use load_end
> uninitialized and Coverity notes this now.  This however leads down
> another interesting path.  We pass this pointer to bootm_load_os and
> that in turn uses this uninitialized value immediately to calculate the
> flush length, and is wrong.  We do not know what load_end will be until
> after bootm_decomp_image is called, so we must only set flush_len after
> that.  All of this also makes it clear that the only reason we pass a
> pointer for load_end to bootm_load_os is so that we can call lmb_reserve
> on success.  Rather than initialize load_end to 0 in do_bootm_states we
> can just call lmb_reserve ourself.
>
> Reported-by: Coverity (CID: 175572)
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  common/bootm.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)

Looks better to me.

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] bootm.c: Correct the flush_len used in bootm_load_os()

[Tom Rini]

On Tue, May 01, 2018 at 12:32:37PM -0400, Tom Rini wrote:

> In do_bootm_states when doing BOOTM_STATE_LOADOS we use load_end
> uninitialized and Coverity notes this now.  This however leads down
> another interesting path.  We pass this pointer to bootm_load_os and
> that in turn uses this uninitialized value immediately to calculate the
> flush length, and is wrong.  We do not know what load_end will be until
> after bootm_decomp_image is called, so we must only set flush_len after
> that.  All of this also makes it clear that the only reason we pass a
> pointer for load_end to bootm_load_os is so that we can call lmb_reserve
> on success.  Rather than initialize load_end to 0 in do_bootm_states we
> can just call lmb_reserve ourself.
> 
> Reported-by: Coverity (CID: 175572)
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180508/130fb56c/attachment.sig>