[PATCH v4] cmd: sf: prevent overwriting the reserved memory
Vaishnav Achath
vaishnav.a at ti.com
Mon Sep 16 12:37:01 CEST 2024
Hi Sughosh,
On 16/09/24 14:53, Sughosh Ganu wrote:
> On Mon, 16 Sept 2024 at 14:22, Vaishnav Achath <vaishnav.a at ti.com> wrote:
>>
>> Hi Sughosh,
>>
>> On 16/09/24 12:13, Sughosh Ganu wrote:
>>> On Mon, 16 Sept 2024 at 11:47, Vaishnav Achath <vaishnav.a at ti.com> wrote:
>>>>
>>>> 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?
>>>
>>> Like I mentioned in my other reply, using a check for lmb_alloc_addr()
>>> allows for memory re-use, which is the behaviour that a large number
>>> of use cases rely on -- if you go through the test scripts, it is
>>> assumed that memory re-use is allowed. That there is no need to
>>> explicitly free up memory, and that has been how the LMB memory has
>>> been used historically. So it is allowed to use some address to load
>>> an image to that address, and then use the same address to load a
>>> different image. The LMB rework series does keep this behaviour
>>> consistent. So it would be better to change the behaviour of the tftp
>>> command to use the same API. I was planning on working on this
>>> cleanup. If you want, you can take it up.
>>>
>>
>> Please let me know if you are planning to work on this in the coming few
>> days, otherwise I can pick it up as we have platforms failing due to this.
>
> I can take this up. Will keep you on Cc so that you can test the
> patches on your boards.
>
Thanks, I will test and report the results once you post the patches.
>>
>>>>
>>>>
>>>>> + 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?
>>>
>>> It is not required to explicitly free up this memory, as it is not an
>>> actual allocation per-se. Why were these functions called alloc
>>> something, I am not sure. But the point is, if you change the tftp
>>> command code to use this API instead, and then use it after a previous
>>> load, it would not fail.
>>>
>>> -sughosh
>>>
>>
>> Agreed, but the commit message says to "ensure it does not overwrite
>> reserved memory addresses" but the implementation marks the region as
>> reserved in the global memory map and is visible in lmb_dump_all() ,
>> which is not expected, is there really a need to mark the region as
>> reserved.
>
> What can be overwritten, and what cannot be can now be determined from
> the flags. If you check the LMB memory map from the bdinfo command,
> you will see that the regions which cannot be overwritten are now
> being marked with the "no-overwrite" flag. The other LMB memory which
> can be re-used to load multiple different images is being marked with
> the "none" flag. One issue with all this is that currently there is no
> document which explains all these concepts. I will work on adding such
> a document. Thanks.
>
Yes, documenting the behavior will clear the confusion.
Thanks and Regards,
Vaishnav
> -sughosh
>
>>
>> Thanks and Regards,
>> Vaishnav
>>
>>>>
>>>> Thanks and Regards,
>>>> Vaishnav
>>>>
>>>>> +}
>>>>> +
>>>>> #endif /* __KERNEL__ */
>>>>>
>>>>> #endif /* _LINUX_LMB_H */
More information about the U-Boot
mailing list