| Message ID | 20220621153623.3786960-1-ardb@kernel.org |
|---|---|
| Headers | show |
| Series | efi: Restructure EFI varstore driver | expand |
On Tue, Jun 21, 2022 at 05:36:18PM +0200, Ard Biesheuvel wrote: > Avoid the efivars layer and simply call the newly introduced EFI > varstore helpers instead. This simplifies the code substantially, and > also allows us to remove some hacks in the shared efivars layer that > were added for efi-pstore specifically. > > Since we don't store the name of the associated EFI variable into each > pstore record when enumerating them, we have to guess the variable name > it was constructed from at deletion time, since we no longer keep a > shadow copy of the variable store. To make this a bit more exact, store > the CRC-32 of the ASCII name into the pstore record's ECC region so we > can use it later to make an educated guess regarding the name of the EFI > variable. I wonder if pstore_record should have a "private" field for backends to use? That seems like it solve the need for overloading the ecc field, and allow for arbitrarily more information to be stored (i.e. store full efi var name instead of an easily-colliding crc32?)
On Tue, 21 Jun 2022 at 23:00, Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jun 21, 2022 at 05:36:18PM +0200, Ard Biesheuvel wrote: > > Avoid the efivars layer and simply call the newly introduced EFI > > varstore helpers instead. This simplifies the code substantially, and > > also allows us to remove some hacks in the shared efivars layer that > > were added for efi-pstore specifically. > > > > Since we don't store the name of the associated EFI variable into each > > pstore record when enumerating them, we have to guess the variable name > > it was constructed from at deletion time, since we no longer keep a > > shadow copy of the variable store. To make this a bit more exact, store > > the CRC-32 of the ASCII name into the pstore record's ECC region so we > > can use it later to make an educated guess regarding the name of the EFI > > variable. > > I wonder if pstore_record should have a "private" field for backends to > use? That seems like it solve the need for overloading the ecc field, > and allow for arbitrarily more information to be stored (i.e. store full > efi var name instead of an easily-colliding crc32?) > We could easily add that - we'd just have to decide how to free the memory it points to.
On Tue, Jun 21, 2022 at 05:36:15PM +0200, Ard Biesheuvel wrote: > If a pstore record has its ecc_notice_size field set to >0, it means the > record's buffer has that many additional bytes appended to the end that > carry backend specific metadata, typically used for error correction. > > Given that this is backend specific, and that user space cannot really > make sense of this metadata anyway, let's not expose it via the pstore > filesystem. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> "ecc_notice_size" is actually describing the length of the string generated and appended by persistent_ram_ecc_string(). I've been bothered by this string, though, as it confuses what was actually stored with additional lines. "Why does every entry end with a string about ECC?" I think it's more sensible to show to userspace the record "as stored". We already prepend some chunking details when a panic write may split the dump across multiple records, so if anyone needs this IN the userspace file contents again, it could move there. I'd rather ECC status be reported at boot, really. Given that nothing I can find[1] parses the ECC notice string, I think it'd be fine to just remove it from the string buffer entirely. -Kees [1] https://codesearch.debian.net/search?q=Corrected+bytes
On Tue, Jun 21, 2022 at 11:12:17PM +0200, Ard Biesheuvel wrote: > On Tue, 21 Jun 2022 at 23:00, Kees Cook <keescook@chromium.org> wrote: > > > > On Tue, Jun 21, 2022 at 05:36:18PM +0200, Ard Biesheuvel wrote: > > > Avoid the efivars layer and simply call the newly introduced EFI > > > varstore helpers instead. This simplifies the code substantially, and > > > also allows us to remove some hacks in the shared efivars layer that > > > were added for efi-pstore specifically. > > > > > > Since we don't store the name of the associated EFI variable into each > > > pstore record when enumerating them, we have to guess the variable name > > > it was constructed from at deletion time, since we no longer keep a > > > shadow copy of the variable store. To make this a bit more exact, store > > > the CRC-32 of the ASCII name into the pstore record's ECC region so we > > > can use it later to make an educated guess regarding the name of the EFI > > > variable. > > > > I wonder if pstore_record should have a "private" field for backends to > > use? That seems like it solve the need for overloading the ecc field, > > and allow for arbitrarily more information to be stored (i.e. store full > > efi var name instead of an easily-colliding crc32?) > > > > We could easily add that - we'd just have to decide how to free the > memory it points to. I assume the pstore core could do that since it manages the record lifetime already?
On Tue, 21 Jun 2022 at 23:21, Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jun 21, 2022 at 11:12:17PM +0200, Ard Biesheuvel wrote: > > On Tue, 21 Jun 2022 at 23:00, Kees Cook <keescook@chromium.org> wrote: > > > > > > On Tue, Jun 21, 2022 at 05:36:18PM +0200, Ard Biesheuvel wrote: > > > > Avoid the efivars layer and simply call the newly introduced EFI > > > > varstore helpers instead. This simplifies the code substantially, and > > > > also allows us to remove some hacks in the shared efivars layer that > > > > were added for efi-pstore specifically. > > > > > > > > Since we don't store the name of the associated EFI variable into each > > > > pstore record when enumerating them, we have to guess the variable name > > > > it was constructed from at deletion time, since we no longer keep a > > > > shadow copy of the variable store. To make this a bit more exact, store > > > > the CRC-32 of the ASCII name into the pstore record's ECC region so we > > > > can use it later to make an educated guess regarding the name of the EFI > > > > variable. > > > > > > I wonder if pstore_record should have a "private" field for backends to > > > use? That seems like it solve the need for overloading the ecc field, > > > and allow for arbitrarily more information to be stored (i.e. store full > > > efi var name instead of an easily-colliding crc32?) > > > > > > > We could easily add that - we'd just have to decide how to free the > > memory it points to. > > I assume the pstore core could do that since it manages the record > lifetime already? > So if priv is non-NULL when it frees the record, it passes it to kfree() ?
On Tue, Jun 21, 2022 at 11:33:29PM +0200, Ard Biesheuvel wrote: > On Tue, 21 Jun 2022 at 23:21, Kees Cook <keescook@chromium.org> wrote: > > > > On Tue, Jun 21, 2022 at 11:12:17PM +0200, Ard Biesheuvel wrote: > > > On Tue, 21 Jun 2022 at 23:00, Kees Cook <keescook@chromium.org> wrote: > > > > > > > > On Tue, Jun 21, 2022 at 05:36:18PM +0200, Ard Biesheuvel wrote: > > > > > Avoid the efivars layer and simply call the newly introduced EFI > > > > > varstore helpers instead. This simplifies the code substantially, and > > > > > also allows us to remove some hacks in the shared efivars layer that > > > > > were added for efi-pstore specifically. > > > > > > > > > > Since we don't store the name of the associated EFI variable into each > > > > > pstore record when enumerating them, we have to guess the variable name > > > > > it was constructed from at deletion time, since we no longer keep a > > > > > shadow copy of the variable store. To make this a bit more exact, store > > > > > the CRC-32 of the ASCII name into the pstore record's ECC region so we > > > > > can use it later to make an educated guess regarding the name of the EFI > > > > > variable. > > > > > > > > I wonder if pstore_record should have a "private" field for backends to > > > > use? That seems like it solve the need for overloading the ecc field, > > > > and allow for arbitrarily more information to be stored (i.e. store full > > > > efi var name instead of an easily-colliding crc32?) > > > > > > > > > > We could easily add that - we'd just have to decide how to free the > > > memory it points to. > > > > I assume the pstore core could do that since it manages the record > > lifetime already? > > > > So if priv is non-NULL when it frees the record, it passes it to kfree() ? That's my idea, yeah. I *think* it'll work; I haven't taken a super-close look, though.