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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ