pxe: clear Bootfile before returning

PATCH In review Stephen Warren 3 participants 6 messages Jul 23, 2014 – Aug 10, 2014

lore

Share & tools

lore

Stephen Warren

author

2014-07-23 00:06 UTC #0 lore raw

PATCH 1/1 pxe: clear Bootfile before returning

From: Stephen Warren <[email protected]>

When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves the downloaded filename to global variable BootFile. If the boot operation is aborted, this global state is not cleared. If "dhcp" is executed later without any arguments, BootFile is not cleared, and when the DHCP response is received, BootpCopyNetParams() writes the value into environment variable bootfile.

This causes the following scenario:

* Boot script executes dhcp; pxe get; pxe boot

* User CTRL-C's the PXE menu, which causes the first menu item to be booted, which causes some file to be downloaded.

(This boot-on-CTRL-C behaviour is arguably a bug too, but it's a separate bug and the bug this patch fixes would still exist if the user simply waited to press CTRL-C until "pxe boot" started downloading files)

* User CTRL-C's the file downloads, but the filename is still written to the bootfile environment variable.

* User re-runs the boot command, which in my case executes "dhcp; pxe get; pxe boot" again, and "dhcp" picks up the saved bootfile environment variable and proceeds to download a file that it shouldn't.

To solve this, modify the implementation of "pxe get" to clear BootFile if the whole boot operation fails, which avoids this whole mess.

An alternative would be to modify netboot_common() such that the no- arguments case explicitly clears the global variable BootFile. However, that would prevent the following command sequences from working:

$ dhcp filename # downloads "filename"
$ dhcp          # downloads $bootfile, i.e. "filename"

or:

$ setenv bootfile filename
$ dhcp          # downloads $bootfile, i.e. "filename"

... and I assume someone relies on U-Boot working that way.

Signed-off-by Stephen Warren <[email protected]>

---
 common/cmd_pxe.c | 2 ++
 1 file changed, 2 insertions(+)

common/cmd_pxe.c +2 -0

index ba48692e8641..28999f573496 100644
@@ -1554,6 +1554,8 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	destroy_pxe_menu(cfg);
 
+	copy_filename(BootFile, "", sizeof(BootFile));
+
 	return 0;
 }

Signature · 1 line

1.8.1.5

Joe Hershberger

2014-07-23 21:37 UTC #1 lore raw

PATCH 1/1 pxe: clear Bootfile before returning

Quoted 39 lines · Stephen Warren

From: Stephen Warren <[email protected]>
When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves the downloaded filename to global variable BootFile. If the boot operation is aborted, this global state is not cleared. If "dhcp" is executed later without any arguments, BootFile is not cleared, and when the DHCP response is received, BootpCopyNetParams() writes the value into environment variable bootfile.
This causes the following scenario:
* Boot script executes dhcp; pxe get; pxe boot
* User CTRL-C's the PXE menu, which causes the first menu item to be booted, which causes some file to be downloaded.
(This boot-on-CTRL-C behaviour is arguably a bug too, but it's a separate bug and the bug this patch fixes would still exist if the user simply waited to press CTRL-C until "pxe boot" started downloading files)
* User CTRL-C's the file downloads, but the filename is still written to the bootfile environment variable.
* User re-runs the boot command, which in my case executes "dhcp; pxe get; pxe boot" again, and "dhcp" picks up the saved bootfile environment variable and proceeds to download a file that it shouldn't.
To solve this, modify the implementation of "pxe get" to clear BootFile if the whole boot operation fails, which avoids this whole mess.
An alternative would be to modify netboot_common() such that the no- arguments case explicitly clears the global variable BootFile. However, that would prevent the following command sequences from working:
$ dhcp filename # downloads "filename"
$ dhcp          # downloads $bootfile, i.e. "filename"

Quoted 7 lines

or:
$ setenv bootfile filename
$ dhcp          # downloads $bootfile, i.e. "filename"
... and I assume someone relies on U-Boot working that way.
Signed-off-by Stephen Warren <[email protected]>

Acked-by Joe Hershberger <[email protected]>

Stephen Warren

author

2014-07-23 21:41 UTC #2 lore raw

PATCH 1/1 pxe: clear Bootfile before returning

Quoted 12 lines · Joe Hershberger

Quoted 9 lines · On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren <[email protected]…
From: Stephen Warren <swarren at nvidia.com <mailto:[email protected]>>
When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves the downloaded filename to global variable BootFile. If the boot operation is aborted, this global state is not cleared. If "dhcp" is executed later without any arguments, BootFile is not cleared, and when the DHCP response is received, BootpCopyNetParams() writes the value into environment variable bootfile.

...

Quoted 2 lines

Acked-by Joe Hershberger <[email protected]
<mailto:[email protected]>>

Thanks. I'm not sure if you ack'd this simply so you'd remember you'd reviewed it, or because you expect someone else to apply the change? If the latter, I should forward the patch to them so they know about it:-)

Joe Hershberger

2014-07-23 21:56 UTC #3 lore raw

PATCH 1/1 pxe: clear Bootfile before returning

Quoted 6 lines · Stephen Warren

Quoted 4 lines · Joe Hershberger
Quoted 1 line · On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren <[email protected]…

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
From: Stephen Warren <swarren at nvidia.com <mailto:[email protected]>>
When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves the downloaded filename to global variable BootFile. If the boot operation is aborted, this global state is not cleared. If "dhcp" is executed later without any arguments, BootFile is not cleared, and when the DHCP response is received, BootpCopyNetParams() writes the value

into

Quoted 8 lines

Quoted 1 line
Quoted 1 line
environment variable bootfile.
...
Quoted 2 lines
Acked-by Joe Hershberger <[email protected]
<mailto:[email protected]>>
Thanks. I'm not sure if you ack'd this simply so you'd remember you'd reviewed it, or because you expect someone else to apply the change? If the latter, I should forward the patch to them so they know about it:-)

Partly for me to remember and partly because recently Tom has been picking these few net things up directly.

Cheers
-Joe

Stephen Warren

author

2014-08-06 15:56 UTC #4 lore raw

PATCH 1/1 pxe: clear Bootfile before returning

Quoted 26 lines · Joe Hershberger

Quoted 4 lines · On Wed, Jul 23, 2014 at 4:41 PM, Stephen Warren <[email protected]…
Quoted 2 lines · Joe Hershberger
On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren
<swarren at wwwdotorg.org <mailto:[email protected]>
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines · <mailto:swarren at wwwdotorg.org <mailto:[email protected]>>> wrote:
From: Stephen Warren <[email protected]
<mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
When "pxe boot" downloads the initrd/kernel/DTB, netboot_common()
saves
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
the downloaded filename to global variable BootFile. If the boot operation is aborted, this global state is not cleared. If "dhcp" is executed later without any arguments, BootFile is not cleared, and
when
Quoted 1 line
Quoted 1 line
Quoted 1 line
the DHCP response is received, BootpCopyNetParams() writes the
value into
Quoted 3 lines
Quoted 1 line
Quoted 1 line
environment variable bootfile.
...
Quoted 1 line
Acked-by Joe Hershberger <[email protected]
<mailto:[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
<mailto:joe.hershberger at ni.com <mailto:[email protected]>>>
Thanks. I'm not sure if you ack'd this simply so you'd remember you'd reviewed it, or because you expect someone else to apply the change? If the latter, I should forward the patch to them so they know about it:-)
Partly for me to remember and partly because recently Tom has been picking these few net things up directly.

Tom, are you planning on picking up this patch?

Tom Rini

2014-08-10 22:21 UTC #5 lore raw

In reply to Stephen Warren

Quoted 38 lines · Stephen Warren

From: Stephen Warren <[email protected]>
When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves the downloaded filename to global variable BootFile. If the boot operation is aborted, this global state is not cleared. If "dhcp" is executed later without any arguments, BootFile is not cleared, and when the DHCP response is received, BootpCopyNetParams() writes the value into environment variable bootfile.
This causes the following scenario:
* Boot script executes dhcp; pxe get; pxe boot
* User CTRL-C's the PXE menu, which causes the first menu item to be booted, which causes some file to be downloaded.
(This boot-on-CTRL-C behaviour is arguably a bug too, but it's a separate bug and the bug this patch fixes would still exist if the user simply waited to press CTRL-C until "pxe boot" started downloading files)
* User CTRL-C's the file downloads, but the filename is still written to the bootfile environment variable.
* User re-runs the boot command, which in my case executes "dhcp; pxe get; pxe boot" again, and "dhcp" picks up the saved bootfile environment variable and proceeds to download a file that it shouldn't.
To solve this, modify the implementation of "pxe get" to clear BootFile if the whole boot operation fails, which avoids this whole mess.
An alternative would be to modify netboot_common() such that the no- arguments case explicitly clears the global variable BootFile. However, that would prevent the following command sequences from working:
$ dhcp filename # downloads "filename"
$ dhcp          # downloads $bootfile, i.e. "filename"

Quoted 8 lines

or:
$ setenv bootfile filename
$ dhcp          # downloads $bootfile, i.e. "filename"
... and I assume someone relies on U-Boot working that way.
Signed-off-by Stephen Warren <[email protected]>
Acked-by Joe Hershberger <[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/20140810/432505eb/attachment.pgp>