linux-hardening - Re: [PATCH] efi: Replace efi_memory_attributes_table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMj1kXEyDjH0uu3Z4eBesV3PEnKGi5ArXXMp7R-hn8HdRytiPg@mail.gmail.com>
Date: Thu, 11 Jul 2024 08:05:43 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc: Kees Cook <kees@...nel.org>, linux-efi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array
 with flexible array

On Thu, 11 Jul 2024 at 01:34, Gustavo A. R. Silva
<gustavo@...eddedor.com> wrote:
>
>
>
> On 10/07/24 17:32, Gustavo A. R. Silva wrote:
> >
> >
> > On 10/07/24 16:55, Kees Cook wrote:
> >> While efi_memory_attributes_table_t::entry isn't used directly as an
> >> array, it is used as a base for pointer arithmetic. The type is wrong
> >> as it's not technically an array of efi_memory_desc_t's; they could be
> >> larger. Regardless, leave the type unchanged and remove the old style
> >> "0" array size. Additionally replace the open-coded entry offset code
> >> with the existing efi_early_memdesc_ptr() helper.
> >>
> >> Signed-off-by: Kees Cook <kees@...nel.org>
> >> ---
> >> Cc: Ard Biesheuvel <ardb@...nel.org>
> >> Cc: linux-efi@...r.kernel.org
> >> ---
> >>   drivers/firmware/efi/memattr.c | 2 +-
> >>   include/linux/efi.h            | 6 +++++-
> >>   2 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> >> index ab85bf8e165a..01142604e8df 100644
> >> --- a/drivers/firmware/efi/memattr.c
> >> +++ b/drivers/firmware/efi/memattr.c
> >> @@ -164,7 +164,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
> >>           bool valid;
> >>           char buf[64];
> >> -        valid = entry_is_valid((void *)tbl->entry + i * tbl->desc_size,
> >> +        valid = entry_is_valid(efi_early_memdesc_ptr(tbl->entry, tbl->desc_size, i),
> >>                          &md);
> >>           size = md.num_pages << EFI_PAGE_SHIFT;
> >>           if (efi_enabled(EFI_DBG) || !valid)
> >> diff --git a/include/linux/efi.h b/include/linux/efi.h
> >> index 418e555459da..b06639c4f6a5 100644
> >> --- a/include/linux/efi.h
> >> +++ b/include/linux/efi.h
> >> @@ -607,7 +607,11 @@ typedef struct {
> >>       u32 num_entries;
> >>       u32 desc_size;
> >>       u32 flags;
> >> -    efi_memory_desc_t entry[0];
> >> +    /*
> >> +     * There are @num_entries following, each of size @desc_size bytes,
> >> +     * including an efi_memory_desc_t header.
> >> +     */
> >> +    efi_memory_desc_t entry[];
> >
> > a candidate for future __counted_by(num_entries * desc_size) ? :p
>
> ah no, this rather be something more like __sized_by(num_entries * desc_size).
>
> --
> Gustavo
>
> >
> > Reviewed-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> >

Thanks. I'll take this for the next cycle, but would you mind adding a
preceding patch that drops the 'early' from the macro name and updates
the existing users (not the ones in arch/x88/boot, which has its own
private copy for some reason). The 'early' is kind of irrelevant, and
no longer accurate now that we use the macro for the memory attributes
table as well as the memory map.