| Message ID | 20221006224212.569555-9-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some pstore improvements | expand |
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?
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/
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/20200312011335.70750-1-qiuxu.zhuo@intel.com/ 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 <gpiccoli@igalia.com> 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/20200312011335.70750-1-qiuxu.zhuo@intel.com/ > > 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 <gpiccoli@igalia.com> 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 ?
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" <gpiccoli@igalia.com> 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.)
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);
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 <ardb@kernel.org> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- drivers/firmware/efi/efi-pstore.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)