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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200921165506.GA549786@rani.riverdale.lan>
Date:   Mon, 21 Sep 2020 12:55:06 -0400
From:   Arvind Sankar <nivedita@...m.mit.edu>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Arvind Sankar <nivedita@...m.mit.edu>,
        Lenny Szubowicz <lszubowi@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-efi <linux-efi@...r.kernel.org>,
        platform-driver-x86@...r.kernel.org,
        linux-security-module@...r.kernel.org, andy.shevchenko@...il.com,
        James Morris <jmorris@...ei.org>, serge@...lyn.com,
        Kees Cook <keescook@...omium.org>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Borislav Petkov <bp@...en8.de>,
        Peter Jones <pjones@...hat.com>,
        David Howells <dhowells@...hat.com>, prarit@...hat.com
Subject: Re: [PATCH V2 1/3] efi: Support for MOK variable config table

On Mon, Sep 21, 2020 at 06:27:17PM +0200, Ard Biesheuvel wrote:
> On Mon, 21 Sep 2020 at 18:19, Arvind Sankar <nivedita@...m.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?
> >
> > > +     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;
> > > +}