* [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array
@ 2024-07-10 22:55 Kees Cook
2024-07-10 23:32 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2024-07-10 22:55 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Kees Cook, linux-efi, linux-kernel, linux-hardening
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[];
} efi_memory_attributes_table_t;
typedef struct {
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array
2024-07-10 22:55 [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array Kees Cook
@ 2024-07-10 23:32 ` Gustavo A. R. Silva
2024-07-10 23:34 ` Gustavo A. R. Silva
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2024-07-10 23:32 UTC (permalink / raw)
To: Kees Cook, Ard Biesheuvel; +Cc: linux-efi, linux-kernel, linux-hardening
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
--
Gustavo
> } efi_memory_attributes_table_t;
>
> typedef struct {
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array
2024-07-10 23:32 ` Gustavo A. R. Silva
@ 2024-07-10 23:34 ` Gustavo A. R. Silva
2024-07-11 6:05 ` Ard Biesheuvel
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo A. R. Silva @ 2024-07-10 23:34 UTC (permalink / raw)
To: Kees Cook, Ard Biesheuvel; +Cc: linux-efi, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array
2024-07-10 23:34 ` Gustavo A. R. Silva
@ 2024-07-11 6:05 ` Ard Biesheuvel
0 siblings, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2024-07-11 6:05 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Kees Cook, linux-efi, linux-kernel, linux-hardening
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-07-11 6:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-10 22:55 [PATCH] efi: Replace efi_memory_attributes_table_t 0-sized array with flexible array Kees Cook
2024-07-10 23:32 ` Gustavo A. R. Silva
2024-07-10 23:34 ` Gustavo A. R. Silva
2024-07-11 6:05 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox