[PATCH v3 19/21] efi: stub: add implementation of efi_random_alloc()

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.

This could do with a comment. When would EFI_CONVENTIONAL_MEMORY not
have this attribute capability in the memory map?

+
+	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?

+
+	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.

+			break;
+		}
+		target -= MD_WEIGHT(md);

I think this needs a comment.