Re: [PATCH 1/3] boot: fdt: Change type of env_get_bootm_low() to phys_addr_t

[Marek Vasut]

On 3/17/24 12:18 PM, Laurent Pinchart wrote:

Hi Marek,

Thank you for the patch.

On Sun, Mar 17, 2024 at 07:16:29AM +0100, Marek Vasut wrote:

Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low().
The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t,
while the function itself still returns ulong. This is potentially dangerous
on 64bit systems, where ulong might not be large enough to hold the content
of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to
what env_get_bootm_size() does, which returns phys_size_t .

Reported-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Signed-off-by: Marek Vasut <marek.vasut+rene...@mailbox.org>
---
Cc: Heinrich Schuchardt <xypron.g...@gmx.de>
Cc: Kuninori Morimoto <kuninori.morimoto...@renesas.com>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Simon Glass <s...@chromium.org>
Cc: Tom Rini <tr...@konsulko.com>
---
  boot/bootm.c       |  2 +-
  boot/image-board.c | 11 ++++++-----
  boot/image-fdt.c   |  9 +++++----
  include/image.h    |  2 +-
  4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index d071537d692..2e818264f5f 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -242,7 +242,7 @@ static int boot_get_kernel(const char *addr_fit, struct 
bootm_headers *images,
  #ifdef CONFIG_LMB
  static void boot_start_lmb(struct bootm_headers *images)
  {
-       ulong           mem_start;
+       phys_addr_t     mem_start;
        phys_size_t     mem_size;

mem_start = env_get_bootm_low();

diff --git a/boot/image-board.c b/boot/image-board.c
index 75f6906cd56..447ced2678a 100644
--- a/boot/image-board.c
+++ b/boot/image-board.c
@@ -107,12 +107,13 @@ static int on_loadaddr(const char *name, const char 
*value, enum env_op op,
  }
  U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr);

-ulong env_get_bootm_low(void)

+phys_addr_t env_get_bootm_low(void)
  {
        char *s = env_get("bootm_low");
+       phys_size_t tmp;

if (s) {

-               ulong tmp = hextoul(s, NULL);
+               tmp = (phys_addr_t)simple_strtoull(s, NULL, 16);
                return tmp;

Can't you write

                return (phys_addr_t)simple_strtoull(s, NULL, 16);

and avoid the temporary variable ?

Fixed in V2, thanks