| Message ID | 20240906202745.11159-5-gourry@gourry.net |
|---|---|
| State | New |
| Headers | show |
| Series | libstub,tpm: fix small bugs and improve error reporting | expand |
Hi Gregory, On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote: > > If the log version is not sane (0 or >2), don't attempt to use > the rest of the log values for anything to avoid potential corruption. > > Signed-off-by: Gregory Price <gourry@gourry.net> > --- > drivers/firmware/efi/tpm.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > index 6e03eed0dc6f..9a080887a3e0 100644 > --- a/drivers/firmware/efi/tpm.c > +++ b/drivers/firmware/efi/tpm.c > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void) > return -ENOMEM; > } > > + if (!log_tbl->version || > + log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > + pr_err(FW_BUG "TPM Events table version invalid (%x)\n", > + log_tbl->version); > + early_memunmap(log_tbl, sizeof(*log_tbl)); > + efi.tpm_log = EFI_INVALID_TABLE_ADDR; > + return -EINVAL; I don't think we need this check at all. Did you actually see this happening? efi_retrieve_eventlog() that runs during the efistub tries to retrieve the log and the EFI protocol itself explicitly says that the firmware *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or 2.0 format. If the firmware does something wrong, we should report the FW BUG in that function, instead of installing the config tables Linux uses internally to handover the log and catching it late. Thanks /Ilias > + } > + > tbl_size = sizeof(*log_tbl) + log_tbl->size; > if (memblock_reserve(efi.tpm_log, tbl_size)) { > pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n", > -- > 2.43.0 >
On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote: > Hi Gregory, > > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote: > > > > If the log version is not sane (0 or >2), don't attempt to use > > the rest of the log values for anything to avoid potential corruption. > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > --- > > drivers/firmware/efi/tpm.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > > index 6e03eed0dc6f..9a080887a3e0 100644 > > --- a/drivers/firmware/efi/tpm.c > > +++ b/drivers/firmware/efi/tpm.c > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void) > > return -ENOMEM; > > } > > > > + if (!log_tbl->version || > > + log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > > + pr_err(FW_BUG "TPM Events table version invalid (%x)\n", > > + log_tbl->version); > > + early_memunmap(log_tbl, sizeof(*log_tbl)); > > + efi.tpm_log = EFI_INVALID_TABLE_ADDR; > > + return -EINVAL; > > I don't think we need this check at all. Did you actually see this happening? > efi_retrieve_eventlog() that runs during the efistub tries to retrieve > the log and the EFI protocol itself explicitly says that the firmware > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or > 2.0 format. If the firmware does something wrong, we should report the > FW BUG in that function, instead of installing the config tables Linux > uses internally to handover the log and catching it late. > > Thanks > /Ilias > We saw this happen and discovered it was a disagreement between EFI/OS/kexec causing the table to be overwritten during kexec. We've since found a fix for that. So the result was that it appeared the firmware was doing something wrong. The sanity check at least allowed us to boot without immediately crashing - because the tables don't get reinstalled, they get re-used (at least that's by best understanding of the whole interaction). If the check seems superfluous, i can drop it. > > > > + } > > + > > tbl_size = sizeof(*log_tbl) + log_tbl->size; > > if (memblock_reserve(efi.tpm_log, tbl_size)) { > > pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n", > > -- > > 2.43.0 > >
On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@gourry.net> wrote: > > On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote: > > Hi Gregory, > > > > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote: > > > > > > If the log version is not sane (0 or >2), don't attempt to use > > > the rest of the log values for anything to avoid potential corruption. > > > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > > --- > > > drivers/firmware/efi/tpm.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > > > index 6e03eed0dc6f..9a080887a3e0 100644 > > > --- a/drivers/firmware/efi/tpm.c > > > +++ b/drivers/firmware/efi/tpm.c > > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void) > > > return -ENOMEM; > > > } > > > > > > + if (!log_tbl->version || > > > + log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > > > + pr_err(FW_BUG "TPM Events table version invalid (%x)\n", > > > + log_tbl->version); > > > + early_memunmap(log_tbl, sizeof(*log_tbl)); > > > + efi.tpm_log = EFI_INVALID_TABLE_ADDR; > > > + return -EINVAL; > > > > I don't think we need this check at all. Did you actually see this happening? > > efi_retrieve_eventlog() that runs during the efistub tries to retrieve > > the log and the EFI protocol itself explicitly says that the firmware > > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or > > 2.0 format. If the firmware does something wrong, we should report the > > FW BUG in that function, instead of installing the config tables Linux > > uses internally to handover the log and catching it late. > > > > Thanks > > /Ilias > > > > We saw this happen and discovered it was a disagreement between EFI/OS/kexec > causing the table to be overwritten during kexec. We've since found a fix for > that. So the result was that it appeared the firmware was doing something > wrong. The sanity check at least allowed us to boot without immediately > crashing - because the tables don't get reinstalled, they get re-used > (at least that's by best understanding of the whole interaction). > > If the check seems superfluous, i can drop it. Ok, that explains why it wasn't caught earlier at least. I would prefer dropping it tbh, but I am going to defer to Ard for that. If we agree that this needs to go in btw, I think you should refactor it a bit. That function already defines an out: label, which unmaps memory. So you can rewrite the above as If(....) { ret = -EINVAL; efi.tpm_log = EFI_INVALID_TABLE_ADDR; goto out; } Cheers /Ilias > > > > > > > > + } > > > + > > > tbl_size = sizeof(*log_tbl) + log_tbl->size; > > > if (memblock_reserve(efi.tpm_log, tbl_size)) { > > > pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n", > > > -- > > > 2.43.0 > > >
On Fri, 13 Sept 2024 at 15:39, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@gourry.net> wrote: > > > > On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote: > > > Hi Gregory, > > > > > > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote: > > > > > > > > If the log version is not sane (0 or >2), don't attempt to use > > > > the rest of the log values for anything to avoid potential corruption. > > > > > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > > > --- > > > > drivers/firmware/efi/tpm.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > > > > index 6e03eed0dc6f..9a080887a3e0 100644 > > > > --- a/drivers/firmware/efi/tpm.c > > > > +++ b/drivers/firmware/efi/tpm.c > > > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void) > > > > return -ENOMEM; > > > > } > > > > > > > > + if (!log_tbl->version || > > > > + log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > > > > + pr_err(FW_BUG "TPM Events table version invalid (%x)\n", > > > > + log_tbl->version); > > > > + early_memunmap(log_tbl, sizeof(*log_tbl)); > > > > + efi.tpm_log = EFI_INVALID_TABLE_ADDR; > > > > + return -EINVAL; > > > > > > I don't think we need this check at all. Did you actually see this happening? > > > efi_retrieve_eventlog() that runs during the efistub tries to retrieve > > > the log and the EFI protocol itself explicitly says that the firmware > > > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or > > > 2.0 format. If the firmware does something wrong, we should report the > > > FW BUG in that function, instead of installing the config tables Linux > > > uses internally to handover the log and catching it late. > > > > > > Thanks > > > /Ilias > > > > > > > We saw this happen and discovered it was a disagreement between EFI/OS/kexec > > causing the table to be overwritten during kexec. We've since found a fix for > > that. So the result was that it appeared the firmware was doing something > > wrong. The sanity check at least allowed us to boot without immediately > > crashing - because the tables don't get reinstalled, they get re-used > > (at least that's by best understanding of the whole interaction). > > > > If the check seems superfluous, i can drop it. > > Ok, that explains why it wasn't caught earlier at least. I would > prefer dropping it tbh, but I am going to defer to Ard for that. > > If we agree that this needs to go in btw, I think you should refactor > it a bit. That function already defines an out: label, which unmaps > memory. So you can rewrite the above as > > If(....) { > ret = -EINVAL; > efi.tpm_log = EFI_INVALID_TABLE_ADDR; > goto out; > } > Validating a table that was created by the EFI stub seems redundant. If the version check needs to be tightened, please do so in efi_retrieve_tcg2_eventlog() (in the stub).
On Fri, 13 Sept 2024 at 15:44, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 13 Sept 2024 at 15:39, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@gourry.net> wrote: > > > > > > On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote: > > > > Hi Gregory, > > > > > > > > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote: > > > > > > > > > > If the log version is not sane (0 or >2), don't attempt to use > > > > > the rest of the log values for anything to avoid potential corruption. > > > > > > > > > > Signed-off-by: Gregory Price <gourry@gourry.net> > > > > > --- > > > > > drivers/firmware/efi/tpm.c | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > > > > > index 6e03eed0dc6f..9a080887a3e0 100644 > > > > > --- a/drivers/firmware/efi/tpm.c > > > > > +++ b/drivers/firmware/efi/tpm.c > > > > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void) > > > > > return -ENOMEM; > > > > > } > > > > > > > > > > + if (!log_tbl->version || > > > > > + log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > > > > > + pr_err(FW_BUG "TPM Events table version invalid (%x)\n", > > > > > + log_tbl->version); > > > > > + early_memunmap(log_tbl, sizeof(*log_tbl)); > > > > > + efi.tpm_log = EFI_INVALID_TABLE_ADDR; > > > > > + return -EINVAL; > > > > > > > > I don't think we need this check at all. Did you actually see this happening? > > > > efi_retrieve_eventlog() that runs during the efistub tries to retrieve > > > > the log and the EFI protocol itself explicitly says that the firmware > > > > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or > > > > 2.0 format. If the firmware does something wrong, we should report the > > > > FW BUG in that function, instead of installing the config tables Linux > > > > uses internally to handover the log and catching it late. > > > > > > > > Thanks > > > > /Ilias > > > > > > > > > > We saw this happen and discovered it was a disagreement between EFI/OS/kexec > > > causing the table to be overwritten during kexec. We've since found a fix for > > > that. So the result was that it appeared the firmware was doing something > > > wrong. The sanity check at least allowed us to boot without immediately > > > crashing - because the tables don't get reinstalled, they get re-used > > > (at least that's by best understanding of the whole interaction). > > > > > > If the check seems superfluous, i can drop it. > > > > Ok, that explains why it wasn't caught earlier at least. I would > > prefer dropping it tbh, but I am going to defer to Ard for that. > > > > If we agree that this needs to go in btw, I think you should refactor > > it a bit. That function already defines an out: label, which unmaps > > memory. So you can rewrite the above as > > > > If(....) { > > ret = -EINVAL; > > efi.tpm_log = EFI_INVALID_TABLE_ADDR; > > goto out; > > } > > > > Validating a table that was created by the EFI stub seems redundant. > If the version check needs to be tightened, please do so in > efi_retrieve_tcg2_eventlog() (in the stub). ... and actually, this version is set by the EFI stub based on which flavor of the TCG protocols it found. So i don't think we need this check to begin with. If we need to detect corruption of these tables, I'd prefer to add a checksum or something like that. But I don't think we should bother.
On Fri, Sep 13, 2024 at 03:47:03PM +0200, Ard Biesheuvel wrote: > > > If we agree that this needs to go in btw, I think you should refactor > > > it a bit. That function already defines an out: label, which unmaps > > > memory. So you can rewrite the above as > > > > > > If(....) { > > > ret = -EINVAL; > > > efi.tpm_log = EFI_INVALID_TABLE_ADDR; > > > goto out; > > > } > > > > > > > Validating a table that was created by the EFI stub seems redundant. > > If the version check needs to be tightened, please do so in > > efi_retrieve_tcg2_eventlog() (in the stub). > > ... and actually, this version is set by the EFI stub based on which > flavor of the TCG protocols it found. > > So i don't think we need this check to begin with. > > If we need to detect corruption of these tables, I'd prefer to add a > checksum or something like that. But I don't think we should bother. Will drop, east enough. Will send v2 later today. ~Gregory
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 6e03eed0dc6f..9a080887a3e0 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void) return -ENOMEM; } + if (!log_tbl->version || + log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { + pr_err(FW_BUG "TPM Events table version invalid (%x)\n", + log_tbl->version); + early_memunmap(log_tbl, sizeof(*log_tbl)); + efi.tpm_log = EFI_INVALID_TABLE_ADDR; + return -EINVAL; + } + tbl_size = sizeof(*log_tbl) + log_tbl->size; if (memblock_reserve(efi.tpm_log, tbl_size)) { pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
If the log version is not sane (0 or >2), don't attempt to use the rest of the log values for anything to avoid potential corruption. Signed-off-by: Gregory Price <gourry@gourry.net> --- drivers/firmware/efi/tpm.c | 9 +++++++++ 1 file changed, 9 insertions(+)