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.
So, add here a module parameter to enable advanced users to change
the UEFI record size for efi-pstore data collection.
Through empirical analysis we observed that extreme low values
(like 8 bytes) could eventually cause writing issues, so we limit
the low end to 1024 bytes (which is also the default).
Cc: Ard Biesheuvel <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
drivers/firmware/efi/efi-pstore.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 97a9e84840a0..78c27f6a83aa 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -10,7 +10,9 @@ MODULE_IMPORT_NS(EFIVAR);
#define DUMP_NAME_LEN 66
-#define EFIVARS_DATA_SIZE_MAX 1024
+static unsigned int record_size = 1024;
+module_param(record_size, uint, 0444);
+MODULE_PARM_DESC(record_size, "size of each pstore UEFI var (in bytes, min/default=1024)");
static bool efivars_pstore_disable =
IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE);
@@ -30,7 +32,7 @@ static int efi_pstore_open(struct pstore_info *psi)
if (err)
return err;
- psi->data = kzalloc(EFIVARS_DATA_SIZE_MAX, GFP_KERNEL);
+ psi->data = kzalloc(record_size, GFP_KERNEL);
if (!psi->data)
return -ENOMEM;
@@ -52,7 +54,7 @@ static inline u64 generic_id(u64 timestamp, unsigned int part, int count)
static int efi_pstore_read_func(struct pstore_record *record,
efi_char16_t *varname)
{
- unsigned long wlen, size = EFIVARS_DATA_SIZE_MAX;
+ unsigned long wlen, size = record_size;
char name[DUMP_NAME_LEN], data_type;
efi_status_t status;
int cnt;
@@ -133,7 +135,7 @@ static ssize_t efi_pstore_read(struct pstore_record *record)
efi_status_t status;
for (;;) {
- varname_size = EFIVARS_DATA_SIZE_MAX;
+ varname_size = record_size;
/*
* If this is the first read() call in the pstore enumeration,
@@ -224,11 +226,14 @@ static __init int efivars_pstore_init(void)
if (efivars_pstore_disable)
return 0;
+ if (record_size < 1024)
+ record_size = 1024;
+
efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
if (!efi_pstore_info.buf)
return -ENOMEM;
- efi_pstore_info.bufsize = 1024;
+ efi_pstore_info.bufsize = record_size;
if (pstore_register(&efi_pstore_info)) {
kfree(efi_pstore_info.buf);
--
2.38.0
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")
But I see no mention of why it was introduced or how it was chosen.
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.
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?
--
Kees Cook
On Fri, 7 Oct 2022 at 01:16, Kees Cook <[email protected]> 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/[email protected]/
First of all, thanks Ard for the historical explanation!
On 07/10/2022 06:11, Ard Biesheuvel wrote:
> [...]
>> 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/[email protected]/
So, you mean 4K as the default? I can change, but I would try to not
mess with the current users, is there a case you can imagine something
like 4k would fail? Maybe 2K is safer?
As for the maximum, I've tested with many values, and when it's larger
than ~30000 for edk2/ovmf, it fails with EFI_OUT_OF_RESOURCES and
doesn't collect the log; other than that, no issues observed.
When set to ~24000, the interesting is that we have fewer big logs in
/sys/fs/pstore, so it's nice to see compared to the bunch of 1K files heheh
Anyway, let's agree on the default and then I can resubmit that, I'm
glad you both consider that it's a good idea =)
Thanks,
Guilherme
On Fri, 7 Oct 2022 at 15:00, Guilherme G. Piccoli <[email protected]> wrote:
>
> First of all, thanks Ard for the historical explanation!
>
>
> On 07/10/2022 06:11, Ard Biesheuvel wrote:
> > [...]
> >> 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/[email protected]/
>
> So, you mean 4K as the default? I can change, but I would try to not
> mess with the current users, is there a case you can imagine something
> like 4k would fail? Maybe 2K is safer?
>
Reducing the number of writes 4x on systems that can support this has
its own advantages, so I am willing to risk it.
> As for the maximum, I've tested with many values, and when it's larger
> than ~30000 for edk2/ovmf, it fails with EFI_OUT_OF_RESOURCES and
> doesn't collect the log; other than that, no issues observed.
>
OVMF has
OvmfPkg/OvmfPkgX64.dsc:
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
OvmfPkg/OvmfPkgX64.dsc:
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
where the first one is without secure boot and the second with secure boot.
Interestingly, the default is
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
so this is probably where this 1k number comes from. So perhaps it is
better to leave it at 1k after all :-(
> When set to ~24000, the interesting is that we have fewer big logs in
> /sys/fs/pstore, so it's nice to see compared to the bunch of 1K files heheh
>
> Anyway, let's agree on the default and then I can resubmit that, I'm
> glad you both consider that it's a good idea =)
>
> Thanks,
>
>
> Guilherme
On 07/10/2022 10:19, Ard Biesheuvel wrote:
> [...]
>
> OVMF has
>
> OvmfPkg/OvmfPkgX64.dsc:
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> OvmfPkg/OvmfPkgX64.dsc:
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>
> where the first one is without secure boot and the second with secure boot.
>
> Interestingly, the default is
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
>
> so this is probably where this 1k number comes from. So perhaps it is
> better to leave it at 1k after all :-(
>
Oh darn...
So, let's stick with 1024 then? If so, no need for re-submitting right?
Thanks again,
Guilherme
On Fri, 7 Oct 2022 at 15:46, Guilherme G. Piccoli <[email protected]> wrote:
>
> On 07/10/2022 10:19, Ard Biesheuvel wrote:
> > [...]
> >
> > OVMF has
> >
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> >
> > where the first one is without secure boot and the second with secure boot.
> >
> > Interestingly, the default is
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
> >
> > so this is probably where this 1k number comes from. So perhaps it is
> > better to leave it at 1k after all :-(
> >
>
> Oh darn...
>
> So, let's stick with 1024 then? If so, no need for re-submitting right?
Well, I did spot this oddity
efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
if (!efi_pstore_info.buf)
return -ENOMEM;
efi_pstore_info.bufsize = 1024;
So that hardcoded 4096 looks odd, but at least it is larger than the
default 1024. So what happens if you increase the record size to >
4096?
On 07/10/2022 12:06, Ard Biesheuvel wrote:
> [...]
> Well, I did spot this oddity
>
> efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
> if (!efi_pstore_info.buf)
> return -ENOMEM;
>
> efi_pstore_info.bufsize = 1024;
>
> So that hardcoded 4096 looks odd, but at least it is larger than the
> default 1024. So what happens if you increase the record size to >
> 4096?
This is a very good finding, thanks a bunch Ard and apologies for this
mistake!
Before this patch it was "safe" doing this way since the allocation was
4096 whereas the size value was 1024. Now, with my change this is not
valid anymore, and my feeling is that it worked fine in my tests because
I'm testing panic (which is a single CPU/no-IRQ scenario), so basically
we're corrupting memory...but nothing broke in my tests due to panic
scenario.
Thanks again, I'll fix that - need to allocate record_size.
Guilherme
On Fri, Oct 07, 2022 at 10:45:33AM -0300, Guilherme G. Piccoli wrote:
> On 07/10/2022 10:19, Ard Biesheuvel wrote:
> > [...]
> >
> > OVMF has
> >
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> > OvmfPkg/OvmfPkgX64.dsc:
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> >
> > where the first one is without secure boot and the second with secure boot.
> >
> > Interestingly, the default is
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x400
> >
> > so this is probably where this 1k number comes from. So perhaps it is
> > better to leave it at 1k after all :-(
> >
>
> Oh darn...
>
> So, let's stick with 1024 then? If so, no need for re-submitting right?
Given OVMF showing this as a max, it doesn't seem right to also make
this a minimum? Perhaps choose a different minimum to be enforced.
Also, can you update the commit log with Ard's archeology on
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ?
--
Kees Cook
On 07/10/2022 16:32, Kees Cook wrote:
> [...]
> Given OVMF showing this as a max, it doesn't seem right to also make
> this a minimum? Perhaps choose a different minimum to be enforced.
Hi Kees! Through my tests, I've noticed low values tend to cause issues
(didn't go further in the investigation), IIRC even 512 caused problems
on "deflate" (worked in the others).
I'll try again 512 to see how it goes, but I'm not so sure what would be
the use of such low values, it does truncate a lot and "pollute" the
pstore fs with many small files. But I can go with any value you/Ard
think is appropriate (given it works with all compression algorithms
heh) - currently the minimum of 1024 is enforced in the patch.
>
> Also, can you update the commit log with Ard's archeology on
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ?
>
Sure, of course!
Cheers,
Guilherme
On October 7, 2022 4:29:55 PM PDT, "Guilherme G. Piccoli" <[email protected]> wrote:
>On 07/10/2022 16:32, Kees Cook wrote:
>> [...]
>> Given OVMF showing this as a max, it doesn't seem right to also make
>> this a minimum? Perhaps choose a different minimum to be enforced.
>
>Hi Kees! Through my tests, I've noticed low values tend to cause issues
>(didn't go further in the investigation), IIRC even 512 caused problems
>on "deflate" (worked in the others).
>
>I'll try again 512 to see how it goes, but I'm not so sure what would be
>the use of such low values, it does truncate a lot and "pollute" the
>pstore fs with many small files. But I can go with any value you/Ard
>think is appropriate (given it works with all compression algorithms
>heh) - currently the minimum of 1024 is enforced in the patch.
Right, but not everyone uses compression. On the other hand, this was never configurable before, so, sure, let's do 1k as a minimum. (And a comment in the source.)
--
Kees Cook