[PATCH 0/2] lmb: rework logic to validate load address for network commands

Rework the logic to verify the load address so that address re-use is not an issue.
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
Sughosh Ganu (2): tftp: rework the logic to validate the load address wget: rework the logic to validate the load address
net/tftp.c | 19 +------------------ net/wget.c | 36 +----------------------------------- 2 files changed, 2 insertions(+), 53 deletions(-)

Use the lmb_read_check() function to verify if it is safe to use a region of memory to load data from a tftp command. The current logic checks the amount of free memory available, starting from the 'load address'. This call fails if the same region of memory has been used earlier. This used to work earlier as the LMB memory map had a local scope and was not persistent. Fix this issue by using the lmb_read_check() call instead which only returns an error in case the memory region has been marked for not allowing re-use.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org ---
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
net/tftp.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index b5d227d8bc..d6744bc24e 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -82,7 +82,6 @@ static ulong tftp_block_wrap; static ulong tftp_block_wrap_offset; static int tftp_state; static ulong tftp_load_addr; -static ulong tftp_load_size; #ifdef CONFIG_TFTP_TSIZE /* The file size reported by the server */ static int tftp_tsize; @@ -159,13 +158,8 @@ static inline int store_block(int block, uchar *src, unsigned int len) void *ptr;
if (CONFIG_IS_ENABLED(LMB)) { - ulong end_addr = tftp_load_addr + tftp_load_size; - - if (!end_addr) - end_addr = ULONG_MAX; - if (store_addr < tftp_load_addr || - store_addr + len > end_addr) { + lmb_read_check(store_addr, len)) { puts("\nTFTP error: "); puts("trying to overwrite reserved memory...\n"); return -1; @@ -712,19 +706,8 @@ static void tftp_timeout_handler(void) } }
-/* Initialize tftp_load_addr and tftp_load_size from image_load_addr and lmb */ static int tftp_init_load_addr(void) { - if (CONFIG_IS_ENABLED(LMB)) { - phys_size_t max_size; - - max_size = lmb_get_free_size(image_load_addr); - if (!max_size) - return -1; - - tftp_load_size = max_size; - } - tftp_load_addr = image_load_addr; return 0; }

On 16/09/24 20:50, Sughosh Ganu wrote:
Use the lmb_read_check() function to verify if it is safe to use a region of memory to load data from a tftp command. The current logic checks the amount of free memory available, starting from the 'load address'. This call fails if the same region of memory has been used earlier. This used to work earlier as the LMB memory map had a local scope and was not persistent. Fix this issue by using the lmb_read_check() call instead which only returns an error in case the memory region has been marked for not allowing re-use.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
Tested-by: Vaishnav Achath vaishnav.a@ti.com
net/tftp.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c index b5d227d8bc..d6744bc24e 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -82,7 +82,6 @@ static ulong tftp_block_wrap; static ulong tftp_block_wrap_offset; static int tftp_state; static ulong tftp_load_addr; -static ulong tftp_load_size; #ifdef CONFIG_TFTP_TSIZE /* The file size reported by the server */ static int tftp_tsize; @@ -159,13 +158,8 @@ static inline int store_block(int block, uchar *src, unsigned int len) void *ptr;
if (CONFIG_IS_ENABLED(LMB)) {
ulong end_addr = tftp_load_addr + tftp_load_size;
if (!end_addr)
end_addr = ULONG_MAX;
- if (store_addr < tftp_load_addr ||
store_addr + len > end_addr) {
lmb_read_check(store_addr, len)) { puts("\nTFTP error: "); puts("trying to overwrite reserved memory...\n"); return -1;
@@ -712,19 +706,8 @@ static void tftp_timeout_handler(void) } }
-/* Initialize tftp_load_addr and tftp_load_size from image_load_addr and lmb */ static int tftp_init_load_addr(void) {
- if (CONFIG_IS_ENABLED(LMB)) {
phys_size_t max_size;
max_size = lmb_get_free_size(image_load_addr);
if (!max_size)
return -1;
tftp_load_size = max_size;
- }
- tftp_load_addr = image_load_addr; return 0; }

Use the lmb_read_check() function to verify if it is safe to use a region of memory to load data from the wget command. The current logic checks the amount of free memory available, starting from the 'load address'. This call fails if the same region of memory has been used earlier. This used to work earlier as the LMB memory map had a local scope and was not persistent. Fix this issue by using the lmb_read_check() call instead which only returns an error in case the memory region has been marked for not allowing re-use.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org ---
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
net/wget.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-)
diff --git a/net/wget.c b/net/wget.c index 4a168641c6..a88c31f236 100644 --- a/net/wget.c +++ b/net/wget.c @@ -64,26 +64,6 @@ static unsigned int retry_tcp_ack_num; /* TCP retry acknowledge number*/ static unsigned int retry_tcp_seq_num; /* TCP retry sequence number */ static int retry_len; /* TCP retry length */
-static ulong wget_load_size; - -/** - * wget_init_max_size() - initialize maximum load size - * - * Return: 0 if success, -1 if fails - */ -static int wget_init_load_size(void) -{ - phys_size_t max_size; - - max_size = lmb_get_free_size(image_load_addr); - if (!max_size) - return -1; - - wget_load_size = max_size; - - return 0; -} - /** * store_block() - store block in memory * @src: source of data @@ -97,13 +77,8 @@ static inline int store_block(uchar *src, unsigned int offset, unsigned int len) uchar *ptr;
if (CONFIG_IS_ENABLED(LMB)) { - ulong end_addr = image_load_addr + wget_load_size; - - if (!end_addr) - end_addr = ULONG_MAX; - if (store_addr < image_load_addr || - store_addr + len > end_addr) { + lmb_read_check(store_addr, len)) { printf("\nwget error: "); printf("trying to overwrite reserved memory...\n"); return -1; @@ -493,15 +468,6 @@ void wget_start(void) debug_cond(DEBUG_WGET, "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
- if (CONFIG_IS_ENABLED(LMB)) { - if (wget_init_load_size()) { - printf("\nwget error: "); - printf("trying to overwrite reserved memory...\n"); - net_set_state(NETLOOP_FAIL); - return; - } - } - net_set_timeout_handler(wget_timeout, wget_timeout_handler); tcp_set_tcp_handler(wget_handler);

On 16/09/24 20:50, Sughosh Ganu wrote:
Use the lmb_read_check() function to verify if it is safe to use a region of memory to load data from the wget command. The current logic checks the amount of free memory available, starting from the 'load address'. This call fails if the same region of memory has been used earlier. This used to work earlier as the LMB memory map had a local scope and was not persistent. Fix this issue by using the lmb_read_check() call instead which only returns an error in case the memory region has been marked for not allowing re-use.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
Tested-by: Vaishnav Achath vaishnav.a@ti.com
net/wget.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-)
diff --git a/net/wget.c b/net/wget.c index 4a168641c6..a88c31f236 100644 --- a/net/wget.c +++ b/net/wget.c @@ -64,26 +64,6 @@ static unsigned int retry_tcp_ack_num; /* TCP retry acknowledge number*/ static unsigned int retry_tcp_seq_num; /* TCP retry sequence number */ static int retry_len; /* TCP retry length */
-static ulong wget_load_size;
-/**
- wget_init_max_size() - initialize maximum load size
- Return: 0 if success, -1 if fails
- */
-static int wget_init_load_size(void) -{
- phys_size_t max_size;
- max_size = lmb_get_free_size(image_load_addr);
- if (!max_size)
return -1;
- wget_load_size = max_size;
- return 0;
-}
- /**
- store_block() - store block in memory
- @src: source of data
@@ -97,13 +77,8 @@ static inline int store_block(uchar *src, unsigned int offset, unsigned int len) uchar *ptr;
if (CONFIG_IS_ENABLED(LMB)) {
ulong end_addr = image_load_addr + wget_load_size;
if (!end_addr)
end_addr = ULONG_MAX;
- if (store_addr < image_load_addr ||
store_addr + len > end_addr) {
lmb_read_check(store_addr, len)) { printf("\nwget error: "); printf("trying to overwrite reserved memory...\n"); return -1;
@@ -493,15 +468,6 @@ void wget_start(void) debug_cond(DEBUG_WGET, "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
- if (CONFIG_IS_ENABLED(LMB)) {
if (wget_init_load_size()) {
printf("\nwget error: ");
printf("trying to overwrite reserved memory...\n");
net_set_state(NETLOOP_FAIL);
return;
}
- }
- net_set_timeout_handler(wget_timeout, wget_timeout_handler); tcp_set_tcp_handler(wget_handler);

On 16/09/2024 17:20, Sughosh Ganu wrote:
Use the lmb_read_check() function to verify if it is safe to use a region of memory to load data from the wget command. The current logic checks the amount of free memory available, starting from the 'load address'. This call fails if the same region of memory has been used earlier. This used to work earlier as the LMB memory map had a local scope and was not persistent. Fix this issue by using the lmb_read_check() call instead which only returns an error in case the memory region has been marked for not allowing re-use.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
This fixes Qualcomm platforms which use LMB to allocate $loaddaddr and friends.
Reviewed-by: Caleb Connolly caleb.connolly@linaro.org
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
net/wget.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-)
diff --git a/net/wget.c b/net/wget.c index 4a168641c6..a88c31f236 100644 --- a/net/wget.c +++ b/net/wget.c @@ -64,26 +64,6 @@ static unsigned int retry_tcp_ack_num; /* TCP retry acknowledge number*/ static unsigned int retry_tcp_seq_num; /* TCP retry sequence number */ static int retry_len; /* TCP retry length */
-static ulong wget_load_size;
-/**
- wget_init_max_size() - initialize maximum load size
- Return: 0 if success, -1 if fails
- */
-static int wget_init_load_size(void) -{
- phys_size_t max_size;
- max_size = lmb_get_free_size(image_load_addr);
- if (!max_size)
return -1;
- wget_load_size = max_size;
- return 0;
-}
- /**
- store_block() - store block in memory
- @src: source of data
@@ -97,13 +77,8 @@ static inline int store_block(uchar *src, unsigned int offset, unsigned int len) uchar *ptr;
if (CONFIG_IS_ENABLED(LMB)) {
ulong end_addr = image_load_addr + wget_load_size;
if (!end_addr)
end_addr = ULONG_MAX;
- if (store_addr < image_load_addr ||
store_addr + len > end_addr) {
lmb_read_check(store_addr, len)) { printf("\nwget error: "); printf("trying to overwrite reserved memory...\n"); return -1;
@@ -493,15 +468,6 @@ void wget_start(void) debug_cond(DEBUG_WGET, "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
- if (CONFIG_IS_ENABLED(LMB)) {
if (wget_init_load_size()) {
printf("\nwget error: ");
printf("trying to overwrite reserved memory...\n");
net_set_state(NETLOOP_FAIL);
return;
}
- }
- net_set_timeout_handler(wget_timeout, wget_timeout_handler); tcp_set_tcp_handler(wget_handler);

On Mon, 16 Sep 2024 20:50:23 +0530, Sughosh Ganu wrote:
Rework the logic to verify the load address so that address re-use is not an issue.
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
[...]
Applied to u-boot/next, thanks!

Hi Tom,
On 21/09/24 08:09, Tom Rini wrote:
On Mon, 16 Sep 2024 20:50:23 +0530, Sughosh Ganu wrote:
Rework the logic to verify the load address so that address re-use is not an issue.
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
[...]
Applied to u-boot/next, thanks!
Can these fixes be applied to master also as the failures are seen now in master as well?
Thanks and Regards, Vaishnav

On Tue, 1 Oct 2024 at 11:37, Vaishnav Achath vaishnav.a@ti.com wrote:
Hi Tom,
On 21/09/24 08:09, Tom Rini wrote:
On Mon, 16 Sep 2024 20:50:23 +0530, Sughosh Ganu wrote:
Rework the logic to verify the load address so that address re-use is not an issue.
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
[...]
Applied to u-boot/next, thanks!
Can these fixes be applied to master also as the failures are seen now in master as well?
The lmb patch series has not made it to master yet. How are you seeing these issues on the master branch when the lmb scope is local ?
-sughosh
Thanks and Regards, Vaishnav

Hi Sughosh, Tom,
On 01/10/24 14:19, Sughosh Ganu wrote:
On Tue, 1 Oct 2024 at 11:37, Vaishnav Achath vaishnav.a@ti.com wrote:
Hi Tom,
On 21/09/24 08:09, Tom Rini wrote:
On Mon, 16 Sep 2024 20:50:23 +0530, Sughosh Ganu wrote:
Rework the logic to verify the load address so that address re-use is not an issue.
Note: To be applied on next, on top of https://patchwork.ozlabs.org/project/uboot/patch/20240913073251.2286529-2-pr...
[...]
Applied to u-boot/next, thanks!
Can these fixes be applied to master also as the failures are seen now in master as well?
The lmb patch series has not made it to master yet. How are you seeing these issues on the master branch when the lmb scope is local ?
-sughosh
Sorry this was a confusion from my end, please ignore my previous comment, master does not have any issue related to this.
Thanks and Regards, Vaishnav
Thanks and Regards, Vaishnav
participants (4)
-
Caleb Connolly
-
Sughosh Ganu
-
Tom Rini
-
Vaishnav Achath