| Message ID | 20240906202745.11159-7-gourry@gourry.net |
|---|---|
| State | New |
| Headers | show |
| Series | libstub,tpm: fix small bugs and improve error reporting | expand |
On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote: > > Current code fails to check for an error case when reading events from > final event log to calculate offsets. Check the error case, report the > error, and break early because all subsequent calls will also fail. > > Signed-off-by: Gregory Price <gourry@gourry.net> > --- > drivers/firmware/efi/libstub/tpm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c > index 4f9f0e049a7a..c71b0d3e66d2 100644 > --- a/drivers/firmware/efi/libstub/tpm.c > +++ b/drivers/firmware/efi/libstub/tpm.c > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca > event_size = __calc_tpm2_event_size(header, > (void *)(long)log_location, > false); > + if (!event_size) { > + efi_err("Invalid TPM Final Event Log Entry\n"); > + break; > + } I don't object to this in principle, the only problem is that these log prints are not recorded anywhere: they are printed to the EFI boot console by the EFI stub, which may not even be visible, and is definitely not captured by the kernel logging routines.
On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote: > On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote: > > > > Current code fails to check for an error case when reading events from > > final event log to calculate offsets. Check the error case, report the > > error, and break early because all subsequent calls will also fail. > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > --- > > drivers/firmware/efi/libstub/tpm.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c > > index 4f9f0e049a7a..c71b0d3e66d2 100644 > > --- a/drivers/firmware/efi/libstub/tpm.c > > +++ b/drivers/firmware/efi/libstub/tpm.c > > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca > > event_size = __calc_tpm2_event_size(header, > > (void *)(long)log_location, > > false); > > + if (!event_size) { > > + efi_err("Invalid TPM Final Event Log Entry\n"); > > + break; > > + } > > I don't object to this in principle, the only problem is that these > log prints are not recorded anywhere: they are printed to the EFI boot > console by the EFI stub, which may not even be visible, and is > definitely not captured by the kernel logging routines. Could simply drop the err and break if you think it's just going to get lost anyway. Not sure there's a good way to generate a signal at this point. ~Gregory
On Fri, 13 Sept 2024 at 17:30, Gregory Price <gourry@gourry.net> wrote: > > On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote: > > On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote: > > > > > > Current code fails to check for an error case when reading events from > > > final event log to calculate offsets. Check the error case, report the > > > error, and break early because all subsequent calls will also fail. > > > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > > --- > > > drivers/firmware/efi/libstub/tpm.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c > > > index 4f9f0e049a7a..c71b0d3e66d2 100644 > > > --- a/drivers/firmware/efi/libstub/tpm.c > > > +++ b/drivers/firmware/efi/libstub/tpm.c > > > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca > > > event_size = __calc_tpm2_event_size(header, > > > (void *)(long)log_location, > > > false); > > > + if (!event_size) { > > > + efi_err("Invalid TPM Final Event Log Entry\n"); > > > + break; > > > + } > > > > I don't object to this in principle, the only problem is that these > > log prints are not recorded anywhere: they are printed to the EFI boot > > console by the EFI stub, which may not even be visible, and is > > definitely not captured by the kernel logging routines. > > Could simply drop the err and break if you think it's just going to get > lost anyway. Not sure there's a good way to generate a signal at this point. > Yeah. For the record, I absolutely detest the kludgy code there, how we parse the map, parse and unmap the event header for every entry in the log. So while I highly appreciate the effort you are putting in to polish this code, I wonder if it wouldn't be better to code this up properly instead.
On Fri, Sep 13, 2024 at 05:59:17PM +0200, Ard Biesheuvel wrote: > On Fri, 13 Sept 2024 at 17:30, Gregory Price <gourry@gourry.net> wrote: > > > > On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote: > > > On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote: > > > > > > > > Current code fails to check for an error case when reading events from > > > > final event log to calculate offsets. Check the error case, report the > > > > error, and break early because all subsequent calls will also fail. > > > > > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > > > --- > > > > drivers/firmware/efi/libstub/tpm.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c > > > > index 4f9f0e049a7a..c71b0d3e66d2 100644 > > > > --- a/drivers/firmware/efi/libstub/tpm.c > > > > +++ b/drivers/firmware/efi/libstub/tpm.c > > > > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca > > > > event_size = __calc_tpm2_event_size(header, > > > > (void *)(long)log_location, > > > > false); > > > > + if (!event_size) { > > > > + efi_err("Invalid TPM Final Event Log Entry\n"); > > > > + break; > > > > + } > > > > > > I don't object to this in principle, the only problem is that these > > > log prints are not recorded anywhere: they are printed to the EFI boot > > > console by the EFI stub, which may not even be visible, and is > > > definitely not captured by the kernel logging routines. > > > > Could simply drop the err and break if you think it's just going to get > > lost anyway. Not sure there's a good way to generate a signal at this point. > > > > Yeah. For the record, I absolutely detest the kludgy code there, how > we parse the map, parse and unmap the event header for every entry in > the log. > > So while I highly appreciate the effort you are putting in to polish > this code, I wonder if it wouldn't be better to code this up properly > instead. Mostly I was both amused and triggered by a patch trying to fix a signed/unsigned bug introducing another signed/unsigned bug lol. Then I found more and felt this injustice cannot stand x_x I'll at least push another version of these fixed up and i'll just drop this print. Probably improving this code isn't worth it unless it's fundamentally broken (which it does not *appear* to be, i spent a day auditing it against the spec, and to my eye it looks largely ok). ~Gregory
diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index 4f9f0e049a7a..c71b0d3e66d2 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca event_size = __calc_tpm2_event_size(header, (void *)(long)log_location, false); + if (!event_size) { + efi_err("Invalid TPM Final Event Log Entry\n"); + break; + } final_events_size += event_size; i--; }
Current code fails to check for an error case when reading events from final event log to calculate offsets. Check the error case, report the error, and break early because all subsequent calls will also fail. Signed-off-by: Gregory Price <gourry@gourry.net> --- drivers/firmware/efi/libstub/tpm.c | 4 ++++ 1 file changed, 4 insertions(+)