From: Ana Bobillo <[email protected]>
Since commit aa3c609e2be5 ("fs: prevent overwriting reserved memory"),
which fixed CVE-2018-18440, the load command refuses to read a file over
reserved memory, such as U-Boot's own code, stack and heap. The bootmeths
and the sysboot command read files with fs_read() instead, which does
not have that check:
- bootmeth_common_read_file() only checks the size of the file against
a limit given by the caller, which is 1GB for extlinux and EFI
- sysboot_read_file() does not check the size at all. sysboot used the
load commands, and so had the check, until
commit 81a2f8d34b4e ("pxe: Refactor sysboot to have one helper")
So a large kernel, initrd, FDT or config file on the boot media, read to
an address taken from the environment, such as ${kernel_addr_r}, can be
written over U-Boot itself.
Export the read which the load command uses as fs_read_checked() and use
it in do_load(), bootmeth_common_read_file() and sysboot_read_file().
Close the filesystem when the check fails, as a successful read does,
since a bootflow scan can now fail the check many times.
Bootmeths now refuse to read a file over reserved memory just as the
load command does, so a board whose environment puts
${kernel_addr_r} or similar in a reserved region will see an error where
the file was written over that region before.
Add a test which tries to read a file to reserved memory with a bootmeth
and with sysboot.
Fixes: a950d31abe98 ("bootstd: Add the bootmeth uclass and helpers")
Fixes: 81a2f8d34b4e ("pxe: Refactor sysboot to have one helper")
Signed-off-by: Ana Bobillo <[email protected]>
---
(no changes since v1)
boot/bootmeth-uclass.c | 2 +-
cmd/sysboot.c | 2 +-
fs/fs.c | 12 +++++--
include/fs.h | 19 +++++++++++
test/boot/bootmeth.c | 75 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 106 insertions(+), 4 deletions(-)
diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
index c841dd0d6d4..b0d0c3a4e38 100644
--- a/boot/bootmeth-uclass.c
+++ b/boot/bootmeth-uclass.c
@@ -449,7 +449,7 @@ int bootmeth_common_read_file(struct udevice *dev, struct
bootflow *bflow,
if (ret)
return log_msg_ret("fs", ret);
- ret = fs_read(file_path, addr, 0, 0, &len_read);
+ ret = fs_read_checked(file_path, addr, 0, 0, &len_read);
if (ret)
return ret;
*sizep = len_read;
diff --git a/cmd/sysboot.c b/cmd/sysboot.c
index 93d4a400830..1e78767eec2 100644
--- a/cmd/sysboot.c
+++ b/cmd/sysboot.c
@@ -35,7 +35,7 @@ static int sysboot_read_file(struct pxe_context *ctx, const
char *file_path,
ret = fs_set_blk_dev(info->ifname, info->dev_part_str, info->fstype);
if (ret)
return ret;
- ret = fs_read(file_path, addr, 0, 0, &len_read);
+ ret = fs_read_checked(file_path, addr, 0, 0, &len_read);
if (ret)
return ret;
*sizep = len_read;
diff --git a/fs/fs.c b/fs/fs.c
index b18ccec8cb4..a8574b30c7f 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -682,8 +682,10 @@ static int _fs_read(const char *filename, ulong addr,
loff_t offset, loff_t len,
#if CONFIG_IS_ENABLED(LMB)
if (do_lmb_check) {
ret = fs_read_lmb_check(filename, addr, offset, len, info);
- if (ret)
+ if (ret) {
+ fs_close();
return ret;
+ }
}
#endif
@@ -709,6 +711,12 @@ int fs_read(const char *filename, ulong addr, loff_t
offset, loff_t len,
return _fs_read(filename, addr, offset, len, 0, actread);
}
+int fs_read_checked(const char *filename, ulong addr, loff_t offset,
+ loff_t len, loff_t *actread)
+{
+ return _fs_read(filename, addr, offset, len, 1, actread);
+}
+
int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
loff_t *actwrite)
{
@@ -911,7 +919,7 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char
*const argv[],
pos = 0;
time = get_timer(0);
- ret = _fs_read(filename, addr, pos, bytes, 1, &len_read);
+ ret = fs_read_checked(filename, addr, pos, bytes, &len_read);
time = get_timer(time);
if (ret < 0) {
log_err("Failed to load '%s'\n", filename);
diff --git a/include/fs.h b/include/fs.h
index 8afc81c0017..28d8a7cc98b 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -152,6 +152,25 @@ int fs_size(const char *filename, loff_t *size);
int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
loff_t *actread);
+/**
+ * fs_read_checked() - read a file without overwriting reserved memory
+ *
+ * This is like fs_read() but, as the 'load' command does, it refuses to read
+ * a file over memory that is reserved (if LMB is enabled), such as U-Boot
+ * itself. Use this to read a file whose size is not under the caller's
+ * control to an address that is not a buffer the caller allocated.
+ *
+ * @filename: full path of the file to read from
+ * @addr: address of the buffer to write to
+ * @offset: offset in the file from where to start reading
+ * @len: the number of bytes to read. Use 0 to read entire file.
+ * @actread: returns the actual number of bytes read
+ * Return: 0 if OK with valid @actread, -ENOSPC if the file would overwrite
+ * reserved memory, other -ve value on other error
+ */
+int fs_read_checked(const char *filename, ulong addr, loff_t offset,
+ loff_t len, loff_t *actread);
+
/**
* fs_write() - write file to the partition previously set by fs_set_blk_dev()
*
diff --git a/test/boot/bootmeth.c b/test/boot/bootmeth.c
index 8400a2e7879..09fc21bbc1b 100644
--- a/test/boot/bootmeth.c
+++ b/test/boot/bootmeth.c
@@ -6,10 +6,16 @@
* Written by Simon Glass <[email protected]>
*/
+#include <bootflow.h>
#include <bootmeth.h>
#include <bootstd.h>
+#include <command.h>
#include <dm.h>
#include <env.h>
+#include <lmb.h>
+#include <mapmem.h>
+#include <vsprintf.h>
+#include <linux/sizes.h>
#include <test/ut.h>
#include "bootstd_common.h"
@@ -228,3 +234,72 @@ static int bootmeth_state(struct unit_test_state *uts)
return 0;
}
BOOTSTD_TEST(bootmeth_state, UTF_DM | UTF_SCAN_FDT);
+
+/* Check for the message given when a file would overwrite reserved memory */
+static int check_reserved_msg(struct unit_test_state *uts)
+{
+ if (IS_ENABLED(CONFIG_LOGF_FUNC))
+ ut_assert_nextline(
+ " fs_read_lmb_check() ** Reading file would overwrite
reserved memory **");
+ else
+ ut_assert_nextline("** Reading file would overwrite reserved
memory **");
+
+ return 0;
+}
+
+/* Check that reading a file cannot overwrite reserved memory */
+static int bootmeth_read_reserved(struct unit_test_state *uts)
+{
+ const ulong size = SZ_4K;
+ struct bootstd_priv *std;
+ struct bootflow *bflow;
+ phys_addr_t addr;
+ char cmd[80];
+ ulong limit;
+ char *buf;
+ int i;
+
+ if (!CONFIG_IS_ENABLED(LMB))
+ return -EAGAIN;
+
+ ut_assertok(run_command("bootdev select 1", 0));
+ ut_assertok(run_command("bootflow scan", 0));
+ ut_assertok(run_command("bootflow select 0", 0));
+ ut_assertok(bootstd_get_priv(&std));
+ bflow = std->cur_bootflow;
+ ut_assertnonnull(bflow);
+ ut_assert(bflow->size <= size);
+
+ /* Reserve some memory, as U-Boot does for itself */
+ ut_assertok(lmb_alloc_mem(LMB_MEM_ALLOC_ANY, SZ_4K, &addr, size,
+ LMB_NOOVERWRITE));
+ buf = map_sysmem(addr, size);
+ memset(buf, 0xaa, size);
+ console_record_reset_enable();
+
+ limit = size;
+ ut_asserteq(-ENOSPC, bootmeth_read_file(bflow->method, bflow,
+ bflow->fname, addr,
+ BFI_EXTLINUX_CFG, &limit));
+ ut_assertok(check_reserved_msg(uts));
+ ut_assert_console_end();
+
+ if (IS_ENABLED(CONFIG_CMD_SYSBOOT)) {
+ snprintf(cmd, sizeof(cmd), "sysboot mmc 1:1 any %lx %s",
+ (ulong)addr, bflow->fname);
+ ut_asserteq(1, run_command(cmd, 0));
+ ut_assert_nextline("Retrieving file: %s", bflow->fname);
+ ut_assertok(check_reserved_msg(uts));
+ ut_assert_nextline("Error reading config file");
+ ut_assert_console_end();
+ ut_assertok(env_set("bootfile", NULL));
+ }
+
+ for (i = 0; i < size; i++)
+ ut_asserteq(0xaa, (u8)buf[i]);
+ unmap_sysmem(buf);
+ ut_assertok(lmb_free(addr, size, LMB_NOOVERWRITE));
+
+ return 0;
+}
+BOOTSTD_TEST(bootmeth_read_reserved, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE);
