catch wrong load address passed to fatload / ext2load

PATCH In review Pavel Machek 4 participants 7 messages Jul 9, 2014 – Jul 22, 2014 VFS

Share & tools

Pavel Machek

author

2014-07-09 20:42 UTC #0 lore raw

PATCH 1/1 catch wrong load address passed to fatload / ext2load

If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.

Signed-off-by Pavel Machek <[email protected]>

fs/fs.c +4 -1

index 79d432d..ea15c5f 100644
@@ -276,6 +276,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	unsigned long pos;
 	int len_read;
 	unsigned long time;
+	char *ep;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;

@@ -286,7 +287,9 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		return 1;
 
 	if (argc >= 4) {
-		addr = simple_strtoul(argv[3], NULL, 16);
+		addr = simple_strtoul(argv[3], &ep, 16);
+		if (ep == argv[3] || *ep != '\0')
+			return CMD_RET_USAGE;
 	} else {
 		addr_str = getenv("loadaddr");
 		if (addr_str != NULL)

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pavel Machek

author

2014-07-09 20:46 UTC #1 lore raw

PATCH 1/1 catch wrong load address passed to fatload / ext2load

Hi!

Quoted 5 lines

If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.
Signed-off-by Pavel Machek <[email protected]>

Oops and actually I should warn... this patch is untested. I'm currently fighting hard to get recent u-boot to run on socfpga.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Vasut

2014-07-09 21:07 UTC #2 lore raw

PATCH 1/1 catch wrong load address passed to fatload / ext2load

In reply to Pavel Machek

Quoted 5 lines · Pavel Machek

If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.
Signed-off-by Pavel Machek <[email protected]>

The handling of simple_strtoul() in fs.c could use improvement like this in general. Do you mind expanding this patch ?

Best regards,
Marek Vasut

Pavel Machek

author

2014-07-09 22:57 UTC #3 lore raw

PATCH 1/1 catch wrong load address passed to fatload / ext2load

Quoted 9 lines · Marek Vasut

Quoted 5 lines · Pavel Machek
If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.
Signed-off-by Pavel Machek <[email protected]>
The handling of simple_strtoul() in fs.c could use improvement like this in general. Do you mind expanding this patch ?

I'd prefer not to do that at this moment: I don't have working-enough u-boot and hate to do changes without testing.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Wolfgang Denk

2014-07-09 23:08 UTC #4 lore raw

PATCH 1/1 catch wrong load address passed to fatload / ext2load

In reply to Pavel Machek

Dear Pavel Machek,

Quoted 20 lines · In message <[email protected]> you wrote:

If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.
Signed-off-by Pavel Machek <[email protected]>
fs/fs.c +1 -0
index 79d432d..ea15c5f 100644
@@ -276,6 +276,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	unsigned long pos;
 	int len_read;
 	unsigned long time;
+	char *ep;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;

fs/fs.c int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], · line 292

Quoted 7 lines

fs/fs.c +3 -1

@@ -286,7 +287,9 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		return 1;
 
 	if (argc >= 4) {
-		addr = simple_strtoul(argv[3], NULL, 16);
+		addr = simple_strtoul(argv[3], &ep, 16);
+		if (ep == argv[3] || *ep != '\0')
+			return CMD_RET_USAGE;

What happens in case of filenames that look like numbers, say "0"? It may be silly to use such, but it should not be impossible to do so.

Viele Gr??e,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
One difference between a man and a machine is that a machine is quiet
when well oiled.

Pavel Machek

author

2014-07-09 23:12 UTC #5 lore raw

PATCH 1/1 catch wrong load address passed to fatload / ext2load

Quoted 25 lines · Wolfgang Denk

Dear Pavel Machek,
Quoted 22 lines · In message <[email protected]> you wrote:
If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.
Signed-off-by Pavel Machek <[email protected]>
fs/fs.c +1 -0
index 79d432d..ea15c5f 100644
@@ -276,6 +276,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	unsigned long pos;
 	int len_read;
 	unsigned long time;
+	char *ep;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -286,7 +287,9 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		return 1;
 

Quoted 9 lines

fs/fs.c int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], · line 292
Quoted 5 lines
fs/fs.c +3 -1
 	if (argc >= 4) {
-		addr = simple_strtoul(argv[3], NULL, 16);
+		addr = simple_strtoul(argv[3], &ep, 16);
+		if (ep == argv[3] || *ep != '\0')
+			return CMD_RET_USAGE;
What happens in case of filenames that look like numbers, say "0"? It may be silly to use such, but it should not be impossible to do so.

It should be ok.

This catches error when someone passes non-number as a load address. As long as you pass load address, nothing changes.

IOW

fatload mmc 0:1 0 filename

# always worked/keeps working
fatload mmc 0:1 filename
# before my patch, this would try to parse filename as number, fail,
# and use default load address adn default filename. Not what user
# wanted.
fatload mmc 0:1 0 0
# Load filename "0" at address 0. Worked before, will work now.

Hope this helps,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Tom Rini

2014-07-22 19:22 UTC #6 lore raw

In reply to Pavel Machek

Quoted 5 lines · Pavel Machek

If filename is passed instead of address to ext2load or fatload, u-boot silently accepts that, and uses 0 for load address and default filename from environment. That is confusing, display help instead.
Signed-off-by Pavel Machek <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140722/cdb455c9/attachment.pgp>