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

[Michal Simek]

On 9/11/24 13:20, Sughosh Ganu wrote:
> On Tue, 10 Sept 2024 at 21:14, Prasad Kummari <prasad.kummari at amd.com> 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 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
>>
>> 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...
>>
>> 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...
>>
>>   cmd/sf.c | 36 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/sf.c b/cmd/sf.c
>> index f43a2e08b3..7bb8bcfce2 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>
>> @@ -272,6 +273,31 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
>>          return 0;
>>   }
>>
>> +#ifdef CONFIG_LMB
>> +static int do_spi_read_lmb_check(ulong start_addr, loff_t len)
>> +{
>> +       phys_size_t max_size;
>> +       ulong end_addr;
>> +
>> +       lmb_dump_all();
>> +
>> +       max_size = lmb_get_free_size(start_addr);
>> +       if (!max_size) {
>> +               return CMD_RET_FAILURE;
>> +       }
>> +
>> +       end_addr = start_addr + max_size;
>> +       if (!end_addr)
>> +               end_addr = ULONG_MAX;
>> +
>> +       if ((start_addr + len) > end_addr) {
>> +               return CMD_RET_FAILURE;
>> +       }
> 
> This is one way to get the load address, yes. But please do note that
> with this method, if the region of memory has already been marked as
> reserved, the call to lmb_get_free_size() will fail, i.e. return a
> value of 0. Whereas if you call lmb_alloc_addr(), and if the memory
> region is marked reserved with the flags set to LMB_NONE, that call
> will not fail, as we can re-reserve memory marked as LMB_NONE. Hence a
> better approach would be to use the logic used in the fs module
> function that I had pointed to in my earlier email.
> 
> Also, like I mentioned earlier, it will be better to refactor the
> functionality, especially if the plan is to introduce this check for
> loading stuff through other interfaces like nand, mmc etc. You can add
> a function in the lmb.c like lmb_read_check() which does that.

Is this something what you are going to work on?

Thanks,
Michal