On Mon, Sep 21, 2020 at 06:27:17PM +0200, Ard Biesheuvel wrote:
> On Mon, 21 Sep 2020 at 18:19, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
> >
> > On Fri, Sep 04, 2020 at 09:31:05PM -0400, Lenny Szubowicz wrote:
> > > + /*
> > > + * 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?
> >
> > > + 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?
>
>
> > > + 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.
> >
> > > + efi_mem_reserve(efi.mokvar_table, map_size_needed);
> > > + efi_mokvar_table_size = map_size_needed;
> > > +}
