[PATCH 1/1] spl: spl_legacy: simplify spl_parse_legacy_validate
The check for an overlap of the loaded image and SPL is overly complicated.
Fixes: 77aed22b48ab ("spl: spl_legacy: Add extra address checks") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- common/spl/spl_legacy.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 095443c63d..9246f555e3 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -22,10 +22,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size;
- if ((start >= spl_start && start < spl_end) || - (end > spl_start && end <= spl_end) || - (start < spl_start && end >= spl_end) || - (start > end && end > spl_start)) + if (end > spl_start && start < spl_end) panic("SPL: Image overlaps SPL\n");
if (size > CONFIG_SYS_BOOTM_LEN)
On 7/22/23 20:46, Heinrich Schuchardt wrote:
The check for an overlap of the loaded image and SPL is overly complicated.
Fixes: 77aed22b48ab ("spl: spl_legacy: Add extra address checks") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
common/spl/spl_legacy.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 095443c63d..9246f555e3 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -22,10 +22,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size;
- if ((start >= spl_start && start < spl_end) ||
(end > spl_start && end <= spl_end) ||(start < spl_start && end >= spl_end) ||(start > end && end > spl_start))
if (end > spl_start && start < spl_end) panic("SPL: Image overlaps SPL\n");
if (size > CONFIG_SYS_BOOTM_LEN)
Does this handle address space wrap around ? This:
=====blob=====|..............|=====blob===== .....|==spl==|..............................
Hint: it does not.
+CC Rasmus.
Look at all the other checks in U-Boot which do, and first factor them out into one check functions, so we can improve on that one. Second, use that function all over the place. Third, improve on it.
On 7/22/23 21:09, Marek Vasut wrote:
On 7/22/23 20:46, Heinrich Schuchardt wrote:
The check for an overlap of the loaded image and SPL is overly complicated.
Fixes: 77aed22b48ab ("spl: spl_legacy: Add extra address checks") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
common/spl/spl_legacy.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 095443c63d..9246f555e3 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -22,10 +22,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size; - if ((start >= spl_start && start < spl_end) || - (end > spl_start && end <= spl_end) || - (start < spl_start && end >= spl_end) || - (start > end && end > spl_start)) + if (end > spl_start && start < spl_end) panic("SPL: Image overlaps SPL\n"); if (size > CONFIG_SYS_BOOTM_LEN)
Does this handle address space wrap around ? This:
=====blob=====|..............|=====blob===== .....|==spl==|..............................
Hint: it does not.
The replaced code does not handle wrap around where end is below spl_start. The following code would cover all cases of image wrap around:
+ if (end > spl_start && start < spl_end + panic("SPL: Image overlaps SPL\n"); + if (start >= end) + panic("SPL: Image wraps around\n");
I guess we don't have to test for spl_start >= spl_end.
+CC Rasmus.
Look at all the other checks in U-Boot which do, and first factor them
Which other instances do you refer to?
The lmb library does not to check for wrap around but the wrap around checks would have to be in other places than lmb_addrs_overlap().
Best regards
Heinrich
out into one check functions, so we can improve on that one. Second, use that function all over the place. Third, improve on it.
On 7/22/23 22:37, Heinrich Schuchardt wrote:
Hi,
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 095443c63d..9246f555e3 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -22,10 +22,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size; - if ((start >= spl_start && start < spl_end) || - (end > spl_start && end <= spl_end) || - (start < spl_start && end >= spl_end) || - (start > end && end > spl_start)) + if (end > spl_start && start < spl_end) panic("SPL: Image overlaps SPL\n"); if (size > CONFIG_SYS_BOOTM_LEN)
Does this handle address space wrap around ? This:
=====blob=====|..............|=====blob===== .....|==spl==|..............................
Hint: it does not.
The replaced code does not handle wrap around where end is below spl_start. The following code would cover all cases of image wrap around:
+ if (end > spl_start && start < spl_end + panic("SPL: Image overlaps SPL\n"); + if (start >= end) + panic("SPL: Image wraps around\n");
I guess we don't have to test for spl_start >= spl_end.
I would really like someone to double-check this piece of code.
+CC Rasmus.
Look at all the other checks in U-Boot which do, and first factor them
Which other instances do you refer to?
See e.g. bootm_find_images() in boot/bootm.c . Basically git grep -i overlap and it shows a few more sites.
The lmb library does not to check for wrap around but the wrap around checks would have to be in other places than lmb_addrs_overlap().
I am kind-of tempted to suggest we should add LMB to SPL to fix this properly, since that way LMB would cover all vital parts of U-Boot and assure they won't be overwritten, not just code.
participants (2)
-
Heinrich Schuchardt -
Marek Vasut