[PATCH] env: fat: Avoid writing to read-only location

[Ilias Apalodimas]

Thanks Andre,


On Thu, 30 Jan 2025 at 15:37, Andre Przywara <andre.przywara at arm.com> wrote:
>
> The env_fat_get_dev_part() function mostly returns a fixed string, set
> via some Kconfig variable. However when the first character is a colon,
> that means that the boot device number is determined at runtime, and
> patched in. This requires altering the string.
>
> So far this was done via some ugly and actually illegal direct write to
> the .rodata string storage. We got away with this because U-Boot maps
> everything as read/write/execute so far.
>
> A proposed patch set actually enforces read-only (and no-execute)
> permissions in the page tables, so this routine now causes an exception:
> =======================
> Loading Environment from FAT... "Synchronous Abort" handler, esr 0x9600004f, far 0xfffb7d4c
> elr: 000000004a054228 lr : 000000004a05421c (reloc)
> elr: 00000000fff7c228 lr : 00000000fff7c21c
> .....
> =======================
>
> Rewrite the routine to do away with the dodgy string manipulation,
> instead allocate the string in the r/w .data section, where we can
> safely manipulate it.
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> ---
> Hi,
>
> found when naively testing Ilias' series on some Allwinner board.
> I am sure there are better ways to implement this (probably as many as
> there are developers), but this works for me.
> This does away with the optimisation of doing the manipulation only
> once, but I am not sure that's really worthwhile?
>
> Cheers,
> Andre
>
>  env/fat.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/env/fat.c b/env/fat.c
> index b04b1d9c315..1ad301eaaff 100644
> --- a/env/fat.c
> +++ b/env/fat.c
> @@ -41,14 +41,12 @@ __weak const char *env_fat_get_intf(void)
>  __weak char *env_fat_get_dev_part(void)
>  {
>  #ifdef CONFIG_MMC
> -       static char *part_str;
> -
> -       if (!part_str) {
> -               part_str = CONFIG_ENV_FAT_DEVICE_AND_PART;
> -               if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc") && part_str[0] == ':') {
> -                       part_str = "0" CONFIG_ENV_FAT_DEVICE_AND_PART;
> -                       part_str[0] += mmc_get_env_dev();
> -               }
> +       /* reserve one more char for the manipulation below */
> +       static char part_str[] = CONFIG_ENV_FAT_DEVICE_AND_PART "\0";
> +
> +       if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc") && part_str[0] == ':') {
> +               part_str[0] = '0' + mmc_get_env_dev();
> +               strcpy(&part_str[1], CONFIG_ENV_FAT_DEVICE_AND_PART);
>         }
>
>         return part_str;
> --
> 2.25.1
>
Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>