On Wed, Jun 12, 2024 at 04:57:08PM +0100, Mate Kukri wrote: > For NX, we need to set the page access permission attributes for write > and execute permissions. > > This patch adds two new primitives, grub_set_mem_attrs() and > grub_clear_mem_attrs(), and associated constant definitions, to be used > for that purpose. > > For most platforms, it adds a dummy implementation that returns > GRUB_ERR_NONE. > > On EFI platforms, it implements the primitives using the EFI > Memory Attribute Protocol (defined in UEFI 2.10 specification). > > Original-Author: Peter Jones <pjo...@redhat.com>
Why not Signed-off-by instead? > Signed-off-by: Mate Kukri <mate.ku...@canonical.com> > --- > grub-core/kern/efi/mm.c | 127 ++++++++++++++++++++++++++++++++++++++++ > include/grub/efi/api.h | 25 ++++++++ > include/grub/mm.h | 33 +++++++++++ > 3 files changed, 185 insertions(+) > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > index 6a6fba891..9af851e8f 100644 > --- a/grub-core/kern/efi/mm.c > +++ b/grub-core/kern/efi/mm.c > @@ -687,3 +687,130 @@ grub_efi_get_ram_base(grub_addr_t *base_addr) > return GRUB_ERR_NONE; > } > #endif > + > +static inline grub_uint64_t You can drop inline from here. > +grub_mem_attrs_to_uefi_mem_attrs (grub_uint64_t attrs) > +{ > + grub_uint64_t ret = GRUB_EFI_MEMORY_RP | > + GRUB_EFI_MEMORY_RO | > + GRUB_EFI_MEMORY_XP; I would put this into one line. > + if (attrs & GRUB_MEM_ATTR_R) > + ret &= ~GRUB_EFI_MEMORY_RP; > + > + if (attrs & GRUB_MEM_ATTR_W) > + ret &= ~GRUB_EFI_MEMORY_RO; > + > + if (attrs & GRUB_MEM_ATTR_X) > + ret &= ~GRUB_EFI_MEMORY_XP; All GRUB_EFI_MEMORY_* constants are not defined in this patch. I think they should be... > + > + return ret; > +} > + > +static inline grub_uint64_t You can drop inline from here. > +uefi_mem_attrs_to_grub_mem_attrs (grub_uint64_t attrs) > +{ > + grub_uint64_t ret = GRUB_MEM_ATTR_R | > + GRUB_MEM_ATTR_W | > + GRUB_MEM_ATTR_X; Again, you do need to wrap this line... > + if (attrs & GRUB_EFI_MEMORY_RP) > + ret &= ~GRUB_MEM_ATTR_R; > + > + if (attrs & GRUB_EFI_MEMORY_RO) > + ret &= ~GRUB_MEM_ATTR_W; > + > + if (attrs & GRUB_EFI_MEMORY_XP) > + ret &= ~GRUB_MEM_ATTR_X; > + > + return ret; > +} > + > +grub_err_t > +grub_get_mem_attrs (grub_addr_t addr, grub_size_t size, grub_uint64_t *attrs) > +{ > + grub_efi_memory_attribute_protocol_t *proto; > + grub_efi_physical_address_t physaddr = addr; > + grub_guid_t protocol_guid = GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID; Please add "static" before grub_guid_t initializations. > + grub_efi_status_t efi_status; > + > + if (physaddr & 0xfff || size & 0xfff || size == 0 || attrs == NULL) Should not 0xfff be defined as a mask (constant)? > + { > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > + N_("grub_get_mem_attrs() called with invalid > arguments")); > + } Please drop redundant curly braces. > + > + proto = grub_efi_locate_protocol (&protocol_guid, 0); > + if (!proto) > + { > + /* No protocol -> do nothing, all memory is RWX in boot services */ > + *attrs = GRUB_MEM_ATTR_R | GRUB_MEM_ATTR_W | GRUB_MEM_ATTR_X; > + return GRUB_ERR_NONE; > + } > + > + efi_status = proto->get_memory_attributes(proto, physaddr, size, attrs); > + if (efi_status != GRUB_EFI_SUCCESS) > + { > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > + N_("grub_get_mem_attrs() called with invalid > arguments")); > + } Ditto and in the other places too... > + > + *attrs = uefi_mem_attrs_to_grub_mem_attrs (*attrs); > + > + grub_dprintf ("nx", "get 0x%"PRIxGRUB_ADDR"-0x%"PRIxGRUB_ADDR":%c%c%c\n", Missing spaces before and after PRIxGRUB_ADDR. > + addr, addr + size - 1, > + (*attrs & GRUB_MEM_ATTR_R) ? 'r' : '-', > + (*attrs & GRUB_MEM_ATTR_W) ? 'w' : '-', > + (*attrs & GRUB_MEM_ATTR_X) ? 'x' : '-'); > + > + return GRUB_ERR_NONE; > +} > + > +grub_err_t > +grub_update_mem_attrs (grub_addr_t addr, grub_size_t size, > + grub_uint64_t set_attrs, grub_uint64_t clear_attrs) > +{ > + grub_efi_memory_attribute_protocol_t *proto; > + grub_efi_physical_address_t physaddr = addr; > + grub_guid_t protocol_guid = GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID; Again, please add "static" to grub_guid_t initializations. > + grub_efi_status_t efi_status = GRUB_EFI_SUCCESS; > + grub_uint64_t uefi_set_attrs, uefi_clear_attrs; s/grub_uint64_t/grub_efi_uint64_t/ > + > + Please drop redundant empty line. > + if (physaddr & 0xfff || size & 0xfff || size == 0) Certainly 0xfff deserves being a constant... > + { > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > + N_("grub_update_mem_attrs() called with invalid > arguments")); > + } > + > + proto = grub_efi_locate_protocol (&protocol_guid, 0); > + if (!proto) Please use "if (proto == NULL)" in all cases like that one. > + { > + /* No protocol -> do nothing, all memory is RWX in boot services */ > + return GRUB_ERR_NONE; > + } Again, redundant curly braces. Please fix this everywhere. > + uefi_set_attrs = grub_mem_attrs_to_uefi_mem_attrs (set_attrs); > + uefi_clear_attrs = grub_mem_attrs_to_uefi_mem_attrs (clear_attrs); > + if (uefi_set_attrs) > + efi_status = proto->set_memory_attributes(proto, physaddr, size, > uefi_set_attrs); > + if (efi_status == GRUB_EFI_SUCCESS && uefi_clear_attrs) > + efi_status = proto->clear_memory_attributes(proto, physaddr, size, > uefi_clear_attrs); > + > + if (efi_status != GRUB_EFI_SUCCESS) > + { > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > + N_("grub_update_mem_attrs() called with invalid > arguments")); > + } > + > + grub_dprintf ("nx", "set +%s%s%s -%s%s%s on > 0x%"PRIxGRUB_ADDR"-0x%"PRIxGRUB_ADDR"\n", > + (set_attrs & GRUB_MEM_ATTR_R) ? "r" : "", > + (set_attrs & GRUB_MEM_ATTR_W) ? "w" : "", > + (set_attrs & GRUB_MEM_ATTR_X) ? "x" : "", > + (clear_attrs & GRUB_MEM_ATTR_R) ? "r" : "", > + (clear_attrs & GRUB_MEM_ATTR_W) ? "w" : "", > + (clear_attrs & GRUB_MEM_ATTR_X) ? "x" : "", > + addr, addr + size - 1); > + > + return GRUB_ERR_NONE; > +} > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h > index d44d00ad7..b686e8afe 100644 > --- a/include/grub/efi/api.h > +++ b/include/grub/efi/api.h > @@ -379,6 +379,11 @@ > {0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f } \ > } > > +#define GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID \ > + { 0xf4560cf6, 0x40ec, 0x4b4a, \ > + { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 } \ > + } > + > struct grub_efi_sal_system_table > { > grub_uint32_t signature; > @@ -1815,4 +1820,24 @@ struct initrd_media_device_path { > } GRUB_PACKED; > typedef struct initrd_media_device_path initrd_media_device_path_t; > > +struct grub_efi_memory_attribute_protocol > +{ > + grub_efi_status_t (__grub_efi_api *get_memory_attributes) ( > + struct grub_efi_memory_attribute_protocol *this, > + grub_efi_physical_address_t base_address, > + grub_efi_uint64_t length, > + grub_efi_uint64_t *attributes); > + grub_efi_status_t (__grub_efi_api *set_memory_attributes) ( > + struct grub_efi_memory_attribute_protocol *this, > + grub_efi_physical_address_t base_address, > + grub_efi_uint64_t length, > + grub_efi_uint64_t attributes); > + grub_efi_status_t (__grub_efi_api *clear_memory_attributes) ( > + struct grub_efi_memory_attribute_protocol *this, > + grub_efi_physical_address_t base_address, > + grub_efi_uint64_t length, > + grub_efi_uint64_t attributes); > +}; > +typedef struct grub_efi_memory_attribute_protocol > grub_efi_memory_attribute_protocol_t; > + > #endif /* ! GRUB_EFI_API_HEADER */ > diff --git a/include/grub/mm.h b/include/grub/mm.h > index f3bf87fa0..dcccfe395 100644 > --- a/include/grub/mm.h > +++ b/include/grub/mm.h > @@ -23,6 +23,7 @@ > #include <grub/err.h> > #include <grub/types.h> > #include <grub/symbol.h> > +#include <grub/err.h> > #include <config.h> > > #ifndef NULL > @@ -56,6 +57,38 @@ void *EXPORT_FUNC(grub_realloc) (void *ptr, grub_size_t > size); > void *EXPORT_FUNC(grub_memalign) (grub_size_t align, grub_size_t size); > #endif > > +#define GRUB_MEM_ATTR_R 0x0000000000000004LLU > +#define GRUB_MEM_ATTR_W 0x0000000000000002LLU > +#define GRUB_MEM_ATTR_X 0x0000000000000001LLU I think we should define a mem attr type and cast to it here. The LLU may not always be grub_uint64_t as you expect. > + > +#ifdef GRUB_MACHINE_EFI > +grub_err_t EXPORT_FUNC(grub_get_mem_attrs) (grub_addr_t addr, > + grub_size_t size, > + grub_uint64_t *attrs); > +grub_err_t EXPORT_FUNC(grub_update_mem_attrs) (grub_addr_t addr, > + grub_size_t size, > + grub_uint64_t set_attrs, > + grub_uint64_t clear_attrs); > +#else /* !GRUB_MACHINE_EFI */ > +static inline grub_err_t > +grub_get_mem_attrs (grub_addr_t addr __attribute__((__unused__)), > + grub_size_t size __attribute__((__unused__)), > + grub_uint64_t *attrs __attribute__((__unused__))) You mark attrs as unused and then use it below... > +{ > + *attrs = GRUB_MEM_ATTR_R | GRUB_MEM_ATTR_W | GRUB_MEM_ATTR_X; > + return GRUB_ERR_NONE; > +} > + > +static inline grub_err_t > +grub_update_mem_attrs (grub_addr_t addr __attribute__((__unused__)), > + grub_size_t size __attribute__((__unused__)), > + grub_uint64_t set_attrs __attribute__((__unused__)), > + grub_uint64_t clear_attrs __attribute__((__unused__))) ... but here it is correct... > +{ > + return GRUB_ERR_NONE; > +} Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel