[PATCH v4] cmd: sf: prevent overwriting the reserved memory

[Vaishnav Achath]

Hi Prasad,

On 13/09/24 13:02, Prasad Kummari wrote:
> Added LMB API to prevent SF command from overwriting reserved
> memory areas. The current SPI code does not use LMB APIs for
> loading data into memory addresses. To resolve this, LMB APIs
> were added to check the load address of an SF command and ensure it
> does not overwrite reserved memory addresses. Similar checks are
> used in TFTP, serial load, and boot code to prevent overwriting
> reserved memory.
> 
> Signed-off-by: Prasad Kummari <prasad.kummari at amd.com>
> ---
> Changes in V4:
> - Removed do_spi_read_lmb_check().
> - Added the lmb_read_check() function in lmb.c, making it reusable for
>    NAND, MMC, etc.
> - Addressed review comments.
> 
> Changes in V3:
> - Removed lmb_init_and_reserve() as part of latest LMB series.
> - Error message moved to one place.
> - lmb_alloc_addr() is not required because the given memory address is
>    being checked to ensure it is free or not.
> 
> Changes in V2:
> - Rebased the code changes on top of the next branch.
> 
> UT:
> Tested on Versal NET board.
> 
> relocaddr   = 0x000000007febc000
> 
> Versal NET> sf read 0x000000007febc000 0x0 0x40
> device 0 offset 0x0, size 0x40
> ERROR: trying to overwrite reserved memory...
> Versal NET> sf write 0x000000007febc000 0x0 0x40
> device 0 offset 0x0, size 0x40
> ERROR: trying to overwrite reserved memory...
> 
> Versal NET> fdt print /reserved-memory
> reserved-memory {
> 	ranges;
> 	#size-cells = <0x00000002>;
> 	#address-cells = <0x00000002>;
> 	tf-a {
> 		reg = <0x00000000 0x70000000 0x00000000 0x00050000>;
> 		no-map;
> 	};
> };
> Versal NET> sf read 0x70000000 0x0 0x40
> device 0 offset 0x0, size 0x40
> ERROR: trying to overwrite reserved memory...
> Versal NET> sf write 0x70000000 0x0 0x40
> device 0 offset 0x0, size 0x40
> ERROR: trying to overwrite reserved memory...
> Versal NET> sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;sf
> write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;sf read
> 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x01000000
> SF: 16777216 bytes @ 0x0 Erased: OK
> device 0 offset 0x0, size 0x1000000
> SF: 16777216 bytes @ 0x0 Written: OK
> device 0 offset 0x0, size 0x1000000
> SF: 16777216 bytes @ 0x0 Read: OK
> Total of 16777216 byte(s) were the same
> 
>   cmd/sf.c      | 8 ++++++++
>   include/lmb.h | 5 +++++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/cmd/sf.c b/cmd/sf.c
> index f43a2e08b3..08e364e191 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -10,6 +10,7 @@
>   #include <div64.h>
>   #include <dm.h>
>   #include <log.h>
> +#include <lmb.h>
>   #include <malloc.h>
>   #include <mapmem.h>
>   #include <spi.h>
> @@ -317,6 +318,13 @@ static int do_spi_flash_read_write(int argc, char *const argv[])
>   			strncmp(argv[0], "write", 5) == 0) {
>   		int read;
>   
> +		if (CONFIG_IS_ENABLED(LMB)) {
> +			if (lmb_read_check(addr, len)) {

Even though the function is named lmb_read_check(), it performs an alloc 
which is never freed, thus it makes it difficult for other callers to 
use the same region for other purposes (some callers use 
lmb_get_free_size() ), as mentioned in the commit message the check is 
only to prevent sf from overwriting reserved region, but it looks like 
this patch makes the load region also as reserved, is this necessary?


> +				printf("ERROR: trying to overwrite reserved memory...\n");
> +				return CMD_RET_FAILURE;
> +			}
> +		}
> +
>   		read = strncmp(argv[0], "read", 4) == 0;
>   		if (read)
>   			ret = spi_flash_read(flash, offset, len, buf);
> diff --git a/include/lmb.h b/include/lmb.h
> index fc2daaa7bf..aee2f9fcda 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -111,6 +111,11 @@ struct lmb *lmb_get(void);
>   int lmb_push(struct lmb *store);
>   void lmb_pop(struct lmb *store);
>   
> +static inline int lmb_read_check(phys_addr_t addr, phys_size_t len)
> +{
> +	return lmb_alloc_addr(addr, len) == addr ? 0 : -1;

who frees this? can we free this right after checking?

Thanks and Regards,
Vaishnav

> +}
> +
>   #endif /* __KERNEL__ */
>   
>   #endif /* _LINUX_LMB_H */