On Mon, 22 Feb, at 03:29:14PM, Ard Biesheuvel wrote:

Please include,

#define pr_fmt(fmt)	"efi: " fmt

here. I've seen in the past that reporters will sometimes miss error
messages when searching for EFI failures if they are not prefixed with
"efi: ".

+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/memblock.h>
+
+#include <asm/early_ioremap.h>
+
+static int __initdata tbl_size;
+
+/*
+ * Reserve the memory associated with the Memory Attributes configuration
+ * table, if it exists.
+ */
+int __init efi_memattr_init(void)
+{
+	efi_memory_attributes_table_t *tbl;
+
+	if (efi.mem_attr_table == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	tbl = early_memremap(efi.mem_attr_table, sizeof(*tbl));
+	if (!tbl) {
+		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
+		       efi.mem_attr_table);
+		return -ENOMEM;
+	}
+
+	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
+	memblock_reserve(efi.mem_attr_table, tbl_size);
+	early_memunmap(tbl, sizeof(*tbl));
+	return 0;
+}

We should really be checking tbl::version here so that if the version
gets bumped in the future, perhaps due to a change in the structure
layout, we don't try and reserve a crazy amount of memory.

+/*
+ * Returns a copy @out of the UEFI memory descriptor @in if it is covered
+ * entirely by a UEFI memory map entry with matching attributes. The virtual
+ * address of @out is set according to the matching entry that was found.
+ */
+static bool validate_entry(const efi_memory_desc_t *in, efi_memory_desc_t *out)
+{

Might I suggest "entry_is_valid" or "valid_entry" for this function? A
boolean return value implies we're asking a question.

+	u64 in_paddr = in->phys_addr;
+	u64 in_size = in->num_pages << EFI_PAGE_SHIFT;
+	efi_memory_desc_t *md;
+
+	if (in->type != EFI_RUNTIME_SERVICES_CODE &&
+	    in->type != EFI_RUNTIME_SERVICES_DATA) {
+		pr_warn("MEMATTR table entry type should be RuntimeServiceCode or RuntimeServicesData\n");

Hehe, I'm not a slave to the 80-column rule but 106-columns is pretty
extreme. Could we reword this to shorten the string length?

+		return false;
+	}

Newline here please.

+	if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) {
+		pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n");
+		return false;
+	}

Newline here please.

+	if (PAGE_SIZE > EFI_PAGE_SIZE &&
+	    (!PAGE_ALIGNED(in->phys_addr) ||
+	     !PAGE_ALIGNED(in->num_pages << EFI_PAGE_SHIFT))) {
+		pr_warn("MEMATTR table entry misaligned\n");
+		return false;
+	}

Could you provide a comment for this check? I realise it's checking
conformance to the region alignment rules for arm64, but a comment to
that effect would be useful.

+	for_each_efi_memory_desc(&memmap, md) {
+		u64 md_paddr = md->phys_addr;
+		u64 md_size = md->num_pages << EFI_PAGE_SHIFT;
+
+		if (!(md->attribute & EFI_MEMORY_RUNTIME))
+			continue;

Newline here please.

+		if (md->virt_addr == 0)
+			/* no virtual mapping has been installed by the stub */
+			break;

Please use braces when there are multiple lines under the if().

+		if (md_paddr <= in_paddr && (in_paddr - md_paddr) < md_size) {

Let's save ourselves one level of indentation here and invert this
check, continuing if the inverted check is false.

+			/*
+			 * This entry covers the start of @in, check whether
+			 * it covers the end as well.
+			 */
+			if (md_paddr + md_size < in_paddr + in_size) {
+				pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n");
+				return false;
+			}

Newline please.

+			if (md->type != in->type) {
+				pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n");
+				return false;
+			}

Newline please.

+			*out = *in;
+			out->virt_addr = in_paddr +
+					 (md->virt_addr - md->phys_addr);
+			return true;
+		}
+	}
+	return false;
+}
+
+int __init efi_memattr_apply_permissions(struct mm_struct *mm,
+					 efi_memattr_perm_setter fn)
+{
+	efi_memory_attributes_table_t *tbl;
+	int i, ret;

It might be worth a comment above this function explaining that it
should only be invoked after the machine has entered virtual mode and
SetVirtualAddressMap() has been called.

Please include a comment explaining what an 'efi_memattr_perm_setter'
is and the rules (if any) constraining the implementation.

+extern int efi_memattr_init(void);
+extern int efi_memattr_apply_permissions(struct mm_struct *mm,
+					 efi_memattr_perm_setter fn);
+
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc(m, md)					   \
 	for ((md) = (m)->map;						   \
-- 
2.5.0