Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size

From: Ard Biesheuvel <ardb@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	kernel-dev@igalia.com, kernel@gpiccoli.net, anton@enomsg.org,
	ccross@android.com, tony.luck@intel.com,
	linux-efi@vger.kernel.org
Subject: Re: [PATCH 8/8] efi: pstore: Add module parameter for setting the record size
Date: Fri, 7 Oct 2022 11:11:08 +0200	[thread overview]
Message-ID: <CAMj1kXF4UyRMh2Y_KakeNBHvkHhTtavASTAxXinDO1rhPe_wYg@mail.gmail.com> (raw)
In-Reply-To: <202210061614.8AA746094A@keescook>

On Fri, 7 Oct 2022 at 01:16, Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 06, 2022 at 07:42:12PM -0300, Guilherme G. Piccoli wrote:
> > By default, the efi-pstore backend hardcode the UEFI variable size
> > as 1024 bytes. That's not a big deal, but at the same time having
> > no way to change that in the kernel is a bit bummer for specialized
> > users - there is not such a limit in the UEFI specification.
>
> It seems to have been added in commit
> e0d59733f6b1 ("efivars, efi-pstore: Hold off deletion of sysfs entry until the scan is completed")
>

Yeah fortunately that kludge is gone now.

> But I see no mention of why it was introduced or how it was chosen.
>

There is some cargo cult from prehistoric EFI times going on here, it
seems. Or maybe just misinterpretation of the maximum size for the
variable *name* vs the variable itself.

> I remember hearing some horror stories from Matthew Garrett about older
> EFI implementations bricking themselves when they stored large
> variables, or something like that, but I don't know if that's meaningful
> here at all.
>

This was related to filling up the variable store to the point where
SetVariable() calls in the firmware itself would start failing,
bricking the system in the process.

The efivars layer below efi-pstore will take care of this, by ensuring
upfront that sufficient space is available.

> I think it'd be great to make it configurable! Ard, do you have any
> sense of what the max/min, etc, should be here?
>

Given that dbx on an arbitrary EFI system with secure boot enabled is
already almost 4k, that seems like a reasonable default. As for the
upper bound, there is no way to know what weird firmware bugs you
might tickle by choosing highly unusual values here.

If you need to store lots of data, you might want to look at [0] for
some work done in the past on using capsule update for preserving data
across a reboot. In the general case, this is not as useful, as the
capsule is only delivered to the firmware after invoking the
ResetSystem() EFI runtime service (as opposed to SetVariable() calls
taking effect immediately). However, if you need to capture large
amounts of data, and can tolerate the uncertainty involved in the
capsule approach, it might be a reasonable option.

[0] https://lore.kernel.org/all/20200312011335.70750-1-qiuxu.zhuo@intel.com/