| Message ID | 20240830212852.2794145-1-usamaarif642@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi: reserve memory for tpm_log only if TPM final events table is valid | expand |
On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote: > If efi.tpm_log is corrupted, log_tbl->size can be garbage (and > negative). This can result in a large memblock reservation, resulting > in the kernel booting without sufficient memory. Move the memblock > reservation after log_tbl->version check, and check the value of > both tbl_size and memblock_reserve. > > Signed-off-by: Usama Arif <usamaarif642@gmail.com> > --- > drivers/firmware/efi/tpm.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > index e8d69bd548f3..cfc6a065f441 100644 > --- a/drivers/firmware/efi/tpm.c > +++ b/drivers/firmware/efi/tpm.c > @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) > return -ENOMEM; > } > > - tbl_size = sizeof(*log_tbl) + log_tbl->size; > - memblock_reserve(efi.tpm_log, tbl_size); > - > if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { > pr_info("TPM Final Events table not present\n"); > goto out; The final event table is not present in TCG 1_2 format, but the tpm log still needs to be mapped. So this change is incorrect for v1_2. > @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void) > goto out; > } > > + tbl_size = sizeof(*log_tbl) + log_tbl->size; > + if (tbl_size < 0) { > + pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n"); > + ret = -EINVAL; > + goto out; > + } > + if (memblock_reserve(efi.tpm_log, tbl_size)) { > + pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n", > + efi.tpm_log, tbl_size); > + ret = -ENOMEM; > + goto out; > + } > + > final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); > > if (!final_tbl) { > -- > 2.43.5 >
On 30/08/2024 17:42, Gregory Price wrote: > On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote: >> If efi.tpm_log is corrupted, log_tbl->size can be garbage (and >> negative). This can result in a large memblock reservation, resulting >> in the kernel booting without sufficient memory. Move the memblock >> reservation after log_tbl->version check, and check the value of >> both tbl_size and memblock_reserve. >> >> Signed-off-by: Usama Arif <usamaarif642@gmail.com> >> --- >> drivers/firmware/efi/tpm.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c >> index e8d69bd548f3..cfc6a065f441 100644 >> --- a/drivers/firmware/efi/tpm.c >> +++ b/drivers/firmware/efi/tpm.c >> @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) >> return -ENOMEM; >> } >> >> - tbl_size = sizeof(*log_tbl) + log_tbl->size; >> - memblock_reserve(efi.tpm_log, tbl_size); >> - >> if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { >> pr_info("TPM Final Events table not present\n"); >> goto out; > > The final event table is not present in TCG 1_2 format, but the > tpm log still needs to be mapped. So this change is incorrect for > v1_2. hmm so we have if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { pr_info("TPM Final Events table not present\n"); goto out; } else if (log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { pr_warn(FW_BUG "TPM Final Events table invalid\n"); goto out; } If the format is TCG 1_2, then log_tbl is not used? > >> @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void) >> goto out; >> } >> >> + tbl_size = sizeof(*log_tbl) + log_tbl->size; >> + if (tbl_size < 0) { >> + pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n"); >> + ret = -EINVAL; >> + goto out; >> + } >> + if (memblock_reserve(efi.tpm_log, tbl_size)) { >> + pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n", >> + efi.tpm_log, tbl_size); >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); >> >> if (!final_tbl) { >> -- >> 2.43.5 >>
On 30/08/2024 17:49, Usama Arif wrote: > > > On 30/08/2024 17:42, Gregory Price wrote: >> On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote: >>> If efi.tpm_log is corrupted, log_tbl->size can be garbage (and >>> negative). This can result in a large memblock reservation, resulting >>> in the kernel booting without sufficient memory. Move the memblock >>> reservation after log_tbl->version check, and check the value of >>> both tbl_size and memblock_reserve. >>> >>> Signed-off-by: Usama Arif <usamaarif642@gmail.com> >>> --- >>> drivers/firmware/efi/tpm.c | 16 +++++++++++++--- >>> 1 file changed, 13 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c >>> index e8d69bd548f3..cfc6a065f441 100644 >>> --- a/drivers/firmware/efi/tpm.c >>> +++ b/drivers/firmware/efi/tpm.c >>> @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) >>> return -ENOMEM; >>> } >>> >>> - tbl_size = sizeof(*log_tbl) + log_tbl->size; >>> - memblock_reserve(efi.tpm_log, tbl_size); >>> - >>> if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { >>> pr_info("TPM Final Events table not present\n"); >>> goto out; >> >> The final event table is not present in TCG 1_2 format, but the >> tpm log still needs to be mapped. So this change is incorrect for >> v1_2. > > hmm so we have > > if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { > pr_info("TPM Final Events table not present\n"); > goto out; > } else if (log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > pr_warn(FW_BUG "TPM Final Events table invalid\n"); > goto out; > } > > If the format is TCG 1_2, then log_tbl is not used? > Ah its the case that log_tbl->size will be valid, even if its TCG 2. Got it, Thanks! > >> >>> @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void) >>> goto out; >>> } >>> >>> + tbl_size = sizeof(*log_tbl) + log_tbl->size; >>> + if (tbl_size < 0) { >>> + pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n"); >>> + ret = -EINVAL; >>> + goto out; >>> + } >>> + if (memblock_reserve(efi.tpm_log, tbl_size)) { >>> + pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n", >>> + efi.tpm_log, tbl_size); >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + >>> final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); >>> >>> if (!final_tbl) { >>> -- >>> 2.43.5 >>> >
On Fri, Aug 30, 2024 at 10:49:46PM +0100, Usama Arif wrote: > > > On 30/08/2024 17:42, Gregory Price wrote: > > On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote: > >> If efi.tpm_log is corrupted, log_tbl->size can be garbage (and > >> negative). This can result in a large memblock reservation, resulting > >> in the kernel booting without sufficient memory. Move the memblock > >> reservation after log_tbl->version check, and check the value of > >> both tbl_size and memblock_reserve. > >> > >> Signed-off-by: Usama Arif <usamaarif642@gmail.com> > >> --- > >> drivers/firmware/efi/tpm.c | 16 +++++++++++++--- > >> 1 file changed, 13 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > >> index e8d69bd548f3..cfc6a065f441 100644 > >> --- a/drivers/firmware/efi/tpm.c > >> +++ b/drivers/firmware/efi/tpm.c > >> @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) > >> return -ENOMEM; > >> } > >> > >> - tbl_size = sizeof(*log_tbl) + log_tbl->size; > >> - memblock_reserve(efi.tpm_log, tbl_size); > >> - > >> if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { > >> pr_info("TPM Final Events table not present\n"); > >> goto out; > > > > The final event table is not present in TCG 1_2 format, but the > > tpm log still needs to be mapped. So this change is incorrect for > > v1_2. > > hmm so we have > > if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { > pr_info("TPM Final Events table not present\n"); > goto out; > } else if (log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > pr_warn(FW_BUG "TPM Final Events table invalid\n"); > goto out; > } > > If the format is TCG 1_2, then log_tbl is not used? > > if format is tcg 1_2 then log_tbl is used, which is reflected in libstub drivers/firmware/efi/libstub/tpm.c efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID; int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2); if (status == EFI_SUCCESS) { status = efi_call_proto(tpm2, get_event_log, version, &log_location, &log_last_entry, &truncated); if (status != EFI_SUCCESS || !log_location) { version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; status = efi_call_proto(tpm2, get_event_log, version, &log_location, &log_last_entry, &truncated); ^^^^^^^^ log still present in TCG 1_2 ^^^^^^^^^^^ } else { final_events_table = get_efi_config_table(EFI_TCG2_FINAL_EVENTS_TABLE_GUID); } } else { ~Gregory
On Fri, Aug 30, 2024 at 10:52:48PM +0100, Usama Arif wrote: > > > On 30/08/2024 17:49, Usama Arif wrote: > > > > > > On 30/08/2024 17:42, Gregory Price wrote: > >> On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote: > >>> If efi.tpm_log is corrupted, log_tbl->size can be garbage (and > >>> negative). This can result in a large memblock reservation, resulting > >>> in the kernel booting without sufficient memory. Move the memblock > >>> reservation after log_tbl->version check, and check the value of > >>> both tbl_size and memblock_reserve. > >>> > >>> Signed-off-by: Usama Arif <usamaarif642@gmail.com> > >>> --- > >>> drivers/firmware/efi/tpm.c | 16 +++++++++++++--- > >>> 1 file changed, 13 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > >>> index e8d69bd548f3..cfc6a065f441 100644 > >>> --- a/drivers/firmware/efi/tpm.c > >>> +++ b/drivers/firmware/efi/tpm.c > >>> @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) > >>> return -ENOMEM; > >>> } > >>> > >>> - tbl_size = sizeof(*log_tbl) + log_tbl->size; > >>> - memblock_reserve(efi.tpm_log, tbl_size); > >>> - > >>> if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { > >>> pr_info("TPM Final Events table not present\n"); > >>> goto out; > >> > >> The final event table is not present in TCG 1_2 format, but the > >> tpm log still needs to be mapped. So this change is incorrect for > >> v1_2. > > > > hmm so we have > > > > if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { > > pr_info("TPM Final Events table not present\n"); > > goto out; > > } else if (log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { > > pr_warn(FW_BUG "TPM Final Events table invalid\n"); > > goto out; > > } > > > > If the format is TCG 1_2, then log_tbl is not used? > > > > Ah its the case that log_tbl->size will be valid, even if its TCG 2. > Got it, Thanks! No, not necessarily. The corruption issue is entirely separate from the patches here. size can technically be 0xffffffff and treated as valid because as far as I can see the spec does not define a maximum size. the tl;dr here is that the above checks on tpm_final_log and log_tbl->version gate mapping/operating on the final log - but do not have anything to say about the event log. There isn't really a good sanity check for whether or not to memblock_reserve the log. Possibly you add a LOG_FORMAT_MAX and check if version >= MAX, but again that should be separate from the change that checks the return value of the memblock_reserve call. ~Gregory
On 30/08/2024 17:58, Gregory Price wrote: > On Fri, Aug 30, 2024 at 10:52:48PM +0100, Usama Arif wrote: >> >> >> On 30/08/2024 17:49, Usama Arif wrote: >>> >>> >>> On 30/08/2024 17:42, Gregory Price wrote: >>>> On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote: >>>>> If efi.tpm_log is corrupted, log_tbl->size can be garbage (and >>>>> negative). This can result in a large memblock reservation, resulting >>>>> in the kernel booting without sufficient memory. Move the memblock >>>>> reservation after log_tbl->version check, and check the value of >>>>> both tbl_size and memblock_reserve. >>>>> >>>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com> >>>>> --- >>>>> drivers/firmware/efi/tpm.c | 16 +++++++++++++--- >>>>> 1 file changed, 13 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c >>>>> index e8d69bd548f3..cfc6a065f441 100644 >>>>> --- a/drivers/firmware/efi/tpm.c >>>>> +++ b/drivers/firmware/efi/tpm.c >>>>> @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) >>>>> return -ENOMEM; >>>>> } >>>>> >>>>> - tbl_size = sizeof(*log_tbl) + log_tbl->size; >>>>> - memblock_reserve(efi.tpm_log, tbl_size); >>>>> - >>>>> if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { >>>>> pr_info("TPM Final Events table not present\n"); >>>>> goto out; >>>> >>>> The final event table is not present in TCG 1_2 format, but the >>>> tpm log still needs to be mapped. So this change is incorrect for >>>> v1_2. >>> >>> hmm so we have >>> >>> if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { >>> pr_info("TPM Final Events table not present\n"); >>> goto out; >>> } else if (log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { >>> pr_warn(FW_BUG "TPM Final Events table invalid\n"); >>> goto out; >>> } >>> >>> If the format is TCG 1_2, then log_tbl is not used? >>> >> >> Ah its the case that log_tbl->size will be valid, even if its TCG 2. >> Got it, Thanks! > > No, not necessarily. The corruption issue is entirely separate from the > patches here. size can technically be 0xffffffff and treated as valid > because as far as I can see the spec does not define a maximum size. > > the tl;dr here is that the above checks on tpm_final_log and log_tbl->version > gate mapping/operating on the final log - but do not have anything to > say about the event log. > > There isn't really a good sanity check for whether or not to memblock_reserve > the log. Possibly you add a LOG_FORMAT_MAX and check if version >= MAX, > but again that should be separate from the change that checks the return > value of the memblock_reserve call. > > ~Gregory Yes, makes sense. Thanks!
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index e8d69bd548f3..cfc6a065f441 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void) return -ENOMEM; } - tbl_size = sizeof(*log_tbl) + log_tbl->size; - memblock_reserve(efi.tpm_log, tbl_size); - if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) { pr_info("TPM Final Events table not present\n"); goto out; @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void) goto out; } + tbl_size = sizeof(*log_tbl) + log_tbl->size; + if (tbl_size < 0) { + pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n"); + ret = -EINVAL; + goto out; + } + if (memblock_reserve(efi.tpm_log, tbl_size)) { + pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n", + efi.tpm_log, tbl_size); + ret = -ENOMEM; + goto out; + } + final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); if (!final_tbl) {
If efi.tpm_log is corrupted, log_tbl->size can be garbage (and negative). This can result in a large memblock reservation, resulting in the kernel booting without sufficient memory. Move the memblock reservation after log_tbl->version check, and check the value of both tbl_size and memblock_reserve. Signed-off-by: Usama Arif <usamaarif642@gmail.com> --- drivers/firmware/efi/tpm.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)