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

[Lenny Szubowicz <lszubowi@redhat.com>]

On 9/21/20 12:55 PM, Arvind Sankar wrote:
> On Mon, Sep 21, 2020 at 06:27:17PM +0200, Ard Biesheuvel wrote:
>> On Mon, 21 Sep 2020 at 18:19, Arvind Sankar <nivedita@alum.mit.edu> 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?

I agree it's unnecessary and I see that Ard has addressed this in a patch.

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

In practice, the loop will only do one or two mappings. That's because of
two factors.

First the code attempts to map a little more than it needs (2*EFI_PAGE_SIZE).
Secondly, right now only one entry in the MOKvar config table might be quite
large, i.e. the entry named MoKListRT. It's extremely likely that the header
of MokListRT entry will be encountered on the first mapping. If that entry
goes beyond what was originally mapped, then a second mapping will cover all
the data in that large entry as well as the remaining small entries that might
follow it.

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