| Message ID | 20240710225538.work.224-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array | expand |
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@kernel.org> > --- > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: linux-efi@vger.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 Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks
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@kernel.org> >> --- >> Cc: Ard Biesheuvel <ardb@kernel.org> >> Cc: linux-efi@vger.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@kernel.org> > > Thanks
On Thu, 11 Jul 2024 at 01:34, Gustavo A. R. Silva <gustavo@embeddedor.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@kernel.org> > >> --- > >> Cc: Ard Biesheuvel <ardb@kernel.org> > >> Cc: linux-efi@vger.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@kernel.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.
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[]; } efi_memory_attributes_table_t; typedef struct {
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@kernel.org> --- Cc: Ard Biesheuvel <ardb@kernel.org> Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/memattr.c | 2 +- include/linux/efi.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)