Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

在 2025/10/29 22:15, Ard Biesheuvel 写道:
> On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@xxxxxxxxxxxxx> wrote:
> > 在 2025/10/28 21:42, Ard Biesheuvel 写道:
> > > On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@xxxxxxxxxxxxx> wrote:
> > > > 在 2025/10/23 16:30, Ard Biesheuvel 写道:
> > > > > On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@xxxxxxxxxxxxx> wrote:
> > > > > > In the efi_virtmap_init(), permission settings have been applied:
> > > > > >
> > > > > > static bool __init efi_virtmap_init(void)
> > > > > > {
> > > > > >            ...
> > > > > >            for_each_efi_memory_desc(md)
> > > > > >                    ...
> > > > > >                    efi_create_mapping(&efi_mm, md);
> > > > > >            ...
> > > > > >            efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
> > > > > >            ...
> > > > > > }
> > > > > >
> > > > > > Therefore, there is no need to apply it again in the efi_create_mapping().
> > > > > >
> > > > > > Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
> > > > > >
> > > > > > Signed-off-by: Qiang Ma <maqianga@xxxxxxxxxxxxx>
> > > > > No, efi_memattr_apply_permissions() uses the /optional/ memory
> > > > > attributes table, whereas efi_create_mapping() uses the permission
> > > > > attributes in the EFI memory map. The memory attributes table is
> > > > > optional, in which case any RO/XP attributes from the memory map
> > > > > should be used.
> > > > I see.
> > > >
> > > > Then, can it be modified like this?
> > > No
> > >
> > > > --- a/arch/arm/kernel/efi.c
> > > > +++ b/arch/arm/kernel/efi.c
> > > > @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
> > > > efi_memory_desc_t *md)
> > > >                    desc.type = MT_MEMORY_RWX_NONCACHED;
> > > >            else if (md->attribute & EFI_MEMORY_WC)
> > > >                    desc.type = MT_DEVICE_WC;
> > > > +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
> > > This will be true for RO, XP or RO+XP.
> > >
> > > > +               desc.type = MT_MEMORY_RO;
> > > This will apply RO permissions even to XP regions, which need to be writable.
> > Thanks for your review.
> > I see.
> >
> > I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
> > and then we can use the RO+XP attribute to implement memory mapping.
> Why? The current code is working fine, no?
Yes, the current code is running normally.

The reasons for the modification are as follows:
I noticed that the arm64/RISC-V efi_create_mapping() always return 0,
but in the code where efi_virtmap_init() calls it, it is as follows:

ret = efi_create_mapping(&efi_mm, md);
if (ret) {
    pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
        &phys, ret);
    return false;
}

This return error print is unnecessary, so I want to remove it.
But, the return value of efi_create_mapping() in the arm doesn't
always return 0, so I'm trying to modify it.