On Mon, Sep 21, 2020 at 06:27:17PM +0200, Ard Biesheuvel wrote:

On Mon, 21 Sep 2020 at 18:19, Arvind Sankar [off-list ref] wrote:

quoted

On Fri, Sep 04, 2020 at 09:31:05PM -0400, Lenny Szubowicz wrote:

quoted

+     /*
+      * The EFI MOK config table must fit within a single EFI memory
+      * descriptor range.
+      */
+     err = efi_mem_desc_lookup(efi.mokvar_table, &md);
+     if (err) {
+             pr_warn("EFI MOKvar config table is not within the EFI memory map\n");
+             return;
+     }
+     end_pa = efi_mem_desc_end(&md);
+     if (efi.mokvar_table >= end_pa) {
+             pr_err("EFI memory descriptor containing MOKvar config table is invalid\n");
+             return;
+     }

efi_mem_desc_lookup() can't return success if efi.mokvar_table >= end_pa,
why check it again?

quoted

+     offset_limit = end_pa - efi.mokvar_table;
+     /*
+      * Validate the MOK config table. Since there is no table header
+      * from which we could get the total size of the MOK config table,
+      * we compute the total size as we validate each variably sized
+      * entry, remapping as necessary.
+      */
+     while (cur_offset + sizeof(*mokvar_entry) <= offset_limit) {
+             mokvar_entry = va + cur_offset;
+             map_size_needed = cur_offset + sizeof(*mokvar_entry);
+             if (map_size_needed > map_size) {
+                     if (va)
+                             early_memunmap(va, map_size);
+                     /*
+                      * Map a little more than the fixed size entry
+                      * header, anticipating some data. It's safe to
+                      * do so as long as we stay within current memory
+                      * descriptor.
+                      */
+                     map_size = min(map_size_needed + 2*EFI_PAGE_SIZE,
+                                    offset_limit);
+                     va = early_memremap(efi.mokvar_table, map_size);

Can't we just map the entire region from efi.mokvar_table to end_pa in
one early_memremap call before the loop and avoid all the remapping
logic?

I suppose that depends on whether there is a reasonable upper bound on
the size which is guaranteed to be mappable using early_memremap()
(e.g., 128 KB on 32-bit ARM, or 256 KB on other architectures)

Ah, sorry, I thought only the number of early mappings was limited, not
the size as well. We could still just map the maximum possible
(NR_FIX_BTMAPS * PAGE_SIZE), since it will fail anyway if the config
table turns out to be bigger than that?

quoted

quoted

+     if (va)
+             early_memunmap(va, map_size);
+     if (err) {
+             pr_err("EFI MOKvar config table is not valid\n");
+             return;
+     }

err will never be non-zero here: it was cleared when the
efi_mem_desc_lookup() was done. I think the initialization of err to
-EINVAL needs to be moved just prior to the loop.

quoted

+     efi_mem_reserve(efi.mokvar_table, map_size_needed);
+     efi_mokvar_table_size = map_size_needed;
+}