[U-Boot] [PATCH 0/8] Fix some coverity warnings

[U-Boot] [PATCH 0/8] Fix some coverity warnings

[Simon Glass]

This series attempts to fix various problems reported by Coverity, in
subsystems where I have some knowledge.


Simon Glass (8):
  log: Fix incorect range check in log_get_cat_name()
  console: Fix handling of NULL global_data
  sandbox: Use memcpy() to move overlapping regions
  fdtgrep: Fix logic of free() in do_fdtgrep()
  fdtgrep: Separate out checking of two allocations
  rsa: Fix missing memory leak on error in fdt_add_bignum()
  spi: sandbox: Fix memory leak in sandbox_sf_bind_emul()
  sandbox: swap_case: Increase number of base address regs

 arch/sandbox/cpu/os.c     |  3 ++-
 common/console.c          |  8 ++++++--
 common/log.c              |  2 +-
 drivers/misc/swap_case.c  |  2 +-
 drivers/mtd/spi/sandbox.c |  6 +++---
 lib/rsa/rsa-sign.c        |  4 +---
 tools/fdtgrep.c           | 15 ++++++++++-----
 7 files changed, 24 insertions(+), 16 deletions(-)

-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 1/8] log: Fix incorect range check in log_get_cat_name()

[Simon Glass]

This allows access to an element after the end of the array. Fix it.

Reported-by: Coverity (CID: 173279)
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/log.c b/common/log.c
index 3b5588ebe7..4e488eca5b 100644
--- a/common/log.c
+++ b/common/log.c
@@ -38,7 +38,7 @@ static const char *log_level_name[LOGL_COUNT] = {
 
 const char *log_get_cat_name(enum log_category_t cat)
 {
-	if (cat > LOGC_COUNT)
+	if (cat >= LOGC_COUNT)
 		return "invalid";
 	if (cat >= LOGC_NONE)
 		return log_cat_name[cat - LOGC_NONE];
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 2/8] console: Fix handling of NULL global_data

[Simon Glass]

Both putc() and puts() can be called before global_data is set up. Some of
the code paths don't handle this correctly. Add an explicit test before
any member is accessed.

Reported-by: Coverity (CID: 169030)
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/console.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/common/console.c b/common/console.c
index 2688af56e1..2ba33dc574 100644
--- a/common/console.c
+++ b/common/console.c
@@ -502,8 +502,10 @@ void putc(const char c)
 		return;
 	}
 #endif
+	if (!gd)
+		return;
 #ifdef CONFIG_CONSOLE_RECORD
-	if (gd && (gd->flags & GD_FLG_RECORD) && gd->console_out.start)
+	if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
 		membuff_putbyte(&gd->console_out, c);
 #endif
 #ifdef CONFIG_SILENT_CONSOLE
@@ -541,8 +543,10 @@ void puts(const char *s)
 		return;
 	}
 #endif
+	if (!gd)
+		return;
 #ifdef CONFIG_CONSOLE_RECORD
-	if (gd && (gd->flags & GD_FLG_RECORD) && gd->console_out.start)
+	if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
 		membuff_put(&gd->console_out, s, strlen(s));
 #endif
 #ifdef CONFIG_SILENT_CONSOLE
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 3/8] sandbox: Use memcpy() to move overlapping regions

[Simon Glass]

The use of strcpy() to remove characters at the start of a string is safe
in U-Boot, since we know the implementation. But in os.c we are using the
C library's strcpy() function, where this behaviour is not permitted.

Update the code to use memcpy() instead.

Reported-by: Coverity (CID: 173279)

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

 arch/sandbox/cpu/os.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 5839932b00..496a8f9bd8 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen)
 	/* Look for 'u-boot' in the parent directory of spl/ */
 	p = strstr(fname, "/spl/");
 	if (p) {
-		strcpy(p, p + 4);
+		/* Remove the "/spl" characters */
+		memmove(p, p + 4, strlen(p + 4) + 1);
 		fd = os_open(fname, O_RDONLY);
 		if (fd >= 0) {
 			close(fd);
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 4/8] fdtgrep: Fix logic of free() in do_fdtgrep()

[Simon Glass]

This loop never actually exits, but the way the code is written this is
not obvious. Add an explicit error check.

Reported-by: Coverity (CID: 131280)

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

 tools/fdtgrep.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
index f2b8b71ed7..c4563e2289 100644
--- a/tools/fdtgrep.c
+++ b/tools/fdtgrep.c
@@ -801,7 +801,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename)
 	 * The first pass will count the regions, but if it is too many,
 	 * we do another pass to actually record them.
 	 */
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < 2; i++) {
 		region = malloc(count * sizeof(struct fdt_region));
 		if (!region) {
 			fprintf(stderr, "Out of memory for %d regions\n",
@@ -820,6 +820,8 @@ static int do_fdtgrep(struct display_info *disp, const char *filename)
 		if (count <= max_regions)
 			break;
 		free(region);
+		fprintf(stderr, "Internal error with fdtgrep_find_region)(\n");
+		return -1;
 	}
 
 	/* Optionally print a list of regions */
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 5/8] fdtgrep: Separate out checking of two allocations

[Simon Glass]

The current code might succeed on the first allocation and fail on the
second. Separate the checks to avoid this problem.

Of course, free() will never fail and the chances that (when allocating
two small areas) one will succeed and one will fail are just as remote.
But this keeps coverity happy.

Reported-by: Coverity (CID: 131226)

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

 tools/fdtgrep.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
index c4563e2289..5593b42203 100644
--- a/tools/fdtgrep.c
+++ b/tools/fdtgrep.c
@@ -133,11 +133,11 @@ static int value_add(struct display_info *disp, struct value_node **headp,
 	}
 
 	str = strdup(str);
+	if (!str)
+		goto err_mem;
 	node = malloc(sizeof(*node));
-	if (!str || !node) {
-		fprintf(stderr, "Out of memory\n");
-		return -1;
-	}
+	if (!node)
+		goto err_mem;
 	node->next = *headp;
 	node->type = type;
 	node->include = include;
@@ -145,6 +145,9 @@ static int value_add(struct display_info *disp, struct value_node **headp,
 	*headp = node;
 
 	return 0;
+err_mem:
+	fprintf(stderr, "Out of memory\n");
+	return -1;
 }
 
 static bool util_is_printable_string(const void *data, int len)
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 6/8] rsa: Fix missing memory leak on error in fdt_add_bignum()

[Simon Glass]

Thsi function can fail without freeing all its memory. Fix it.

Reported-by: Coverity (CID: 131217)
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/rsa/rsa-sign.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index d2788bf79a..2a09d2b19e 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -667,15 +667,13 @@ static int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
 	 * might fail several times
 	 */
 	ret = fdt_setprop(blob, noffset, prop_name, buf, size);
-	if (ret)
-		return -FDT_ERR_NOSPACE;
 	free(buf);
 	BN_free(tmp);
 	BN_free(big2);
 	BN_free(big32);
 	BN_free(big2_32);
 
-	return ret;
+	return ret ? -FDT_ERR_NOSPACE : 0;
 }
 
 int rsa_add_verify_data(struct image_sign_info *info, void *keydest)
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 7/8] spi: sandbox: Fix memory leak in sandbox_sf_bind_emul()

[Simon Glass]

Move the strdup() call so that it is only done when we know we will bind
the device.

Reported-by: Coverity (CID: 131216)

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

 drivers/mtd/spi/sandbox.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c
index 7893efee12..ae29c034b9 100644
--- a/drivers/mtd/spi/sandbox.c
+++ b/drivers/mtd/spi/sandbox.c
@@ -567,14 +567,14 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
 	strncpy(name, spec, sizeof(name) - 6);
 	name[sizeof(name) - 6] = '\0';
 	strcat(name, "-emul");
-	str = strdup(name);
-	if (!str)
-		return -ENOMEM;
 	drv = lists_driver_lookup_name("sandbox_sf_emul");
 	if (!drv) {
 		puts("Cannot find sandbox_sf_emul driver\n");
 		return -ENOENT;
 	}
+	str = strdup(name);
+	if (!str)
+		return -ENOMEM;
 	ret = device_bind(bus, drv, str, NULL, of_offset, &emul);
 	if (ret) {
 		printf("Cannot create emul device for spec '%s' (err=%d)\n",
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 8/8] sandbox: swap_case: Increase number of base address regs

[Simon Glass]

At present the code overruns the bar[] array. Fix this.

Reported-by: Coverity (CID: 131199)

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

 drivers/misc/swap_case.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/swap_case.c b/drivers/misc/swap_case.c
index 56cd0700fa..b777404c09 100644
--- a/drivers/misc/swap_case.c
+++ b/drivers/misc/swap_case.c
@@ -21,7 +21,7 @@
  */
 struct swap_case_platdata {
 	u16 command;
-	u32 bar[2];
+	u32 bar[6];
 };
 
 #define offset_to_barnum(offset)	\
-- 
2.18.0.rc1.242.g61856ae69a-goog

[U-Boot] [PATCH 3/8] sandbox: Use memcpy() to move overlapping regions

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> The use of strcpy() to remove characters at the start of a string is safe
> in U-Boot, since we know the implementation. But in os.c we are using the
> C library's strcpy() function, where this behaviour is not permitted.
> 
> Update the code to use memcpy() instead.
> 
> Reported-by: Coverity (CID: 173279)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  arch/sandbox/cpu/os.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index 5839932b00..496a8f9bd8 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen)
>  	/* Look for 'u-boot' in the parent directory of spl/ */
>  	p = strstr(fname, "/spl/");

I have built sandbox_spl_defconfig.

First observation: any parallel build fails. But that is to be handled
in a seperate patch.

The logic to find u-boot here is broken. From the top directory of the
git repository I executed:

$ spl/u-boot-spl

U-Boot SPL 2018.07-rc1-00063-g277daa5c0f (Jun 09 2018 - 20:40:23 +0200)
Trying to boot from sandbox
(spl/u-boot not found, error -2)
SPL: failed to boot from all boot devices
### ERROR ### Please RESET the board ###

Why would you expect fname to start with /spl/?

In my case fname = 'spl/u-boot'. Why do you test for '/spl/' (only)?

Just for the record if I execute
$ ./spl/u-boot-spl
everything works fine. But who would precede a directory name with './'?

Should we not concatenate the spl executable and the u-boot executable
during the build process so that spl/u-boot knows where to find u-boot?
That would be much safer than any assumption about relative paths.

Best regards

Heinrich

>  	if (p) {
> -		strcpy(p, p + 4);
> +		/* Remove the "/spl" characters */
> +		memmove(p, p + 4, strlen(p + 4) + 1);
>  		fd = os_open(fname, O_RDONLY);
>  		if (fd >= 0) {
>  			close(fd);
> 

[U-Boot] [PATCH 1/8] log: Fix incorect range check in log_get_cat_name()

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> This allows access to an element after the end of the array. Fix it.
> 
> Reported-by: Coverity (CID: 173279)
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  common/log.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/log.c b/common/log.c
> index 3b5588ebe7..4e488eca5b 100644
> --- a/common/log.c
> +++ b/common/log.c
> @@ -38,7 +38,7 @@ static const char *log_level_name[LOGL_COUNT] = {
>  
>  const char *log_get_cat_name(enum log_category_t cat)
>  {
> -	if (cat > LOGC_COUNT)
> +	if (cat >= LOGC_COUNT)
>  		return "invalid";
>  	if (cat >= LOGC_NONE)
>  		return log_cat_name[cat - LOGC_NONE];
> 

Please, consider all possible values of cat:

enums can take negative values or be an invalid uclass id. The function
terminates with

return uclass_get_name((enum uclass_id)cat);

This statement will return NULL if cat does not refer to an installed
uclass driver but above you decided that in case of an error you want to
return "invalid". This looks inconsistent to me.

Best regards

Heinrich

[U-Boot] [PATCH 2/8] console: Fix handling of NULL global_data

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> Both putc() and puts() can be called before global_data is set up. Some of
> the code paths don't handle this correctly. Add an explicit test before
> any member is accessed.
> 
> Reported-by: Coverity (CID: 169030)
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH 4/8] fdtgrep: Fix logic of free() in do_fdtgrep()

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> This loop never actually exits, but the way the code is written this is
> not obvious. Add an explicit error check.
> 
> Reported-by: Coverity (CID: 131280)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/fdtgrep.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
> index f2b8b71ed7..c4563e2289 100644
> --- a/tools/fdtgrep.c
> +++ b/tools/fdtgrep.c
> @@ -801,7 +801,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename)
>  	 * The first pass will count the regions, but if it is too many,
>  	 * we do another pass to actually record them.
>  	 */
> -	for (i = 0; i < 3; i++) {
> +	for (i = 0; i < 2; i++) {
>  		region = malloc(count * sizeof(struct fdt_region));
>  		if (!region) {
>  			fprintf(stderr, "Out of memory for %d regions\n",

Can't we call fdtgrep_find_regions() with max_regions = 0 and region =
NULL to do the counting and get rid of the loop? That may be a bit
slower but the code will be much easier to read.

> @@ -820,6 +820,8 @@ static int do_fdtgrep(struct display_info *disp, const char *filename)

Have a look at the lines in between:

                if (count < 0) {
                        report_error("fdt_find_regions", count);
                        return -1;
                }

Here a free(region) is missing.

Best regards

Heinrich

>  		if (count <= max_regions)
>  			break;
>  		free(region);
> +		fprintf(stderr, "Internal error with fdtgrep_find_region)(\n");
> +		return -1;
>  	}
>  
>  	/* Optionally print a list of regions */
> 

[U-Boot] [PATCH 5/8] fdtgrep: Separate out checking of two allocations

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> The current code might succeed on the first allocation and fail on the
> second. Separate the checks to avoid this problem.
> 
> Of course, free() will never fail and the chances that (when allocating
> two small areas) one will succeed and one will fail are just as remote.
> But this keeps coverity happy.
> 
> Reported-by: Coverity (CID: 131226)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/fdtgrep.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
> index c4563e2289..5593b42203 100644
> --- a/tools/fdtgrep.c
> +++ b/tools/fdtgrep.c
> @@ -133,11 +133,11 @@ static int value_add(struct display_info *disp, struct value_node **headp,
>  	}
>  
>  	str = strdup(str);
> +	if (!str)
> +		goto err_mem;
>  	node = malloc(sizeof(*node));
> -	if (!str || !node) {
> -		fprintf(stderr, "Out of memory\n");
> -		return -1;
> -	}
> +	if (!node)
> +		goto err_mem;
>  	node->next = *headp;
>  	node->type = type;
>  	node->include = include;
> @@ -145,6 +145,9 @@ static int value_add(struct display_info *disp, struct value_node **headp,
>  	*headp = node;
>

free(str) is missing here.


>  	return 0;
> +err_mem:

free(str) is missing here.

Best regards

Heinrich

> +	fprintf(stderr, "Out of memory\n");
> +	return -1;
>  }
>  
>  static bool util_is_printable_string(const void *data, int len)
> 

[U-Boot] [PATCH 6/8] rsa: Fix missing memory leak on error in fdt_add_bignum()

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> Thsi function can fail without freeing all its memory. Fix it.
> 
> Reported-by: Coverity (CID: 131217)
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  lib/rsa/rsa-sign.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
> index d2788bf79a..2a09d2b19e 100644
> --- a/lib/rsa/rsa-sign.c
> +++ b/lib/rsa/rsa-sign.c
> @@ -667,15 +667,13 @@ static int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
>  	 * might fail several times

Please, fix the memory leaks above this line too.

Best regards

Heinrich

>  	 */
>  	ret = fdt_setprop(blob, noffset, prop_name, buf, size);
> -	if (ret)
> -		return -FDT_ERR_NOSPACE;
>  	free(buf);
>  	BN_free(tmp);
>  	BN_free(big2);
>  	BN_free(big32);
>  	BN_free(big2_32);
>  
> -	return ret;
> +	return ret ? -FDT_ERR_NOSPACE : 0;
>  }
>  
>  int rsa_add_verify_data(struct image_sign_info *info, void *keydest)
> 

[U-Boot] [PATCH 7/8] spi: sandbox: Fix memory leak in sandbox_sf_bind_emul()

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> Move the strdup() call so that it is only done when we know we will bind
> the device.
> 
> Reported-by: Coverity (CID: 131216)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/mtd/spi/sandbox.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c
> index 7893efee12..ae29c034b9 100644
> --- a/drivers/mtd/spi/sandbox.c
> +++ b/drivers/mtd/spi/sandbox.c
> @@ -567,14 +567,14 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
>  	strncpy(name, spec, sizeof(name) - 6);
>  	name[sizeof(name) - 6] = '\0';
>  	strcat(name, "-emul");
> -	str = strdup(name);
> -	if (!str)
> -		return -ENOMEM;
>  	drv = lists_driver_lookup_name("sandbox_sf_emul");
>  	if (!drv) {
>  		puts("Cannot find sandbox_sf_emul driver\n");
>  		return -ENOENT;
>  	}
> +	str = strdup(name);
> +	if (!str)
> +		return -ENOMEM;
>  	ret = device_bind(bus, drv, str, NULL, of_offset, &emul);
>  	if (ret) {
>  		printf("Cannot create emul device for spec '%s' (err=%d)\n",

Aren't you leaking str's memory here? I would have expected free(str).

Best regards

Heinrich

> 

[U-Boot] [PATCH 8/8] sandbox: swap_case: Increase number of base address regs

[Heinrich Schuchardt]

On 06/09/2018 08:22 PM, Simon Glass wrote:
> At present the code overruns the bar[] array. Fix this.
> 
> Reported-by: Coverity (CID: 131199)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH 7/8] spi: sandbox: Fix memory leak in sandbox_sf_bind_emul()

[Simon Glass]

Hi Heinrich,

On 9 June 2018 at 11:54, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/09/2018 08:22 PM, Simon Glass wrote:
>> Move the strdup() call so that it is only done when we know we will bind
>> the device.
>>
>> Reported-by: Coverity (CID: 131216)
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  drivers/mtd/spi/sandbox.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c
>> index 7893efee12..ae29c034b9 100644
>> --- a/drivers/mtd/spi/sandbox.c
>> +++ b/drivers/mtd/spi/sandbox.c
>> @@ -567,14 +567,14 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
>>       strncpy(name, spec, sizeof(name) - 6);
>>       name[sizeof(name) - 6] = '\0';
>>       strcat(name, "-emul");
>> -     str = strdup(name);
>> -     if (!str)
>> -             return -ENOMEM;
>>       drv = lists_driver_lookup_name("sandbox_sf_emul");
>>       if (!drv) {
>>               puts("Cannot find sandbox_sf_emul driver\n");
>>               return -ENOENT;
>>       }
>> +     str = strdup(name);
>> +     if (!str)
>> +             return -ENOMEM;
>>       ret = device_bind(bus, drv, str, NULL, of_offset, &emul);
>>       if (ret) {
>>               printf("Cannot create emul device for spec '%s' (err=%d)\n",
>
> Aren't you leaking str's memory here? I would have expected free(str).

We need that for device_bind() which requires that the name be allocated for it.

Regards,
Simon

[U-Boot] [PATCH 3/8] sandbox: Use memcpy() to move overlapping regions

[Simon Glass]

Hi Heinrich,

On 9 June 2018 at 10:56, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/09/2018 08:22 PM, Simon Glass wrote:
>> The use of strcpy() to remove characters at the start of a string is safe
>> in U-Boot, since we know the implementation. But in os.c we are using the
>> C library's strcpy() function, where this behaviour is not permitted.
>>
>> Update the code to use memcpy() instead.
>>
>> Reported-by: Coverity (CID: 173279)
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  arch/sandbox/cpu/os.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
>> index 5839932b00..496a8f9bd8 100644
>> --- a/arch/sandbox/cpu/os.c
>> +++ b/arch/sandbox/cpu/os.c
>> @@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen)
>>       /* Look for 'u-boot' in the parent directory of spl/ */
>>       p = strstr(fname, "/spl/");
>
> I have built sandbox_spl_defconfig.
>
> First observation: any parallel build fails. But that is to be handled
> in a seperate patch.

I haven't seen that problem myself, but please send a patch or any
details you have.

>
> The logic to find u-boot here is broken. From the top directory of the
> git repository I executed:
>
> $ spl/u-boot-spl
>
> U-Boot SPL 2018.07-rc1-00063-g277daa5c0f (Jun 09 2018 - 20:40:23 +0200)
> Trying to boot from sandbox
> (spl/u-boot not found, error -2)
> SPL: failed to boot from all boot devices
> ### ERROR ### Please RESET the board ###
>
> Why would you expect fname to start with /spl/?
>
> In my case fname = 'spl/u-boot'. Why do you test for '/spl/' (only)?
>
> Just for the record if I execute
> $ ./spl/u-boot-spl
> everything works fine. But who would precede a directory name with './'?

It should be save enough to drop the leading / from the strstr(), if
that is what you are suggesting.

>
> Should we not concatenate the spl executable and the u-boot executable
> during the build process so that spl/u-boot knows where to find u-boot?
> That would be much safer than any assumption about relative paths.

We could do that, and there is support for doing this at a low level
(os_jump_to_image()). I've been working on some binman enhancements to
build a Chrome OS image, which would use that feature.

But executing from the filesystem seems reasonable to me, particularly
for running tests.

Regards,
Simon

>
> Best regards
>
> Heinrich
>
>>       if (p) {
>> -             strcpy(p, p + 4);
>> +             /* Remove the "/spl" characters */
>> +             memmove(p, p + 4, strlen(p + 4) + 1);
>>               fd = os_open(fname, O_RDONLY);
>>               if (fd >= 0) {
>>                       close(fd);
>>
>

[U-Boot] [PATCH 4/8] fdtgrep: Fix logic of free() in do_fdtgrep()

[Simon Glass]

Hi Heinrich,

On 9 June 2018 at 13:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/09/2018 08:22 PM, Simon Glass wrote:
>> This loop never actually exits, but the way the code is written this is
>> not obvious. Add an explicit error check.
>>
>> Reported-by: Coverity (CID: 131280)
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  tools/fdtgrep.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
>> index f2b8b71ed7..c4563e2289 100644
>> --- a/tools/fdtgrep.c
>> +++ b/tools/fdtgrep.c
>> @@ -801,7 +801,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename)
>>        * The first pass will count the regions, but if it is too many,
>>        * we do another pass to actually record them.
>>        */
>> -     for (i = 0; i < 3; i++) {
>> +     for (i = 0; i < 2; i++) {
>>               region = malloc(count * sizeof(struct fdt_region));
>>               if (!region) {
>>                       fprintf(stderr, "Out of memory for %d regions\n",
>
> Can't we call fdtgrep_find_regions() with max_regions = 0 and region =
> NULL to do the counting and get rid of the loop? That may be a bit
> slower but the code will be much easier to read.

Yes I think that would be possible. It would require a change to
fdtgrep_first_region since it assumes there is space for its region.

It seems like a good idea to me, but is beyond the scope of this coverity fix.

Regards,
Simon

[U-Boot] [PATCH 5/8] fdtgrep: Separate out checking of two allocations

[Simon Glass]

Hi Heinrich,

On 9 June 2018 at 13:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/09/2018 08:22 PM, Simon Glass wrote:
>> The current code might succeed on the first allocation and fail on the
>> second. Separate the checks to avoid this problem.
>>
>> Of course, free() will never fail and the chances that (when allocating
>> two small areas) one will succeed and one will fail are just as remote.
>> But this keeps coverity happy.
>>
>> Reported-by: Coverity (CID: 131226)
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  tools/fdtgrep.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
>> index c4563e2289..5593b42203 100644
>> --- a/tools/fdtgrep.c
>> +++ b/tools/fdtgrep.c
>> @@ -133,11 +133,11 @@ static int value_add(struct display_info *disp, struct value_node **headp,
>>       }
>>
>>       str = strdup(str);
>> +     if (!str)
>> +             goto err_mem;
>>       node = malloc(sizeof(*node));
>> -     if (!str || !node) {
>> -             fprintf(stderr, "Out of memory\n");
>> -             return -1;
>> -     }
>> +     if (!node)
>> +             goto err_mem;
>>       node->next = *headp;
>>       node->type = type;
>>       node->include = include;
>> @@ -145,6 +145,9 @@ static int value_add(struct display_info *disp, struct value_node **headp,
>>       *headp = node;
>>
>
> free(str) is missing here.

If we free the string then the node will be invalid. That is the point
of the strdup(), so that the node can have a copy of the string.

>
>
>>       return 0;
>> +err_mem:
>
> free(str) is missing here.

But the strdup() failed, so str is NULL and there is nothing to free.

>
> Best regards
>
> Heinrich
>
>> +     fprintf(stderr, "Out of memory\n");
>> +     return -1;
>>  }
>>
>>  static bool util_is_printable_string(const void *data, int len)
>>
>

Regards,
Simon

[U-Boot] [PATCH 6/8] rsa: Fix missing memory leak on error in fdt_add_bignum()

[Simon Glass]

Hi Heinrich,

On 9 June 2018 at 13:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/09/2018 08:22 PM, Simon Glass wrote:
>> Thsi function can fail without freeing all its memory. Fix it.
>>
>> Reported-by: Coverity (CID: 131217)
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  lib/rsa/rsa-sign.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
>> index d2788bf79a..2a09d2b19e 100644
>> --- a/lib/rsa/rsa-sign.c
>> +++ b/lib/rsa/rsa-sign.c
>> @@ -667,15 +667,13 @@ static int fdt_add_bignum(void *blob, int noffset, const char *prop_name,
>>        * might fail several times
>
> Please, fix the memory leaks above this line too.

Coverity does not seem to detect these as memory leaks.

I've added a comment explaining why I don't think it makes sense to 'fix' these.

Regards,
Simon

[U-Boot] [PATCH 7/8] spi: sandbox: Fix memory leak in sandbox_sf_bind_emul()

[Simon Glass]

Hi Heinrich,

On 10 June 2018 at 05:35, Simon Glass <sjg@chromium.org> wrote:
> Hi Heinrich,
>
> On 9 June 2018 at 11:54, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 06/09/2018 08:22 PM, Simon Glass wrote:
>>> Move the strdup() call so that it is only done when we know we will bind
>>> the device.
>>>
>>> Reported-by: Coverity (CID: 131216)
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>>  drivers/mtd/spi/sandbox.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c
>>> index 7893efee12..ae29c034b9 100644
>>> --- a/drivers/mtd/spi/sandbox.c
>>> +++ b/drivers/mtd/spi/sandbox.c
>>> @@ -567,14 +567,14 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
>>>       strncpy(name, spec, sizeof(name) - 6);
>>>       name[sizeof(name) - 6] = '\0';
>>>       strcat(name, "-emul");
>>> -     str = strdup(name);
>>> -     if (!str)
>>> -             return -ENOMEM;
>>>       drv = lists_driver_lookup_name("sandbox_sf_emul");
>>>       if (!drv) {
>>>               puts("Cannot find sandbox_sf_emul driver\n");
>>>               return -ENOENT;
>>>       }
>>> +     str = strdup(name);
>>> +     if (!str)
>>> +             return -ENOMEM;
>>>       ret = device_bind(bus, drv, str, NULL, of_offset, &emul);
>>>       if (ret) {
>>>               printf("Cannot create emul device for spec '%s' (err=%d)\n",
>>
>> Aren't you leaking str's memory here? I would have expected free(str).
>
> We need that for device_bind() which requires that the name be allocated for it.

OK I see what you mean now. If device_bind() fails it should be safe
to free it, since it is not supposed to leave anything around from its
failure to bind the device.

Regards,
Simon