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

[Michal Simek]

On 9/13/24 09:32, 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)) {
> +				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;
> +}
> +
>   #endif /* __KERNEL__ */
>   
>   #endif /* _LINUX_LMB_H */


It is coming based on discussion with Sughosh in v3 that's why

Suggested-by: Sughosh Ganu <sughosh.ganu at linaro.org>

Thanks,
Michal