Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Arvind Sankar <niveditas98@gmail.com>
From:   Arvind Sankar <nivedita@alum.mit.edu>
Date:   Mon, 21 Sep 2020 12:55:06 -0400
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     Arvind Sankar <nivedita@alum.mit.edu>,
        Lenny Szubowicz <lszubowi@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-efi <linux-efi@vger.kernel.org>,
        platform-driver-x86@vger.kernel.org,
        linux-security-module@vger.kernel.org, andy.shevchenko@gmail.com,
        James Morris <jmorris@namei.org>, serge@hallyn.com,
        Kees Cook <keescook@chromium.org>,
        Mimi Zohar <zohar@linux.ibm.com>,
        Borislav Petkov <bp@alien8.de>,
        Peter Jones <pjones@redhat.com>,
        David Howells <dhowells@redhat.com>, prarit@redhat.com
Subject: Re: [PATCH V2 1/3] efi: Support for MOK variable config table
Message-ID: <20200921165506.GA549786@rani.riverdale.lan>
References: <20200905013107.10457-1-lszubowi@redhat.com>
 <20200905013107.10457-2-lszubowi@redhat.com>
 <20200921161859.GA544292@rani.riverdale.lan>
 <CAMj1kXFV7LqsyHM8iM5yQwJX4tKbY=w9vfjERvjyabVDKcbJpA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAMj1kXFV7LqsyHM8iM5yQwJX4tKbY=w9vfjERvjyabVDKcbJpA@mail.gmail.com>
Precedence: bulk

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