disabling MokListXRT vendor dbx addendum (Was: Re: Report of (additional) boot regression on MacBookPro14,3 with 15.4)

[Peter Jones]

On Mon, May 24, 2021 at 11:40 AM Julian Andres Klode
<julian.klode at canonical.com> wrote:
>
> On Mon, May 24, 2021 at 11:09:36AM -0400, Peter Jones wrote:
> > On Mon, May 24, 2021 at 9:53 AM Julian Andres Klode
> > <julian.klode at canonical.com> wrote:
> > > We believe that this, and several other issues we are seeing, are caused
> > > by our vendor dbx being 19 KB large.
> > >
> > > As such, we'd like to ship the attached patch to disable adding the
> > > vendor dbx to MokListX when it is being mirrored to MokListXRT.
> > >
> > > We do not believe that this is a security issue: The MokListXRT variable
> > > is not used by our released kernels yet, and we can just bake the vendor
> > > dbx into future kernels that do read MokListXRT and avoid passing it
> > > through the limited storage space.
> >
> > I'm not strongly against this, but what I'd /rather/ see is a flag
> > added to say to only add the "addend" to our config table, not to the
> > variable.
>
> The problem I saw is that the reading happens in the function doing the
> mirroring, so that needs refactoring. And I wanted to avoid this, as we
> really urgently need to get shim into a state where we can release it
> to stable Ubuntu releases, and doing the refactoring introduces more
> regression potential.

I think your patch is acceptable for the short term, since you're
already in a situation where kexec isn't honoring them.

> > > This should unblock MacBooks and the older ThinkPads which ran out of
> > > storage space while trying to mirror the var :)
> > >
> > > On a related note, we are also considering to disable mirroring entirely
> > > when not booting securely, so we don't even try to write vars on
> > > those MacBooks and X220 ThinkPads and such.
> >
> > There is value in mirroring even when not enforcing, though not so
> > much when SB is unsupported—it helps with the ability to understand
> > what would happen on a given machine if SB were enabled.  For the
> > longer term we need to be moving towards an Audit Mode+IEIT model when
> > not enforcing.  That said, again I'd be pretty happy with a solution
> > that just uses the config table and not the variables.  We've
> > demonstrated clearly that the variables simply do not work for any
> > non-trivial use case here; we can do "best effort" with the variables
> > for compatibility with older userland mokutil and kexec, but that's
> > all we're going to accomplish.
> >
> > The config table has to be the mandatory part going forward, and we
> > need to be looking at phasing out the use of variables anyway. So I
> > would encourage you to look at making that work correctly and only
> > nerfing the variables.
>
> Wondering if we should just have a VendorDbx field instead of merging
> it in the table. That would also avoid the refactoring altogether -
> well, so would removing mirroring ;).

I really don't like adding more variables with the same meaning that
kernel and userland tooling have to check. The ideal number of "db"
variables and "dbx" variables from that point of view are both one.
We're already at two for each, and every new variable we add to
express the same thing is another point of failure in every consumer.
Unless we have a good sense of why separate lists would be meaningful
to the code consuming it, it's hard for me to see justification for
not providing that code with a unified list.  If there are good use
cases for that, I could probably be convinced.