Re: [PATCH V2 1/3] efi: Support for MOK variable config table

[Arvind Sankar]

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;
> > > +}