[U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[Jonathan Golder]

Passing NULL to fs_read() for actread value results in hanging U-Boot
at least on our ARM plattform (TI AM335x). Since fs_read() and
following functions do not catch nullpointers, writing to 0x0 occurs.

Passing a local dummy var instead of NULL solves this issue.

Signed-off-by: Jonathan Golder <jonathan.golder@kurz-elektronik.de>
Cc: Anatolij Gustschin <agust@denx.de>
---
 common/splash_source.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/splash_source.c b/common/splash_source.c
index a5eeb3f..d1647c8 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -216,6 +216,7 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
 {
 	int res = 0;
 	loff_t bmp_size;
+	loff_t actread;
 	char *splash_file;
 
 	splash_file = getenv("splashfile");
@@ -251,7 +252,7 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
 	}
 
 	splash_select_fs_dev(location);
-	res = fs_read(splash_file, bmp_load_addr, 0, 0, NULL);
+	res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
 
 out:
 	if (location->ubivol != NULL)
-- 
1.9.1

[U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[Igor Grinberg]

Hi Jonathan,

On 02/24/17 18:46, Jonathan Golder wrote:
> Passing NULL to fs_read() for actread value results in hanging U-Boot
> at least on our ARM plattform (TI AM335x). Since fs_read() and
> following functions do not catch nullpointers, writing to 0x0 occurs.
> 
> Passing a local dummy var instead of NULL solves this issue.

I haven't looked at fs_read() yet, but from the above it seems
that a better approach would be to fix the fs_read()?
Might there be use cases when it is legitimate to pass NULL?

> 
> Signed-off-by: Jonathan Golder <jonathan.golder@kurz-elektronik.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
>  common/splash_source.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/common/splash_source.c b/common/splash_source.c
> index a5eeb3f..d1647c8 100644
> --- a/common/splash_source.c
> +++ b/common/splash_source.c
> @@ -216,6 +216,7 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
>  {
>  	int res = 0;
>  	loff_t bmp_size;
> +	loff_t actread;
>  	char *splash_file;
>  
>  	splash_file = getenv("splashfile");
> @@ -251,7 +252,7 @@ static int splash_load_fs(struct splash_location *location, u32 bmp_load_addr)
>  	}
>  
>  	splash_select_fs_dev(location);
> -	res = fs_read(splash_file, bmp_load_addr, 0, 0, NULL);
> +	res = fs_read(splash_file, bmp_load_addr, 0, 0, &actread);
>  
>  out:
>  	if (location->ubivol != NULL)
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[Igor Grinberg]

Hi Jonathan,

On 02/27/17 10:10, Jonathan Golder wrote:
> Hi Igor,
> 
>> I haven't looked at fs_read() yet, but from the above it seems that
>> a better approach would be to fix the fs_read()? Might there be use
>> cases when it is legitimate to pass NULL?
> 
> 
> Well, actually I have not dived into the depths of the fs_read ()
> deeper than realising that something went wrong.
> 
> Before posting this patch I greped for usages of fs_read() with NULL
> as last parameter and found only this occurrence. So I supposed that
> passing NULL for actread is not an intended use case of fs_read ().

Probably, yet this use case appeared and correct me if I'm wrong,
the splash source code does not really care of that parameter, and
it wants to use the fs_read().
So maybe the problem is in fs_read() API not covering a legitimate
use case?
Of course this patch will fix the problem by hiding it till next time.

-- 
Regards,
Igor.

[U-Boot] [PATCH] splash: Prevent splash_load_fs from writing to 0x0

[Anatolij Gustschin]

On Fri, 24 Feb 2017 17:46:10 +0100
Jonathan Golder jonathan.golder at kurz-elektronik.de wrote:

> Passing NULL to fs_read() for actread value results in hanging U-Boot
> at least on our ARM plattform (TI AM335x). Since fs_read() and
> following functions do not catch nullpointers, writing to 0x0 occurs.
> 
> Passing a local dummy var instead of NULL solves this issue.
> 
> Signed-off-by: Jonathan Golder <jonathan.golder@kurz-elektronik.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
>  common/splash_source.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

applied to u-boot-video/master, thanks!

--
Anatolij