Re: [Crash-utility] RISCV64: Use va_kernel_pa_offset in VTOP()
On 2023/07/24 13:06, Song Shuai wrote:
> Since RISC-V Linux v6.4, the commit 3335068f8721 ("riscv: Use
> PUD/P4D/PGD pages for the linear mapping") changes the
> phys_ram_base from the kernel_map.phys_addr to the start of DRAM.
>
> The Crash's VTOP() still uses phys_ram_base and kernel_map.virt_addr
> to translate kernel virtual address, that made Crash boot failed with
> Linux v6.4 and later version.
>
> Let Linux export kernel_map.va_kernel_pa_offset in v6.5 and Crash can
> use "va_kernel_pa_offset" to translate the kernel virtual address in
> VTOP() correctly.
>
> Signed-off-by: Song Shuai
> ---
> You can check/test the Linux changes from this link:
> https://github.com/sugarfillet/linux/commits/6.5-rc3-crash
>
> And I'll send the Linux changes to riscv/for-next If you're ok with this
> patch.
> ---
> defs.h| 4 ++--
> riscv64.c | 22 ++
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 358f365..46b9857 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -3662,8 +3662,7 @@ typedef signed int s32;
> ulong _X = X;
> \
> (THIS_KERNEL_VERSION >= LINUX(5,13,0) &&
> \
> (_X) >= machdep->machspec->kernel_link_addr) ?
> \
> - (((unsigned long)(_X)-(machdep->machspec->kernel_link_addr)) +
> \
> - machdep->machspec->phys_base):
> \
> + ((unsigned long)(_X)-(machdep->machspec->va_kernel_pa_offset)):
> \
> (((unsigned long)(_X)-(machdep->kvbase)) +
> \
>machdep->machspec->phys_base);
> \
> })
> @@ -7021,6 +7020,7 @@ struct machine_specific {
> ulong modules_vaddr;
> ulong modules_end;
> ulong kernel_link_addr;
> + ulong va_kernel_pa_offset;
>
> ulong _page_present;
> ulong _page_read;
> diff --git a/riscv64.c b/riscv64.c
> index 6b9a688..b9e50b4 100644
> --- a/riscv64.c
> +++ b/riscv64.c
> @@ -418,6 +418,27 @@ error:
> error(FATAL, "cannot get vm layout\n");
> }
>
> +static void
> +riscv64_get_va_kernel_pa_offset(struct machine_specific *ms)
> +{
> + unsigned long kernel_version = riscv64_get_kernel_version();
> +
> + /*
> + * va_kernel_pa_offset is defined in Linux kernel since 6.5.
> + */
> + if (kernel_version >= LINUX(6,5,0)) {
The kernel patches look accepted, so for the crash patch detail,
I think this first version check is not necessary, we can just use the
vmcoreinfo entry if available. With it, backporting the kernel patches
to e.g. 6.4.0 will also be supported.
Thanks,
Kazu
> + char *string;
> + if ((string =
> pc->read_vmcoreinfo("NUMBER(va_kernel_pa_offset)"))) {
> + ms->va_kernel_pa_offset = htol(string, QUIET, NULL);
> + free(string);
> + } else
> + error(FATAL, "cannot read va_kernel_pa_offset\n");
> + } else if (kernel_version >= LINUX(6,4,0))
> + error(FATAL, "cannot determine va_kernel_pa_offset since Linux
> 6.4\n");
> + else
> + ms->va_kernel_pa_offset = ms->kernel_link_addr - ms->phys_base;
> +}
> +
> static int
> riscv64_is_kvaddr(ulong vaddr)
> {
> @@ -1352,6 +1373,7 @@ riscv64_init(int when)
> riscv64_get_struct_page_size(machdep->machspec);
> riscv64_get_va_bits(machdep->machspec);
> riscv64_get_va_range(machdep->machspec);
> + riscv64_get_va_kernel_pa_offset(machdep->machspec);
>
> pt_level_alloc(>pgd, "cannot malloc pgd space.");
> pt_level_alloc(>machspec->p4d, "cannot malloc p4d
> space.");
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter
On Thu, 2023-08-03 at 16:01 -0700, Tushar Sugandhi wrote:
> >> +scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
> >> + update_counter);
> >> +
> >> +buf_len = strlen(buf);
> >> +
> >> +result = ima_measure_critical_data("tpm_pcr_update_counter",
> >> event_name,
> >> + buf, buf_len, false, NULL, 0);
> >>
> > The new record should contain everything needed to verify the
> > pcrCounter. For example, each IMA measurement record updates the
> > pcrCounter for each TPM bank enabled. So the number of enabled TPM
> > banks and number of IMA measurements should also be included in this
> > record.
> Agreed. That should be valuable information.
> How does the below format look like for the buf above?
>
> version=..;num_enabled_pcr_banks=;pcrUpdateCounter=;num_ima_measurements=;
Refer to comment in 5/6.
> > Perhaps include a version number as well, so that if we ever want to
> > include other information, we could.
> By version number, do you mean kernel_version, or a new version
> number specific to this record? Or something else?
This is a record version type number. The record format shouldn't
change, but we should be prepared for it changing. A single number
should be fine.
--
thanks,
Mimi
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 5/6] ima: measure TPM update counter at ima_init
On Thu, 2023-08-03 at 16:34 -0700, Tushar Sugandhi wrote:
> >> +++ b/security/integrity/ima/ima_init.c
> >> @@ -154,5 +154,8 @@ int __init ima_init(void)
> >> UTS_RELEASE, strlen(UTS_RELEASE), false,
> >> NULL, 0);
> >>
> >> + /* Measures TPM update counter at ima_init */
> >> + ima_measure_update_counter("ima_init_tpm_update_counter");
> >> +
> > With "ima_policy=critical_data" on the boot command line, the IMA
> > measurement list record looks like:
> >
> > 6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter
> > 7570646174655f636f756e7465723d3330383b
> >
> > Please change the "ima_init_tpm_update_counter" to something shorter
> > and the hex encoded ascii string and pcr counter to something readable.
> I believe you are seeing the above line in ascill_runtime_measurements log.
Yes, the ascii_runtime_measurements are suppose to be readable to the
end user.
> The ascii logging format is consistent with other event data for
> critical_data event e.g. kernel_version.
Then you got it wrong.
> 10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf
> sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version
> 362e332e302d7263332b
> 10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf
> sha1:d8c187524412f74a961f2051a9529c009e700337
> ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b
>
> Entries in the binary runtime measurements look readable to me.
You've inverted the meaning of the ascii and binary runtime measurement
lists. For comparison look at the ima-ng/ima-sig records.
> ima_init_tpm_update_counter update_counter=130;
> ...
> kexec_load_tpm_update_counte rupdate_counter=133;
>
> Please let me know if you still want me to change the format.
OI course the ascii measurement list should be human readable.
> > Perhaps name this critical-data "tpm" and "tpm-info", similar to the
> From patch 4/6:
> +result = ima_measure_critical_data("tpm_pcr_update_counter",
> event_name,
> + buf, buf_len, false, NULL, 0);
>
> The critical_data event_label value is currently set to
> "tpm_pcr_update_counter".
Why is the string so long? Long strings or variables don't make the
code or logs more understandable. Please shorten both the strings and
variables.
> I can rename event_label to "tpm-info", so that the admins can filter the
> event in IMA policy based on the label if needed.
The new record needs to be self containd and verifiable. The
additional info I suggested were just examples. Please take the time
to consider what needs to be included in the new record. Decide
whether this is a TPM security critical data record. Only then, decide
on the naming.
--
thanks,
Mimi
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 5/6] ima: measure TPM update counter at ima_init
On 8/3/23 15:15, Mimi Zohar wrote:
On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
IMA log entries can be lost due to a variety of causes, such as code bugs
or error conditions, leading to a mismatch between TPM PCRs and
the IMA log. Measuring TPM PCR update counter during ima_init would
provide a baseline counter for the number of times the TPM PCRs are
updated. The remote attestation service can compare this baseline
counter with a subsequent measured one (e.g., post-kexec soft-boot) to
identify if there are any lost IMA log events.
Measure the TPM update counter at ima init.
No need for separate patches for one line changes like this. Either
merge patches 5/6 and 6/6 or all three 4/6, 5/6, 6/6 together.
Sounds good.
I will merge 4/6, 5/6, 6/6 together.
Signed-off-by: Tushar Sugandhi
---
security/integrity/ima/ima_init.c | 3 +++
security/integrity/ima/ima_main.c | 1 +
2 files changed, 4 insertions(+)
diff --git a/security/integrity/ima/ima_init.c
b/security/integrity/ima/ima_init.c
index 63979aefc95f..9bb18d6c2fd6 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -154,5 +154,8 @@ int __init ima_init(void)
UTS_RELEASE, strlen(UTS_RELEASE), false,
NULL, 0);
+ /* Measures TPM update counter at ima_init */
+ ima_measure_update_counter("ima_init_tpm_update_counter");
+
With "ima_policy=critical_data" on the boot command line, the IMA
measurement list record looks like:
6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter
7570646174655f636f756e7465723d3330383b
Please change the "ima_init_tpm_update_counter" to something shorter
and the hex encoded ascii string and pcr counter to something readable.
I believe you are seeing the above line in ascill_runtime_measurements log.
The ascii logging format is consistent with other event data for
critical_data event e.g. kernel_version.
10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf
sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version
362e332e302d7263332b
10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf
sha1:d8c187524412f74a961f2051a9529c009e700337
ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b
Entries in the binary runtime measurements look readable to me.
ima_init_tpm_update_counter update_counter=130;
...
kexec_load_tpm_update_counte rupdate_counter=133;
Please let me know if you still want me to change the format.
Perhaps name this critical-data "tpm" and "tpm-info", similar to the
From patch 4/6:
+ result = ima_measure_critical_data("tpm_pcr_update_counter",
event_name,
+ buf, buf_len, false, NULL, 0);
The critical_data event_label value is currently set to
"tpm_pcr_update_counter".
I can rename event_label to "tpm-info", so that the admins can filter the
event in IMA policy based on the label if needed.
As you know, event_label doesn't appear in IMA log, it can appear in IMA
policy.
Whereas event_name appears in IMA log.
I was thinking of using event_name to identify when was the info captured.
(e.g. ima_init, kexec_load, or at some other event in future).
We can either do
(a)
event_label = "tpm-info" event_name = "tpm-info-ima-init" |
"tpm-info-kexec-load" | ...
-or-
(b)
event_label = "tpm" event_name = "tpm-info"
and event_data to describe the where/when this info was captured.
e.g.
version=..;num_enabled_pcr_banks=;pcrUpdateCounter=;num_ima_measurements=;event=kexec_load;
Let me know if you would prefer option (a) or (b) or something else.
SELinux "selinux" and "selinux-state". Then again, if this is TPM
critical-data we should rethink what other info should be included.
As you suggested in Patch 4/6, I will add version, number of enabled pcr
banks,
pcrUpdateCounter, and num_ima_measurements. I think we should include
the TPM
version as well (1 v/s 2).
Please let me know if you think of any other attribute to record.
return rc;
}
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 1bcd45cc5a6a..93357c245e82 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1035,6 +1035,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf,
int size)
buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
NULL, false, NULL, 0);
fdput(f);
+
}
/**
Unnecessary change.
oops. Thanks for catching. Will fix.
~Tushar
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter
Thanks for the review Mimi.
On 8/3/23 14:42, Mimi Zohar wrote:
On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
Currently TPM update counter is not available external to the system,
for instance, a remote attestation service. It is a problem because
the service cannot easily determine if the IMA log entries are missing.
The IMA functionality needs to be extended to measure the TPM update
counter from various subsystems in Linux kernel to help detect if
the IMA log entries are missing.
Implement a function, 'ima_measure_update_counter()' which would retrieve
the TPM update counter using the previously defined function
'ima_tpm_get_update_counter()'. Format it as a string with the value
"update_counter=;", and measure it using the function
'ima_measure_critical_data()'.
The function takes an event name as input, and the update counter value
is measured as part of this event.
Signed-off-by: Tushar Sugandhi
Explicit TPM2 quote commands do not return the quoted PCR values or the
pcrCounter value. Define and include a new IMA measurement record
containing the pcrCounter, other TPM info, and IMA info in the IMA
measurement list to help simplify detecting a trimmed/truncated
measurement list.
Sounds good.
---
include/linux/ima.h | 1 +
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 28
3 files changed, 30 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86b57757c7b1..f15f3a6a4c72 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label,
const char *event_name,
const void *buf, size_t buf_len,
bool hash, u8 *digest, size_t digest_len);
+int ima_measure_update_counter(const char *event_name);
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
extern void ima_appraise_parse_cmdline(void);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4acd0e5a830f..5484bd362237 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -168,6 +168,7 @@ int __init ima_init_digests(void);
int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
void *lsm_data);
int ima_tpm_get_update_counter(u32 *cpu_update_counter);
+int ima_measure_update_counter(const char *event_name);
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index d66a0a36415e..1bcd45cc5a6a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label,
}
EXPORT_SYMBOL_GPL(ima_measure_critical_data);
+#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128
+int ima_measure_update_counter(const char *event_name)
+{
+ int result;
+ u32 update_counter = 0;
+ char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE];
+ int buf_len;
+
+ if (!event_name)
+ return -ENOPARAM;
+
+ result = ima_tpm_get_update_counter(_counter);
+
+ if (result != 0)
+ return result;
+
+ scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
+ update_counter);
+
+ buf_len = strlen(buf);
+
+ result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
+ buf, buf_len, false, NULL, 0);
The new record should contain everything needed to verify the
pcrCounter. For example, each IMA measurement record updates the
pcrCounter for each TPM bank enabled. So the number of enabled TPM
banks and number of IMA measurements should also be included in this
record.
Agreed. That should be valuable information.
How does the below format look like for the buf above?
version=..;num_enabled_pcr_banks=;pcrUpdateCounter=;num_ima_measurements=;
Perhaps include a version number as well, so that if we ever want to
include other information, we could.
By version number, do you mean kernel_version, or a new version
number specific to this record? Or something else?
~Tushar
Mimi
+
+ return result;
+}
+EXPORT_SYMBOL_GPL(ima_measure_update_counter);
+
static int __init init_ima(void)
{
int error;
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/6] Measuring TPM update counter in IMA
On 8/3/23 18:36, Mimi Zohar wrote: On Thu, 2023-08-03 at 18:09 -0400, Stefan Berger wrote: I can remove the kexec example if it is causing confusion.> Please let me know. I am not convinced we need this series ... :-( Your kexec series prevents further logging and especially PCR extensions after the frozen measurement log has been created and in ima_add_template_entry(), if we hit an oom condition, then we luckily do not extend the PCR either. If either the log was to have one more entry than number PCR extensions occurred or vice versa, then the remote attestation service will see this mismatch no matter what and all the PCR update counter won't help (and is generally not a good indicator for this purpose imo) for it to recover from this. It's better to declare the system as un-trusted/ corrupted in this case then. As previously mentioned, there is a patch set that doesn't carry any records across kexec, if the the measurement list is too large, and another proposal to trim the measurement list. In both of these cases including a new IMA mesaurement record, at least after the boot_aggregate, would help simplify detecting whether the measurement list has been trimmed/truncated. And if you can detect that I would log an event but not using the PCR update counter. Unless the state of PCRs is also logged, it's going to be unrecoverable for a log+quote verifier from there. Stefan ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/6] Measuring TPM update counter in IMA
On Thu, 2023-08-03 at 18:09 -0400, Stefan Berger wrote: > > I can remove the kexec example if it is causing confusion.> Please let me > > know. > > I am not convinced we need this series ... :-( Your kexec series prevents > further logging and especially PCR extensions after the frozen measurement log > has been created and in ima_add_template_entry(), if we hit an oom condition, > then we luckily do not extend the PCR either. If either the log was to have > one > more entry than number PCR extensions occurred or vice versa, then the remote > attestation service will see this mismatch no matter what and all the PCR > update > counter won't help (and is generally not a good indicator for this purpose > imo) > for it to recover from this. It's better to declare the system as un-trusted/ > corrupted in this case then. As previously mentioned, there is a patch set that doesn't carry any records across kexec, if the the measurement list is too large, and another proposal to trim the measurement list. In both of these cases including a new IMA mesaurement record, at least after the boot_aggregate, would help simplify detecting whether the measurement list has been trimmed/truncated. -- thanks, Mimi ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 5/6] ima: measure TPM update counter at ima_init
On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
> IMA log entries can be lost due to a variety of causes, such as code bugs
> or error conditions, leading to a mismatch between TPM PCRs and
> the IMA log. Measuring TPM PCR update counter during ima_init would
> provide a baseline counter for the number of times the TPM PCRs are
> updated. The remote attestation service can compare this baseline
> counter with a subsequent measured one (e.g., post-kexec soft-boot) to
> identify if there are any lost IMA log events.
>
> Measure the TPM update counter at ima init.
No need for separate patches for one line changes like this. Either
merge patches 5/6 and 6/6 or all three 4/6, 5/6, 6/6 together.
>
> Signed-off-by: Tushar Sugandhi
> ---
> security/integrity/ima/ima_init.c | 3 +++
> security/integrity/ima/ima_main.c | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> index 63979aefc95f..9bb18d6c2fd6 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -154,5 +154,8 @@ int __init ima_init(void)
> UTS_RELEASE, strlen(UTS_RELEASE), false,
> NULL, 0);
>
> + /* Measures TPM update counter at ima_init */
> + ima_measure_update_counter("ima_init_tpm_update_counter");
> +
With "ima_policy=critical_data" on the boot command line, the IMA
measurement list record looks like:
6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter
7570646174655f636f756e7465723d3330383b
Please change the "ima_init_tpm_update_counter" to something shorter
and the hex encoded ascii string and pcr counter to something readable.
Perhaps name this critical-data "tpm" and "tpm-info", similar to the
SELinux "selinux" and "selinux-state". Then again, if this is TPM
critical-data we should rethink what other info should be included.
> return rc;
> }
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 1bcd45cc5a6a..93357c245e82 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1035,6 +1035,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf,
> int size)
> buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
> NULL, false, NULL, 0);
> fdput(f);
> +
> }
>
> /**
Unnecessary change.
--
thanks,
Mimi
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/6] Measuring TPM update counter in IMA
On 8/3/23 17:30, Tushar Sugandhi wrote: Thanks Stefan for reviewing this series. Appreciate it. On 8/3/23 06:37, Stefan Berger wrote: On 8/1/23 14:19, Tushar Sugandhi wrote: Entries in IMA log may be lost due to code bugs, certain error conditions I hope we don't have such bugs. And I guess the most critical ones would be between logging and PCR extensions I hope so too, but in general, it’s hard to prove a negative. That’s why this patch series. :) being met etc. This can result in TPM PCRs getting out of sync with the IMA log. One such example is events between kexec 'load' and kexec 'execute' getting lost from the IMA log when the system soft-boots into the new Kernel using kexec[1]. The remote attestation service does not Though this particular condition I thought would go away with your kexec series. Currently the events in-between kexec ‘load’ and ‘execute’ are always lost – because IMA log is captured at ‘load’. My kexec series addresses this scenario. But as you said, there is always a possibility that the events will still be lost during kexec because of error conditions, OOM etc. The other conditions would be an out-of-memory or TPM failure. The OOM would probably be more critical since something that was supposed to be logged couldn't be logged and now you cannot show this anymore and presumably not even an error condition could be logged. Precisely. As you can see in patch 5 of this series, I am logging the counter at ima_init (ima_init_tpm_update_counter). So we will get the baseline counter at the system boot, where memory pressure is hopefully low. Even if we are not able to log the counter later due to OOM, this baseline counter along with the number of events in the IMA log should help detect if IMA log is missing events. How do you account for updates to other PCRs than PCR 10 that a user may use for whatever reason? I think you would have to have the TPM driver count the updates for PCR 10. Form what I can see there's one global PCR update counter for all PCRs and all banks. Also, if we hit an oom condition when logging then the PCR is not extended, which is good since otherwise we would be hopelessly out-of-sync. https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_queue.c#L179 have any information if the PCR mismatch with IMA log is because of loss of entries in the IMA log or something else. TPM 2.0 provides an update counter which is incremented each time a PCR is updated [2]. Measuring the TPM PCR update counter in IMA subsystem will help the remote attestation service to validate if there are any missing entries in the IMA log, when the system goes through certain important state changes (e.g. kexec soft boot, IMA log snapshotting etc.) This patch series provides the required functionality to measure the update counter through IMA subsystem by - - introducing a function to retrieve PCR update counter in the TPM subsystem. - IMA functionality to acquire the update counter from the TPM subsystem. - Measuring the update counter at system boot and at kexec Kernel load. Then the bugs you mentioned above that may happen between system boot and kexec load are still going to confuse anyone looking at the log and quote. I don't think you should mention them. I thought you would provide a way to sync I used the kexec load-execute bug as an example to demonstrate the value of measuring update counter. There could be other examples which I am not aware of. As we discussed above, even when I fix the kexec bug – there is still a possibility that events may go missing in error/OOM cases. Yes, and we're not extending the PCRs then either, which would be catastrophic if we were. I can remove the kexec example if it is causing confusion.> Please let me know. I am not convinced we need this series ... :-( Your kexec series prevents further logging and especially PCR extensions after the frozen measurement log has been created and in ima_add_template_entry(), if we hit an oom condition, then we luckily do not extend the PCR either. If either the log was to have one more entry than number PCR extensions occurred or vice versa, then the remote attestation service will see this mismatch no matter what and all the PCR update counter won't help (and is generally not a good indicator for this purpose imo) for it to recover from this. It's better to declare the system as un-trusted/ corrupted in this case then. Stefan up on every step... I don’t fully understand what you mean by “provide a way to sync up on every step”. Could you please elaborate? Also, I thought you had a variable in your kexec series that would prevent all further logging and measuring once the log had been marshalled during kexec 'exec' stage and this wasn't necessary. No, the variable suspend_ima_measurements[1] suspends the measurements while copying the kexec buffer during kexec execute to ensure consistency of the IMA
Re: [PATCH 0/6] Measuring TPM update counter in IMA
Thanks Stefan for reviewing this series. Appreciate it. Re-sending this email. I accidentally had some HTML content, the email bounced back from integrity mailing list. On 8/3/23 06:37, Stefan Berger wrote: On 8/1/23 14:19, Tushar Sugandhi wrote: Entries in IMA log may be lost due to code bugs, certain error conditions I hope we don't have such bugs. And I guess the most critical ones would be between logging and PCR extensions I hope so too, but in general, it’s hard to prove a negative. That’s why this patch series. :) being met etc. This can result in TPM PCRs getting out of sync with the IMA log. One such example is events between kexec 'load' and kexec 'execute' getting lost from the IMA log when the system soft-boots into the new Kernel using kexec[1]. The remote attestation service does not Though this particular condition I thought would go away with your kexec series. Currently the events in-between kexec ‘load’ and ‘execute’ are always lost – because IMA log is captured at ‘load’. My kexec series addresses this scenario. But as you said, there is always a possibility that the events will still be lost during kexec because of error conditions, OOM etc. The other conditions would be an out-of-memory or TPM failure. The OOM would probably be more critical since something that was supposed to be logged couldn't be logged and now you cannot show this anymore and presumably not even an error condition could be logged. Precisely. As you can see in patch 5 of this series, I am logging the counter at ima_init (ima_init_tpm_update_counter). So we will get the baseline counter at the system boot, where memory pressure is hopefully low. Even if we are not able to log the counter later due to OOM, this baseline counter along with the number of events in the IMA log should help detect if IMA log is missing events. https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_queue.c#L179 have any information if the PCR mismatch with IMA log is because of loss of entries in the IMA log or something else. TPM 2.0 provides an update counter which is incremented each time a PCR is updated [2]. Measuring the TPM PCR update counter in IMA subsystem will help the remote attestation service to validate if there are any missing entries in the IMA log, when the system goes through certain important state changes (e.g. kexec soft boot, IMA log snapshotting etc.) This patch series provides the required functionality to measure the update counter through IMA subsystem by - - introducing a function to retrieve PCR update counter in the TPM subsystem. - IMA functionality to acquire the update counter from the TPM subsystem. - Measuring the update counter at system boot and at kexec Kernel load. Then the bugs you mentioned above that may happen between system boot and kexec load are still going to confuse anyone looking at the log and quote. I don't think you should mention them. I thought you would provide a way to sync I used the kexec load-execute bug as an example to demonstrate the value of measuring update counter. There could be other examples which I am not aware of. As we discussed above, even when I fix the kexec bug – there is still a possibility that events may go missing in error/OOM cases. I can remove the kexec example if it is causing confusion. Please let me know. up on every step... I don’t fully understand what you mean by “provide a way to sync up on every step”. Could you please elaborate? Also, I thought you had a variable in your kexec series that would prevent all further logging and measuring once the log had been marshalled during kexec 'exec' stage and this wasn't necessary. No, the variable suspend_ima_measurements[1] suspends the measurements while copying the kexec buffer during kexec execute to ensure consistency of the IMA measurements. It doesn’t prevent all future logging. The variable is reset and the IMA measurements resume when the system boots into the new Kernel image. [1] https://patchwork.kernel.org/project/linux-integrity/patch/20230703215709.1195644-10-tusha...@linux.microsoft.com/ ~Tushar Stefan This patch series would be a prerequisite for the next version of kexec load/execute series[1] and the future IMA log snapshotting patch series. [1] https://lore.kernel.org/all/20230703215709.1195644-1-tusha...@linux.microsoft.com/ ima: measure events between kexec load and execute [2] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf Section 22.4.2, Page 206. Tushar Sugandhi (6): tpm: implement TPM2 function to get update counter tpm: provide functionality to get update counter ima: get TPM update counter ima: implement functionality to measure TPM update counter ima: measure TPM update counter at ima_init kexec: measure TPM update counter in ima log at kexec load drivers/char/tpm/tpm-interface.c | 28
Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter
On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
> Currently TPM update counter is not available external to the system,
> for instance, a remote attestation service. It is a problem because
> the service cannot easily determine if the IMA log entries are missing.
> The IMA functionality needs to be extended to measure the TPM update
> counter from various subsystems in Linux kernel to help detect if
> the IMA log entries are missing.
>
> Implement a function, 'ima_measure_update_counter()' which would retrieve
> the TPM update counter using the previously defined function
> 'ima_tpm_get_update_counter()'. Format it as a string with the value
> "update_counter=;", and measure it using the function
> 'ima_measure_critical_data()'.
>
> The function takes an event name as input, and the update counter value
> is measured as part of this event.
>
> Signed-off-by: Tushar Sugandhi
Explicit TPM2 quote commands do not return the quoted PCR values or the
pcrCounter value. Define and include a new IMA measurement record
containing the pcrCounter, other TPM info, and IMA info in the IMA
measurement list to help simplify detecting a trimmed/truncated
measurement list.
> ---
> include/linux/ima.h | 1 +
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_main.c | 28
> 3 files changed, 30 insertions(+)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 86b57757c7b1..f15f3a6a4c72 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char
> *event_label,
>const char *event_name,
>const void *buf, size_t buf_len,
>bool hash, u8 *digest, size_t digest_len);
> +int ima_measure_update_counter(const char *event_name);
>
> #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> extern void ima_appraise_parse_cmdline(void);
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 4acd0e5a830f..5484bd362237 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -168,6 +168,7 @@ int __init ima_init_digests(void);
> int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> void *lsm_data);
> int ima_tpm_get_update_counter(u32 *cpu_update_counter);
> +int ima_measure_update_counter(const char *event_name);
>
> /*
> * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index d66a0a36415e..1bcd45cc5a6a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label,
> }
> EXPORT_SYMBOL_GPL(ima_measure_critical_data);
>
> +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128
> +int ima_measure_update_counter(const char *event_name)
> +{
> + int result;
> + u32 update_counter = 0;
> + char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE];
> + int buf_len;
> +
> + if (!event_name)
> + return -ENOPARAM;
> +
> + result = ima_tpm_get_update_counter(_counter);
> +
> + if (result != 0)
> + return result;
> +
> + scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
> + update_counter);
> +
> + buf_len = strlen(buf);
> +
> + result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
> + buf, buf_len, false, NULL, 0);
>
The new record should contain everything needed to verify the
pcrCounter. For example, each IMA measurement record updates the
pcrCounter for each TPM bank enabled. So the number of enabled TPM
banks and number of IMA measurements should also be included in this
record.
Perhaps include a version number as well, so that if we ever want to
include other information, we could.
Mimi
> +
> + return result;
> +}
> +EXPORT_SYMBOL_GPL(ima_measure_update_counter);
> +
> static int __init init_ima(void)
> {
> int error;
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter
On 8/3/23 01:57, Jarkko Sakkinen wrote: On Thu Aug 3, 2023 at 4:22 AM EEST, Mimi Zohar wrote: On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote: From long description I see zero motivation to ack this change, except some heresay about IMA requiring it. Why does IMA need update_cnt and why this is not documented to the long description? The motivation is to detect whether the IMA measurement list has been truncated, for whatever reason. A new IMA record should be defined containing the "pcrCounter" value. (I have not had a chance to review this patch set.) This new record would be a pre-req for both Tushar's "ima: measure events between kexec load and execute" patch set and Sush's proposal to trim the measurement list. (I haven't looked at it yet either.) Please describe the story in a bit more understandable form. In the commit messages it is not good to have some redundancy in patch sets. BR, Jarkko Thanks Jarkko. I had provided the overall context in the cover letter. But I understand the cover letter will be lost when the patches are merged. I will describe the story in the patch descriptions as well, in the next version of this series. ~Tushar ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter
On 8/3/23 01:43, Jarkko Sakkinen wrote: On Thu Aug 3, 2023 at 12:04 AM EEST, Tushar Sugandhi wrote: Btw, the function tpm2_pcr_read is not exposed directly to the other subsystems (like IMA). It is exposed via tpm_pcr_read. Do you want to expose tpm2_pcr_read directly, or do you want me to update the function signature of tpm_pcr_read as well? As long as you mention that 'update_cnt' causes divegence in the in-kernel API, and therefore tpm[12]_pcr_read() cannnot be under the same orchestrator. Yup. I will mention that in the description/comment. If you take this path, please implement a precursory patch, which replace the existing call sites with some sequence of tpm[12]_pcr_read() and tpm_is_tpm2() calls. Sure. I will add a precursory patch which will replace the existing call sites. Thanks for confirming the approach. ~Tushar BR, Jarkko ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter
On 8/2/23 18:22, Mimi Zohar wrote: On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote: From long description I see zero motivation to ack this change, except some heresay about IMA requiring it. Why does IMA need update_cnt and why this is not documented to the long description? The motivation is to detect whether the IMA measurement list has been truncated, for whatever reason. A new IMA record should be defined containing the "pcrCounter" value. (I have not had a chance to review this patch set.) This new record would be a pre-req for both Tushar's "ima: measure events between kexec load and execute" patch set and Sush's proposal to trim the measurement list. (I haven't looked at it yet either.) Thanks Mimi. Take your time. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v25 01/10] drivers/base: refactor cpu.c to use .is_visible()
On Thu, Aug 03, 2023 at 01:20:28PM -0500, Eric DeVolder wrote: > > > On 7/21/23 11:32, Eric DeVolder wrote: > > > > > > On 7/3/23 11:53, Eric DeVolder wrote: > > > > > > > > > On 7/3/23 08:05, Greg KH wrote: > > > > On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: > > > > > - the function body of the callback functions are now wrapped with > > > > > IS_ENABLED(); as the callback function must exist now that the > > > > > attribute is always compiled-in (though not necessarily visible). > > > > > > > > Why do you need to do this last thing? Is it a code savings goal? Or > > > > something else? The file will not be present in the system if the > > > > option is not enabled, so it should be safe to not do this unless you > > > > feel it's necessary for some reason? > > > > > > To accommodate the request, all DEVICE_ATTR() must be > > > unconditionally present in this file. The DEVICE_ATTR() requires the > > > .show() callback. As the callback is referenced from a data > > > structure, the callback has to be present for link. All the > > > callbacks for these attributes are in this file. > > > > > > I have two basic choices for gutting the function body if the config > > > feature is not enabled. I can either use #ifdef or IS_ENABLED(). > > > Thomas has made it clear I need to use IS_ENABLED(). I can certainly > > > use #ifdef (which is what I did in v24). > > > > > > > > > > > Not doing this would make the diff easier to read :) > > > > > > I agree this is messy. I'm not really sure what this request/effort > > > achieves as these attributes are not strongly related (unlike > > > cacheinfo) and the way the file was before results in less code. > > > > > > At any rate, please indicate if you'd rather I use #ifdef. > > > Thanks for your time! > > > eric > > > > > > > > > > > thanks, > > > > > > > > greg k-h > > > > Hi Greg, > > I was wondering if you might weigh-in so that I can proceed. > > > > I think there are three options on the table: > > - use #ifdef to comment out these function bodies, which keeps the diff > > much more readable > > - use IS_ENABLED() as Thomas has requested I do, but makes the diff more > > difficult to read > > - remove this refactor altogether, perhaps post-poning until after this > > crash hotplug series merges, as this refactor is largely unrelated to > > crash hotplug. > > > > Thank you for your time on this topic! > > eric > > Hi Greg, > If you have an opinion on how to proceed, please provide. Sorry, totally swamped by "stuff". I don't know, use your judgement here and send a new version, don't wait for me to weigh in on design decisions for longer than a week. thanks, greg k-h ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v25 01/10] drivers/base: refactor cpu.c to use .is_visible()
On 7/21/23 11:32, Eric DeVolder wrote: On 7/3/23 11:53, Eric DeVolder wrote: On 7/3/23 08:05, Greg KH wrote: On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote: - the function body of the callback functions are now wrapped with IS_ENABLED(); as the callback function must exist now that the attribute is always compiled-in (though not necessarily visible). Why do you need to do this last thing? Is it a code savings goal? Or something else? The file will not be present in the system if the option is not enabled, so it should be safe to not do this unless you feel it's necessary for some reason? To accommodate the request, all DEVICE_ATTR() must be unconditionally present in this file. The DEVICE_ATTR() requires the .show() callback. As the callback is referenced from a data structure, the callback has to be present for link. All the callbacks for these attributes are in this file. I have two basic choices for gutting the function body if the config feature is not enabled. I can either use #ifdef or IS_ENABLED(). Thomas has made it clear I need to use IS_ENABLED(). I can certainly use #ifdef (which is what I did in v24). Not doing this would make the diff easier to read :) I agree this is messy. I'm not really sure what this request/effort achieves as these attributes are not strongly related (unlike cacheinfo) and the way the file was before results in less code. At any rate, please indicate if you'd rather I use #ifdef. Thanks for your time! eric thanks, greg k-h Hi Greg, I was wondering if you might weigh-in so that I can proceed. I think there are three options on the table: - use #ifdef to comment out these function bodies, which keeps the diff much more readable - use IS_ENABLED() as Thomas has requested I do, but makes the diff more difficult to read - remove this refactor altogether, perhaps post-poning until after this crash hotplug series merges, as this refactor is largely unrelated to crash hotplug. Thank you for your time on this topic! eric Hi Greg, If you have an opinion on how to proceed, please provide. Thanks, eric ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 2/3] riscv/purgatory: do not link with string.o and its dependencies
On 8/3/2023 5:13 PM, Palmer Dabbelt wrote:
> On Wed, 26 Jul 2023 09:33:49 PDT (-0700), Conor Dooley wrote:
>> On Wed, Jul 26, 2023 at 11:54:00AM +0200, Petr Tesarik wrote:
>>> From: Petr Tesarik
>>>
>>> Linking with this object file makes kexec_file_load(2) fail because of
>>> multiple unknown relocation types:
>>>
>>> - R_RISCV_ADD16, R_RISCV_SUB16: used by alternatives in strcmp()
>>> - R_RISCV_GOT_HI20: used to resolve _ctype in strcasecmp()
>>>
>>> All this hassle is needed for one single call to memcmp() from
>>> verify_sha256_digest() to compare 32 bytes of SHA256 checksum.
>>>
>>> Simply replace this memcmp() call with an explicit loop over those 32
>>> bytes
>>> and remove the need to link with string.o and all the other object files
>>> that provide undefined symbols from that object file.
>>>
>>> Fixes: 838b3e28488f ("RISC-V: Load purgatory in kexec_file")
>>> Signed-off-by: Petr Tesarik
>>
>> This version keeps the automation happy,
>> Acked-by: Conor Dooley
>
> Oddly enough, this breaks my builds. I tried fixing the first one with
> something like
FTR I have no idea how a patch to the purgatory code and its Makefile
can have any effect on the compilation of asm-offsets.s in another
directory.
Petr T
> From 41c5a952f77e53bf4201296abff0132725aa19e6 Mon Sep 17 00:00:00 2001
> From: Palmer Dabbelt
> Date: Wed, 2 Aug 2023 20:22:33 -0700
> Subject: [PATCH] RISC-V: Include io from timex
> Without this I get some implicit declarations.
> CC arch/riscv/kernel/asm-offsets.s
> In file included from linux/include/linux/timex.h:67,
> from linux/include/linux/time32.h:13,
> from linux/include/linux/time.h:60,
> from linux/include/linux/ktime.h:24,
> from linux/include/linux/timer.h:6,
> from linux/include/linux/workqueue.h:9,
> from linux/include/linux/mm_types.h:19,
> from linux/include/linux/mmzone.h:22,
> from linux/include/linux/gfp.h:7,
> from linux/include/linux/mm.h:7,
> from linux/arch/riscv/kernel/asm-offsets.c:10:
> linux/arch/riscv/include/asm/timex.h: In function 'get_cycles':
> linux/arch/riscv/include/asm/timex.h:25:16: error: implicit
> declaration of function 'readl_relaxed'
> [-Werror=implicit-function-declaration]
> 25 | return readl_relaxed(((u32 *)clint_time_val));
> |
> Signed-off-by: Palmer Dabbelt
> ---
> arch/riscv/include/asm/timex.h | 1 +
> 1 file changed, 1 insertion(+)
> diff --git a/arch/riscv/include/asm/timex.h
> b/arch/riscv/include/asm/timex.h
> index a06697846e69..1a4d181193e0 100644
> --- a/arch/riscv/include/asm/timex.h
> +++ b/arch/riscv/include/asm/timex.h
> @@ -7,6 +7,7 @@
> #define _ASM_RISCV_TIMEX_H
> #include
> +#include
> typedef unsigned long cycles_t;
>
> -- 2.41.0
>
> The other two look fine and are somewhat independent, so I've picked
> those up
> for fixes.
>
> Thanks!
>
>>
>> Thanks,
>> Conor.
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 2/3] riscv/purgatory: do not link with string.o and its dependencies
On Thu, Aug 03, 2023 at 08:13:48AM -0700, Palmer Dabbelt wrote:
> On Wed, 26 Jul 2023 09:33:49 PDT (-0700), Conor Dooley wrote:
> > On Wed, Jul 26, 2023 at 11:54:00AM +0200, Petr Tesarik wrote:
> > > From: Petr Tesarik
> > >
> > > Linking with this object file makes kexec_file_load(2) fail because of
> > > multiple unknown relocation types:
> > >
> > > - R_RISCV_ADD16, R_RISCV_SUB16: used by alternatives in strcmp()
> > > - R_RISCV_GOT_HI20: used to resolve _ctype in strcasecmp()
> > >
> > > All this hassle is needed for one single call to memcmp() from
> > > verify_sha256_digest() to compare 32 bytes of SHA256 checksum.
> > >
> > > Simply replace this memcmp() call with an explicit loop over those 32
> > > bytes
> > > and remove the need to link with string.o and all the other object files
> > > that provide undefined symbols from that object file.
> > >
> > > Fixes: 838b3e28488f ("RISC-V: Load purgatory in kexec_file")
> > > Signed-off-by: Petr Tesarik
> >
> > This version keeps the automation happy,
> > Acked-by: Conor Dooley
>
> Oddly enough, this breaks my builds. I tried fixing the first one with
> something like
You say "tried", does that mean it also didn't work?
What's the config?
Cheers,
Conor.
>
>From 41c5a952f77e53bf4201296abff0132725aa19e6 Mon Sep 17 00:00:00 2001
>From: Palmer Dabbelt
>Date: Wed, 2 Aug 2023 20:22:33 -0700
>Subject: [PATCH] RISC-V: Include io from timex
>Without this I get some implicit declarations.
> CC arch/riscv/kernel/asm-offsets.s
>In file included from linux/include/linux/timex.h:67,
> from linux/include/linux/time32.h:13,
> from linux/include/linux/time.h:60,
> from linux/include/linux/ktime.h:24,
> from linux/include/linux/timer.h:6,
> from linux/include/linux/workqueue.h:9,
> from linux/include/linux/mm_types.h:19,
> from linux/include/linux/mmzone.h:22,
> from linux/include/linux/gfp.h:7,
> from linux/include/linux/mm.h:7,
> from linux/arch/riscv/kernel/asm-offsets.c:10:
>linux/arch/riscv/include/asm/timex.h: In function 'get_cycles':
>linux/arch/riscv/include/asm/timex.h:25:16: error: implicit declaration of
> function 'readl_relaxed' [-Werror=implicit-function-declaration]
> 25 | return readl_relaxed(((u32 *)clint_time_val));
> |
>Signed-off-by: Palmer Dabbelt
>---
> arch/riscv/include/asm/timex.h | 1 +
> 1 file changed, 1 insertion(+)
>diff --git a/arch/riscv/include/asm/timex.h
> b/arch/riscv/include/asm/timex.h
>index a06697846e69..1a4d181193e0 100644
>--- a/arch/riscv/include/asm/timex.h
>+++ b/arch/riscv/include/asm/timex.h
>@@ -7,6 +7,7 @@
> #define _ASM_RISCV_TIMEX_H
> #include
>+#include
> typedef unsigned long cycles_t;
>
>--2.41.0
>
> The other two look fine and are somewhat independent, so I've picked those up
> for fixes.
>
> Thanks!
>
> >
> > Thanks,
> > Conor.
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RESEND PATCH -fixes 1/2] riscv: Export va_kernel_pa_offset in vmcoreinfo
On Mon, 24 Jul 2023 18:09:16 +0800, Song Shuai wrote:
> Since RISC-V Linux v6.4, the commit 3335068f8721 ("riscv: Use
> PUD/P4D/PGD pages for the linear mapping") changes phys_ram_base
> from the physical start of the kernel to the actual start of the DRAM.
>
> The Crash-utility's VTOP() still uses phys_ram_base and kernel_map.virt_addr
> to translate kernel virtual address, that failed the Crash with Linux v6.4
> [1].
>
> [...]
Applied, thanks!
[1/2] riscv: Export va_kernel_pa_offset in vmcoreinfo
https://git.kernel.org/palmer/c/fbe7d19d2b7f
[2/2] Documentation: kdump: Add va_kernel_pa_offset for RISCV64
https://git.kernel.org/palmer/c/640c503d7dbd
Best regards,
--
Palmer Dabbelt
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RESEND PATCH -fixes 1/2] riscv: Export va_kernel_pa_offset in vmcoreinfo
Hello:
This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt :
On Mon, 24 Jul 2023 18:09:16 +0800 you wrote:
> Since RISC-V Linux v6.4, the commit 3335068f8721 ("riscv: Use
> PUD/P4D/PGD pages for the linear mapping") changes phys_ram_base
> from the physical start of the kernel to the actual start of the DRAM.
>
> The Crash-utility's VTOP() still uses phys_ram_base and kernel_map.virt_addr
> to translate kernel virtual address, that failed the Crash with Linux v6.4
> [1].
>
> [...]
Here is the summary with links:
- [RESEND,-fixes,1/2] riscv: Export va_kernel_pa_offset in vmcoreinfo
https://git.kernel.org/riscv/c/fbe7d19d2b7f
- [RESEND,-fixes,2/2] Documentation: kdump: Add va_kernel_pa_offset for
RISCV64
https://git.kernel.org/riscv/c/640c503d7dbd
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 2/3] riscv/purgatory: do not link with string.o and its dependencies
On Wed, 26 Jul 2023 09:33:49 PDT (-0700), Conor Dooley wrote:
On Wed, Jul 26, 2023 at 11:54:00AM +0200, Petr Tesarik wrote:
From: Petr Tesarik
Linking with this object file makes kexec_file_load(2) fail because of
multiple unknown relocation types:
- R_RISCV_ADD16, R_RISCV_SUB16: used by alternatives in strcmp()
- R_RISCV_GOT_HI20: used to resolve _ctype in strcasecmp()
All this hassle is needed for one single call to memcmp() from
verify_sha256_digest() to compare 32 bytes of SHA256 checksum.
Simply replace this memcmp() call with an explicit loop over those 32 bytes
and remove the need to link with string.o and all the other object files
that provide undefined symbols from that object file.
Fixes: 838b3e28488f ("RISC-V: Load purgatory in kexec_file")
Signed-off-by: Petr Tesarik
This version keeps the automation happy,
Acked-by: Conor Dooley
Oddly enough, this breaks my builds. I tried fixing the first one with
something like
From 41c5a952f77e53bf4201296abff0132725aa19e6 Mon Sep 17 00:00:00 2001
From: Palmer Dabbelt
Date: Wed, 2 Aug 2023 20:22:33 -0700
Subject: [PATCH] RISC-V: Include io from timex
Without this I get some implicit declarations.
CC arch/riscv/kernel/asm-offsets.s
In file included from linux/include/linux/timex.h:67,
from linux/include/linux/time32.h:13,
from linux/include/linux/time.h:60,
from linux/include/linux/ktime.h:24,
from linux/include/linux/timer.h:6,
from linux/include/linux/workqueue.h:9,
from linux/include/linux/mm_types.h:19,
from linux/include/linux/mmzone.h:22,
from linux/include/linux/gfp.h:7,
from linux/include/linux/mm.h:7,
from linux/arch/riscv/kernel/asm-offsets.c:10:
linux/arch/riscv/include/asm/timex.h: In function 'get_cycles':
linux/arch/riscv/include/asm/timex.h:25:16: error: implicit declaration of
function 'readl_relaxed' [-Werror=implicit-function-declaration]
25 | return readl_relaxed(((u32 *)clint_time_val));
|
Signed-off-by: Palmer Dabbelt
---
arch/riscv/include/asm/timex.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h
index a06697846e69..1a4d181193e0 100644
--- a/arch/riscv/include/asm/timex.h
+++ b/arch/riscv/include/asm/timex.h
@@ -7,6 +7,7 @@
#define _ASM_RISCV_TIMEX_H
#include
+#include
typedef unsigned long cycles_t;
--
2.41.0
The other two look fine and are somewhat independent, so I've picked those up
for fixes.
Thanks!
Thanks,
Conor.
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 0/3] RISC-V: Fix a few kexec_file_load(2) failures
Hello: This series was applied to riscv/linux.git (fixes) by Palmer Dabbelt : On Wed, 26 Jul 2023 11:53:58 +0200 you wrote: > From: Petr Tesarik > > The kexec_file_load(2) syscall does not work at least in some kernel > builds. For details see the relevant section in this blog post: > > https://sigillatum.tesarici.cz/2023-07-21-state-of-riscv64-kdump.html > > [...] Here is the summary with links: - [v2,1/3] riscv/kexec: handle R_RISCV_CALL_PLT relocation type https://git.kernel.org/riscv/c/1be0b05b3a80 - [v2,2/3] riscv/purgatory: do not link with string.o and its dependencies (no matching commit) - [v2,3/3] riscv/kexec: load initrd high in available memory https://git.kernel.org/riscv/c/0ccd2e803745 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for kexec kernel
On Thu, 3 Aug 2023 at 13:11, Ard Biesheuvel wrote: > > On Wed, 2 Aug 2023 at 17:52, Borislav Petkov wrote: > > > > On Wed, Aug 02, 2023 at 04:55:27PM +0200, Ard Biesheuvel wrote: > > > ... because now, entering via startup_32 is broken, given that it only > > > maps the kernel image itself and relies on the #PF handling for > > > everything else it accesses, including firmware tables. > > > > > > AFAICT this also means that entering via startup_32 is broken entirely > > > for any configuration that enables the cc blob config table check, > > > regardless of the platform. > > > > Lemme brain-dump what Tom and I just talked on IRC. > > > > That startup_32 entry path for SNP guests was used with old grubs which > > used to enter through there and not anymore, reportedly. Which means, > > that must've worked at some point but Joerg would know. CCed. > > > > Sadly, not only 'old' grubs - GRUB mainline only recently added > support for booting Linux/x86 via the EFI stub (because I wrote the > code for them), but it will still fall back to the previous mode for > kernels that are built without EFI stub support, or which are older > than ~v5.8 (because their EFI stub does not implement the generic EFI > initrd loading mechanism) > > This fallback still appears to enter via startup_32, even when GRUB > itself runs in long mode in the context of EFI. > > > Newer grubs enter through the 64-bit entry point and thus are fine > > - otherwise we would be seeing explosions left and right. > > > > Yeah. what seems to be saving our ass here is that startup_32 maps the > first 1G of physical address space 4 times, and x86_64 EFI usually > puts firmware tables below 4G. This means the cc blob check doesn't > fault, but it may dereference bogus memory traversing the config table > array looking for the cc blob GUID. However, the system table field > holding the size of the array may also appear as bogus so this may > still break in weird ways. > > > So dependent on what we wanna do, if we kill the 32-bit path, we can > > kill the 32-bit C-bit verif code. But that's for later and an item on my > > TODO list. > > > > I don't think we can kill it yet, but it would be nice if we could > avoid the need to support SNP boot when entering that way. https://lists.gnu.org/archive/html/grub-devel/2023-08/msg5.html Coming to your distro any decade now! ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/6] Measuring TPM update counter in IMA
On 8/1/23 14:19, Tushar Sugandhi wrote: Entries in IMA log may be lost due to code bugs, certain error conditions I hope we don't have such bugs. And I guess the most critical ones would be between logging and PCR extensions being met etc. This can result in TPM PCRs getting out of sync with the IMA log. One such example is events between kexec 'load' and kexec 'execute' getting lost from the IMA log when the system soft-boots into the new Kernel using kexec[1]. The remote attestation service does not Though this particular condition I thought would go away with your kexec series. The other conditions would be an out-of-memory or TPM failure. The OOM would probably be more critical since something that was supposed to be logged couldn't be logged and now you cannot show this anymore and presumably not even an error condition could be logged. https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_queue.c#L179 have any information if the PCR mismatch with IMA log is because of loss of entries in the IMA log or something else. TPM 2.0 provides an update counter which is incremented each time a PCR is updated [2]. Measuring the TPM PCR update counter in IMA subsystem will help the remote attestation service to validate if there are any missing entries in the IMA log, when the system goes through certain important state changes (e.g. kexec soft boot, IMA log snapshotting etc.) This patch series provides the required functionality to measure the update counter through IMA subsystem by - - introducing a function to retrieve PCR update counter in the TPM subsystem. - IMA functionality to acquire the update counter from the TPM subsystem. - Measuring the update counter at system boot and at kexec Kernel load. Then the bugs you mentioned above that may happen between system boot and kexec load are still going to confuse anyone looking at the log and quote. I don't think you should mention them. I thought you would provide a way to sync up on every step... Also, I thought you had a variable in your kexec series that would prevent all further logging and measuring once the log had been marshalled during kexec 'exec' stage and this wasn't necessary. Stefan This patch series would be a prerequisite for the next version of kexec load/execute series[1] and the future IMA log snapshotting patch series. [1] https://lore.kernel.org/all/20230703215709.1195644-1-tusha...@linux.microsoft.com/ ima: measure events between kexec load and execute [2] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf Section 22.4.2, Page 206. Tushar Sugandhi (6): tpm: implement TPM2 function to get update counter tpm: provide functionality to get update counter ima: get TPM update counter ima: implement functionality to measure TPM update counter ima: measure TPM update counter at ima_init kexec: measure TPM update counter in ima log at kexec load drivers/char/tpm/tpm-interface.c | 28 + drivers/char/tpm/tpm.h | 3 ++ drivers/char/tpm/tpm2-cmd.c| 48 ++ include/linux/ima.h| 1 + include/linux/tpm.h| 8 + kernel/kexec_file.c| 3 ++ security/integrity/ima/ima.h | 2 ++ security/integrity/ima/ima_init.c | 3 ++ security/integrity/ima/ima_main.c | 29 ++ security/integrity/ima/ima_queue.c | 16 ++ 10 files changed, 141 insertions(+) ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for kexec kernel
On Wed, 2 Aug 2023 at 17:52, Borislav Petkov wrote: > > On Wed, Aug 02, 2023 at 04:55:27PM +0200, Ard Biesheuvel wrote: > > ... because now, entering via startup_32 is broken, given that it only > > maps the kernel image itself and relies on the #PF handling for > > everything else it accesses, including firmware tables. > > > > AFAICT this also means that entering via startup_32 is broken entirely > > for any configuration that enables the cc blob config table check, > > regardless of the platform. > > Lemme brain-dump what Tom and I just talked on IRC. > > That startup_32 entry path for SNP guests was used with old grubs which > used to enter through there and not anymore, reportedly. Which means, > that must've worked at some point but Joerg would know. CCed. > Sadly, not only 'old' grubs - GRUB mainline only recently added support for booting Linux/x86 via the EFI stub (because I wrote the code for them), but it will still fall back to the previous mode for kernels that are built without EFI stub support, or which are older than ~v5.8 (because their EFI stub does not implement the generic EFI initrd loading mechanism) This fallback still appears to enter via startup_32, even when GRUB itself runs in long mode in the context of EFI. > Newer grubs enter through the 64-bit entry point and thus are fine > - otherwise we would be seeing explosions left and right. > Yeah. what seems to be saving our ass here is that startup_32 maps the first 1G of physical address space 4 times, and x86_64 EFI usually puts firmware tables below 4G. This means the cc blob check doesn't fault, but it may dereference bogus memory traversing the config table array looking for the cc blob GUID. However, the system table field holding the size of the array may also appear as bogus so this may still break in weird ways. > So dependent on what we wanna do, if we kill the 32-bit path, we can > kill the 32-bit C-bit verif code. But that's for later and an item on my > TODO list. > I don't think we can kill it yet, but it would be nice if we could avoid the need to support SNP boot when entering that way. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter
On Thu Aug 3, 2023 at 4:22 AM EEST, Mimi Zohar wrote: > On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote: > > > > From long description I see zero motivation to ack this change, except > > some heresay about IMA requiring it. Why does IMA need update_cnt and > > why this is not documented to the long description? > > The motivation is to detect whether the IMA measurement list has been > truncated, for whatever reason. A new IMA record should be defined > containing the "pcrCounter" value. (I have not had a chance to review > this patch set.) > > This new record would be a pre-req for both Tushar's "ima: measure > events between kexec load and execute" patch set and Sush's proposal to > trim the measurement list. (I haven't looked at it yet either.) Please describe the story in a bit more understandable form. In the commit messages it is not good to have some redundancy in patch sets. BR, Jarkko ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC][PATCH] iwlwifi: Add pci .shutdown() hook for iwlwifi driver
Add kexec to the CC list so kexec people can know this.
On Thu, Aug 3, 2023 at 10:55 AM Tao Liu wrote:
>
> Previously no .shutdown() hook is implemented for iwlwifi driver, a
> ETIMEDOUT error will occur during the kexec kernel bootup. As a
> consequence, wifi is unusable after kexec into the new kernel.
>
> This issue is observed and patch tested on the following wireless cards:
>
> 1) Network controller: Intel Corporation Comet Lake PCH-LP CNVi WiFi,
>Subsystem: Intel Corporation Wi-Fi 6 AX201 160MHz
> 2) Network controller: Intel Corporation Wireless-AC 9260,
>Subsystem: Intel Corporation Device e014
>
> Signed-off-by: Tao Liu
> ---
>
> Hi folks,
>
> This is a RFC patch and I'm not sure about the correctness of the code,
> especially about the pci_clear_master() part. What I want is to stop any
> ongoing DMA access, in case if the memory overwritting during kexec
> kernel bootup. But there is already pci_clear_master(pci_dev) in
> drivers/pci/pci-driver.c:pci_device_shutdown(), so I'm not sure if it is
> still needed in the driver side. And I only tested the patch against the
> above 2 wireless cards and worked OK, not sure if it can work for others.
> Please review the patch, thanks in advance!
>
> Thanks,
> Tao Liu
>
> ---
> drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> index 73c1fb3c0c5e..24c4c2dd7cb0 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> @@ -1513,6 +1513,18 @@ static void iwl_pci_remove(struct pci_dev *pdev)
> iwl_trans_pcie_free(trans);
> }
>
> +static void iwl_pci_shutdown(struct pci_dev *pdev)
> +{
> + struct iwl_trans *trans = pci_get_drvdata(pdev);
> +
> + if (!trans)
> + return;
> +
> + iwl_drv_stop(trans->drv);
> +
> + pci_clear_master(pdev);
> +}
> +
> #ifdef CONFIG_PM_SLEEP
>
> static int iwl_pci_suspend(struct device *device)
> @@ -1583,6 +1595,7 @@ static struct pci_driver iwl_pci_driver = {
> .id_table = iwl_hw_card_ids,
> .probe = iwl_pci_probe,
> .remove = iwl_pci_remove,
> + .shutdown = iwl_pci_shutdown,
> .driver.pm = IWL_PM_OPS,
> };
>
> --
> 2.40.1
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter
On Thu Aug 3, 2023 at 12:04 AM EEST, Tushar Sugandhi wrote: > Btw, the function tpm2_pcr_read is not exposed directly to the other > subsystems (like IMA). It is exposed via tpm_pcr_read. > > Do you want to expose tpm2_pcr_read directly, > or do you want me to update the function signature of tpm_pcr_read as well? As long as you mention that 'update_cnt' causes divegence in the in-kernel API, and therefore tpm[12]_pcr_read() cannnot be under the same orchestrator. If you take this path, please implement a precursory patch, which replace the existing call sites with some sequence of tpm[12]_pcr_read() and tpm_is_tpm2() calls. BR, Jarkko ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCHv6 3/5] kexec/zboot: Add arch independent zboot support
On Thu, Aug 03, 2023 at 10:37:10AM +0800, Pingfan Liu wrote: > On Wed, Aug 2, 2023 at 8:33 PM Simon Horman wrote: > > > > On Wed, Aug 02, 2023 at 02:17:57PM +0200, Simon Horman wrote: > > > On Wed, Aug 02, 2023 at 02:16:33PM +0200, Simon Horman wrote: > > > > On Wed, Aug 02, 2023 at 05:53:59PM +0800, Pingfan Liu wrote: > > > > > Hi Simon, > > > > > > > > > > Thanks for the try. Please see the comment below. > > > > > > > > > > On Tue, Aug 1, 2023 at 3:00 PM Simon Horman wrote: > > > > > > > > > > > > On Mon, Jul 24, 2023 at 10:21:40AM +0800, Pingfan Liu wrote: > > > > > > > From: Jeremy Linton > > > > > > > > > > > > > > The linux kernel CONFIG_ZBOOT option creates > > > > > > > self decompressing PE kernel images. So this means > > > > > > > that kexec should have a generic understanding of > > > > > > > the format which may be used by multiple arches. > > > > > > > > > > > > > > So lets add an arch independent validation > > > > > > > and decompression routine. > > > > > > > > > > > > > > Signed-off-by: Jeremy Linton > > > > > > > [Modified by Pingfan to export kernel fd] > > > > > > > Signed-off-by: Pingfan Liu > > > > > > > > > > > > Hi Pingfan, > > > > > > > > > > > > unfortunately this causes a build failure on hppa. > > > > > > > > > > > > ../../kexec/kexec-pe-zboot.c:31:10: fatal error: kexec-pe-zboot.h: > > > > > > No such file or directory > > > > > >31 | #include > > > > > > | ^~ > > > > > > > > > > > > Link: > > > > > > https://github.com/horms/kexec-tools/actions/runs/5723580523/job/15508425790 > > > > > > > > > > > > > > > > It is not related to cross-compiling. Actually, I have tried to > > > > > simplify the test matrix, which limits the compilation only on x86_64. > > > > > And I got the similar error [1] > > > > > > > > > > The workflow control file is [2], which clips out all arches except > > > > > x86_64. But I can successfully build it on the Fedora system with the > > > > > following bash script, which is based on the github's build log. So > > > > > maybe it is a bug with the compiling tools? > > > > > > > > > > kexec_tools_dir="./" > > > > > > > > > > mkdir $kexec_tools_dir/_build \ > > > > > $kexec_tools_dir/_build/sub \ > > > > > $kexec_tools_dir/_inst \ > > > > > $kexec_tools_dir/_dest > > > > > chmod a-w $kexec_tools_dir > > > > > test -d $kexec_tools_dir/_build > > > > > INSTALL_BASE=$(cd $kexec_tools_dir/_inst && pwd | sed -e > > > > > 's,^[^:\\/]:[\\/],/,') &&\ > > > > > DESTDIR="$kexec_tools_dir/_dest" && \ > > > > > cd $kexec_tools_dir/_build/sub && \ > > > > > ../../configure \ > > > > > \ > > > > > --srcdir=../.. --prefix="$INSTALL_BASE" && \ > > > > > make -j8 > > > > > > > > > > > > > > > [1]: > > > > > https://github.com/pfliu/kexec-tools/actions/runs/5737254109/job/15548520863 > > > > > [2]: > > > > > https://github.com/pfliu/kexec-tools/blob/zbootV6/.github/workflows/main.yml > > > > > > > > Thanks, > > > > > > > > I guess that kexec-pe-zboot.h is missing in the build environment for > > > > the > > > > GitHub actions, but present in your Fedora environment. > > > > > > > > Could you take a look and see where your copy of kexec-pe-zboot.h > > > > came from? > > > > > > Actually it seems to be added by this patch (sorry for not noticing)! > > > So I guess it is an include path problem. > > > > I think I have found the problem. > > The kexec-tools build system is a bit unusual, > > and the new file, kexec-pe-zboot.h, was not included in distribution > > tarballs. Thus the build failures. > > > > Unexpectedly, and thank for your insight. > > > I think you can resolve that by squashing the following into this patch. > > > > diff --git a/include/Makefile b/include/Makefile > > --- a/include/Makefile > > +++ b/include/Makefile > > @@ -1,6 +1,7 @@ > > dist += include/Makefile \ > > include/config.h\ > > include/config.h.in \ > > + include/kexec-pe-zboot.h\ > > include/kexec-uImage.h \ > > include/x86/x86-linux.h \ > > include/x86/mb_info.h \ > > > > After applying this patch, the github workflow successfully finished. > I will send out V7 immediately. > > Appreciate for your kind help again. Thanks, I see v7 now. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
