On 21 January 2016 at 17:10, Matt Fleming <[email protected]> wrote: > On Mon, 11 Jan, at 02:19:13PM, Ard Biesheuvel wrote: >> This implements efi_random_alloc(), which allocates a chunk of memory of >> a certain size at a certain alignment, and uses the random_seed argument >> it receives to randomize the offset of the allocation. > > s/offset/address/ ? > > I see what you're getting at with the word "offset" but ultimately, > this is a memory allocation function, and it returns an address. > > "offset" implies to me that the implementation allocates a larger > memory chunk than is required and returns an address that is >= the > start of the bigger-than-required-allocation. >
Well, offset is horribly overloaded in our world, so let's stick with 'address' >> This is implemented by iterating over the UEFI memory map, counting the >> number of suitable slots (aligned offsets) within each region, and picking >> a random number between 0 and 'number of slots - 1' to select the slot, >> This should guarantee that each possible offset is chosen equally likely. >> >> Suggested-by: Kees Cook <[email protected]> >> Cc: Matt Fleming <[email protected]> >> Signed-off-by: Ard Biesheuvel <[email protected]> >> --- >> drivers/firmware/efi/libstub/efistub.h | 4 + >> drivers/firmware/efi/libstub/random.c | 85 ++++++++++++++++++++ >> 2 files changed, 89 insertions(+) >> >> diff --git a/drivers/firmware/efi/libstub/efistub.h >> b/drivers/firmware/efi/libstub/efistub.h >> index 206b7252b9d1..7a38e29da53d 100644 >> --- a/drivers/firmware/efi/libstub/efistub.h >> +++ b/drivers/firmware/efi/libstub/efistub.h >> @@ -46,4 +46,8 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, >> unsigned long map_size, >> efi_status_t efi_get_random_bytes(efi_system_table_t *sys_table, >> unsigned long size, u8 *out); >> >> +efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg, >> + unsigned long size, unsigned long align_bits, >> + unsigned long *addr, unsigned long random_seed); >> + >> #endif >> diff --git a/drivers/firmware/efi/libstub/random.c >> b/drivers/firmware/efi/libstub/random.c >> index f539b1e31459..d4829824508c 100644 >> --- a/drivers/firmware/efi/libstub/random.c >> +++ b/drivers/firmware/efi/libstub/random.c >> @@ -33,3 +33,88 @@ efi_status_t efi_get_random_bytes(efi_system_table_t >> *sys_table, >> >> return rng->get_rng(rng, NULL, size, out); >> } >> + >> +/* >> + * Return a weight for a memory entry depending on how many offsets it >> covers >> + * that are suitably aligned and supply enough room for the allocation. >> + */ >> +static unsigned long get_entry_weight(efi_memory_desc_t *md, unsigned long >> size, >> + unsigned long align_bits) >> +{ >> + u64 start, end; >> + >> + if (md->type != EFI_CONVENTIONAL_MEMORY) >> + return 0; >> + >> + if (!(md->attribute & EFI_MEMORY_WB)) >> + return 0; > > This could do with a comment. When would EFI_CONVENTIONAL_MEMORY not > have this attribute capability in the memory map? > Actually, I think I should drop it instead. The other alloc functions only check for EFI_CONVENTIONAL_MEMORY, and this is intended to be generic code. Also, I have never seen a system with EFI_CONVENTIONAL_MEMORY with the WB bit cleared. >> + >> + start = round_up(md->phys_addr, 1 << align_bits); >> + end = round_down(md->phys_addr + md->num_pages * EFI_PAGE_SIZE - size, >> + 1 << align_bits); >> + >> + if (start >= end) >> + return 0; >> + >> + return (end - start) >> align_bits; >> +} >> + >> +/* >> + * The UEFI memory descriptors have a virtual address field that is only >> used >> + * when installing the virtual mapping using SetVirtualAddressMap(). Since >> it >> + * is unused here, we can reuse it to keep track of each descriptor's >> weight. >> + */ >> +#define MD_WEIGHT(md) ((md)->virt_addr) >> + >> +efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg, >> + unsigned long size, unsigned long align_bits, >> + unsigned long *addr, unsigned long random_seed) >> +{ >> + unsigned long map_size, desc_size, max_weight = 0, target; >> + efi_memory_desc_t *memory_map; >> + efi_status_t status = EFI_NOT_FOUND; >> + int l; > > Could you pick a more descriptive variable name? > Sure :-) >> + >> + status = efi_get_memory_map(sys_table_arg, &memory_map, &map_size, >> + &desc_size, NULL, NULL); >> + if (status != EFI_SUCCESS) >> + return status; >> + >> + /* assign each entry in the memory map a weight */ >> + for (l = 0; l < map_size; l += desc_size) { >> + efi_memory_desc_t *md = (void *)memory_map + l; >> + unsigned long weight; >> + >> + weight = get_entry_weight(md, size, align_bits); >> + MD_WEIGHT(md) = weight; >> + max_weight += weight; >> + } >> + >> + /* find a random number between 0 and max_weight */ >> + target = (max_weight * (u16)random_seed) >> 16; >> + >> + /* find the entry whose accumulated weight covers the target */ >> + for (l = 0; l < map_size; l += desc_size) { >> + efi_memory_desc_t *md = (void *)memory_map + l; >> + >> + if (target < MD_WEIGHT(md)) { >> + unsigned long pages; >> + >> + *addr = round_up(md->phys_addr, 1 << align_bits) + >> + (target << align_bits); >> + pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE; >> + >> + status = efi_call_early(allocate_pages, >> + EFI_ALLOCATE_ADDRESS, >> + EFI_LOADER_DATA, >> + pages, >> + (efi_physical_addr_t *)addr); > > You're mixing data types here. efi_physical_addr_t is always 64-bits, > but 'addr' is unsigned long, which is 32-bits on 32-bit platforms. > This cast isn't safe. > OK, I will fix that. >> + break; >> + } >> + target -= MD_WEIGHT(md); > > I think this needs a comment. Sure. Note that in my local version, I already replaced max_weight with total_weight since it wasn't entirely accurate. So I'll try to pick a better name for target as well. Thanks, Ard.
