[PATCH 1/1] cmd: fdt: check fdt address
Trying to read a device-tree from an illegal address leads to a crash.
Check that the parameter passed to 'fdt addr' is within the RAM area and non-zero.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- cmd/fdt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/cmd/fdt.c b/cmd/fdt.c index 331564c13b..dc954ea7d5 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -193,6 +193,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) }
addr = hextoul(argv[0], NULL); + if (!addr || addr < gd->ram_base || addr >= gd->ram_top) { + printf("Invalid address\n"); + return CMD_RET_FAILURE; + } + blob = map_sysmem(addr, 0); if ((quiet && fdt_check_header(blob)) || (!quiet && !fdt_valid(&blob)))
On Thu, Dec 21, 2023 at 01:00:11AM +0100, Heinrich Schuchardt wrote:
Trying to read a device-tree from an illegal address leads to a crash.
Check that the parameter passed to 'fdt addr' is within the RAM area and non-zero.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/fdt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/cmd/fdt.c b/cmd/fdt.c index 331564c13b..dc954ea7d5 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -193,6 +193,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) }
addr = hextoul(argv[0], NULL);
if (!addr || addr < gd->ram_base || addr >= gd->ram_top) {printf("Invalid address\n");return CMD_RET_FAILURE;}- blob = map_sysmem(addr, 0); if ((quiet && fdt_check_header(blob)) || (!quiet && !fdt_valid(&blob)))
As I mentioned on IRC, we don't sanity check addresses being within memory normally. The problem (and one that is making me cranky) is that it seems a number of RISC-V platforms have invalid kernel/fdt/initrd addresses set in the environment and then have PREBOOT used to fix fdt_addr_r so that distro_bootcmd doesn't blow up.
Hi Heinrich,
On Wed, Dec 20, 2023 at 5:00 PM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Trying to read a device-tree from an illegal address leads to a crash.
Check that the parameter passed to 'fdt addr' is within the RAM area and non-zero.
What is the motivation for this patch? Is something crashing? We don't do this with the 'md' command, for example.
Regards, Simon
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/fdt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/cmd/fdt.c b/cmd/fdt.c index 331564c13b..dc954ea7d5 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -193,6 +193,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) }
addr = hextoul(argv[0], NULL);
if (!addr || addr < gd->ram_base || addr >= gd->ram_top) {printf("Invalid address\n");return CMD_RET_FAILURE;}blob = map_sysmem(addr, 0); if ((quiet && fdt_check_header(blob)) || (!quiet && !fdt_valid(&blob)))-- 2.40.1
On 12/31/23 15:22, Simon Glass wrote:
Hi Heinrich,
On Wed, Dec 20, 2023 at 5:00 PM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Trying to read a device-tree from an illegal address leads to a crash.
Check that the parameter passed to 'fdt addr' is within the RAM area and non-zero.
What is the motivation for this patch? Is something crashing? We don't do this with the 'md' command, for example.
QEMU with too little memory was crashing. But maybe we should write an explicit warning about too little memory instead.
Best regards
Heinrich
Regards, Simon
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/fdt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/cmd/fdt.c b/cmd/fdt.c index 331564c13b..dc954ea7d5 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -193,6 +193,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) }
addr = hextoul(argv[0], NULL);
if (!addr || addr < gd->ram_base || addr >= gd->ram_top) {printf("Invalid address\n");return CMD_RET_FAILURE;}blob = map_sysmem(addr, 0); if ((quiet && fdt_check_header(blob)) || (!quiet && !fdt_valid(&blob)))-- 2.40.1
Hi Heinrich,
On Sun, Dec 31, 2023 at 9:31 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 12/31/23 15:22, Simon Glass wrote:
Hi Heinrich,
On Wed, Dec 20, 2023 at 5:00 PM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Trying to read a device-tree from an illegal address leads to a crash.
Check that the parameter passed to 'fdt addr' is within the RAM area and non-zero.
What is the motivation for this patch? Is something crashing? We don't do this with the 'md' command, for example.
QEMU with too little memory was crashing. But maybe we should write an explicit warning about too little memory instead.
Probably.
[..]
Regards, Simon
participants (3)
-
Heinrich Schuchardt -
Simon Glass -
Tom Rini