Re: [PATCH v6 05/14] x86: Secure Launch main header file
On 5/12/23 9:10 AM, Ross Philipson wrote:
On 5/12/23 07:00, Matthew Garrett wrote:
On Thu, May 04, 2023 at 02:50:14PM +, Ross Philipson wrote:
+static inline int tpm12_log_event(void *evtlog_base, u32 evtlog_size,
+ u32 event_size, void *event)
+{
+ struct tpm12_event_log_header *evtlog =
+ (struct tpm12_event_log_header *)evtlog_base;
+
+ if (memcmp(evtlog->signature, TPM12_EVTLOG_SIGNATURE,
+ sizeof(TPM12_EVTLOG_SIGNATURE)))
+ return -EINVAL;
+
+ if (evtlog->container_size > evtlog_size)
+ return -EINVAL;
+
+ if (evtlog->next_event_offset + event_size >
evtlog->container_size)
+ return -E2BIG;
+
+ memcpy(evtlog_base + evtlog->next_event_offset, event, event_size);
+ evtlog->next_event_offset += event_size;
+
+ return 0;
+}
+
+static inline int tpm20_log_event(struct
txt_heap_event_log_pointer2_1_element *elem,
+ void *evtlog_base, u32 evtlog_size,
+ u32 event_size, void *event)
+{
+ struct tcg_pcr_event *header =
+ (struct tcg_pcr_event *)evtlog_base;
+
+ /* Has to be at least big enough for the signature */
+ if (header->event_size < sizeof(TCG_SPECID_SIG))
+ return -EINVAL;
+
+ if (memcmp((u8 *)header + sizeof(struct tcg_pcr_event),
+ TCG_SPECID_SIG, sizeof(TCG_SPECID_SIG)))
+ return -EINVAL;
+
+ if (elem->allocated_event_container_size > evtlog_size)
+ return -EINVAL;
+
+ if (elem->next_record_offset + event_size >
+ elem->allocated_event_container_size)
+ return -E2BIG;
+
+ memcpy(evtlog_base + elem->next_record_offset, event, event_size);
+ elem->next_record_offset += event_size;
+
+ return 0;
+}
+
These seem like they'd potentially be useful outside the context of SL,
maybe put them in a more generic location? Very much a nice to have, not
a blocker from my side.
Yea we can look into finding a nice home somewhere in the TPM event log
code for these.
After looking at it, it seems we would have to drag a whole bunch of TXT
related structures into the TPM event log code. I don't think this is
really worth it for what these functions do.
Thanks
Ross
+/*
+ * External functions avalailable in mainline kernel.
Nit: "available"
Ack
Thanks
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 05/14] x86: Secure Launch main header file
On 5/12/23 07:00, Matthew Garrett wrote:
On Thu, May 04, 2023 at 02:50:14PM +, Ross Philipson wrote:
+static inline int tpm12_log_event(void *evtlog_base, u32 evtlog_size,
+ u32 event_size, void *event)
+{
+ struct tpm12_event_log_header *evtlog =
+ (struct tpm12_event_log_header *)evtlog_base;
+
+ if (memcmp(evtlog->signature, TPM12_EVTLOG_SIGNATURE,
+ sizeof(TPM12_EVTLOG_SIGNATURE)))
+ return -EINVAL;
+
+ if (evtlog->container_size > evtlog_size)
+ return -EINVAL;
+
+ if (evtlog->next_event_offset + event_size > evtlog->container_size)
+ return -E2BIG;
+
+ memcpy(evtlog_base + evtlog->next_event_offset, event, event_size);
+ evtlog->next_event_offset += event_size;
+
+ return 0;
+}
+
+static inline int tpm20_log_event(struct txt_heap_event_log_pointer2_1_element
*elem,
+ void *evtlog_base, u32 evtlog_size,
+ u32 event_size, void *event)
+{
+ struct tcg_pcr_event *header =
+ (struct tcg_pcr_event *)evtlog_base;
+
+ /* Has to be at least big enough for the signature */
+ if (header->event_size < sizeof(TCG_SPECID_SIG))
+ return -EINVAL;
+
+ if (memcmp((u8 *)header + sizeof(struct tcg_pcr_event),
+ TCG_SPECID_SIG, sizeof(TCG_SPECID_SIG)))
+ return -EINVAL;
+
+ if (elem->allocated_event_container_size > evtlog_size)
+ return -EINVAL;
+
+ if (elem->next_record_offset + event_size >
+ elem->allocated_event_container_size)
+ return -E2BIG;
+
+ memcpy(evtlog_base + elem->next_record_offset, event, event_size);
+ elem->next_record_offset += event_size;
+
+ return 0;
+}
+
These seem like they'd potentially be useful outside the context of SL,
maybe put them in a more generic location? Very much a nice to have, not
a blocker from my side.
Yea we can look into finding a nice home somewhere in the TPM event log
code for these.
+/*
+ * External functions avalailable in mainline kernel.
Nit: "available"
Ack
Thanks
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 05/14] x86: Secure Launch main header file
On Thu, May 04, 2023 at 02:50:14PM +, Ross Philipson wrote:
> +static inline int tpm12_log_event(void *evtlog_base, u32 evtlog_size,
> + u32 event_size, void *event)
> +{
> + struct tpm12_event_log_header *evtlog =
> + (struct tpm12_event_log_header *)evtlog_base;
> +
> + if (memcmp(evtlog->signature, TPM12_EVTLOG_SIGNATURE,
> +sizeof(TPM12_EVTLOG_SIGNATURE)))
> + return -EINVAL;
> +
> + if (evtlog->container_size > evtlog_size)
> + return -EINVAL;
> +
> + if (evtlog->next_event_offset + event_size > evtlog->container_size)
> + return -E2BIG;
> +
> + memcpy(evtlog_base + evtlog->next_event_offset, event, event_size);
> + evtlog->next_event_offset += event_size;
> +
> + return 0;
> +}
> +
> +static inline int tpm20_log_event(struct
> txt_heap_event_log_pointer2_1_element *elem,
> + void *evtlog_base, u32 evtlog_size,
> + u32 event_size, void *event)
> +{
> + struct tcg_pcr_event *header =
> + (struct tcg_pcr_event *)evtlog_base;
> +
> + /* Has to be at least big enough for the signature */
> + if (header->event_size < sizeof(TCG_SPECID_SIG))
> + return -EINVAL;
> +
> + if (memcmp((u8 *)header + sizeof(struct tcg_pcr_event),
> +TCG_SPECID_SIG, sizeof(TCG_SPECID_SIG)))
> + return -EINVAL;
> +
> + if (elem->allocated_event_container_size > evtlog_size)
> + return -EINVAL;
> +
> + if (elem->next_record_offset + event_size >
> + elem->allocated_event_container_size)
> + return -E2BIG;
> +
> + memcpy(evtlog_base + elem->next_record_offset, event, event_size);
> + elem->next_record_offset += event_size;
> +
> + return 0;
> +}
> +
These seem like they'd potentially be useful outside the context of SL,
maybe put them in a more generic location? Very much a nice to have, not
a blocker from my side.
> +/*
> + * External functions avalailable in mainline kernel.
Nit: "available"
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 05/14] x86: Secure Launch main header file
On 5/5/23 12:25, Simon Horman wrote:
On Thu, May 04, 2023 at 02:50:14PM +, Ross Philipson wrote:
Introduce the main Secure Launch header file used in the early SL stub
and the early setup code.
Signed-off-by: Ross Philipson
---
include/linux/slaunch.h | 513
1 file changed, 513 insertions(+)
create mode 100644 include/linux/slaunch.h
diff --git a/include/linux/slaunch.h b/include/linux/slaunch.h
...
+/* TXTCR_STS status bits */
+#define TXT_SENTER_DONE_STS(1<<0)
+#define TXT_SEXIT_DONE_STS (1<<1)
nit: Please consider using BIT()
Ack
...
+/*
+ * External functions avalailable in mainline kernel.
+ */
+extern void slaunch_setup_txt(void);
+extern u32 slaunch_get_flags(void);
+extern struct sl_ap_wake_info *slaunch_get_ap_wake_info(void);
+extern struct acpi_table_header *slaunch_get_dmar_table(struct
acpi_table_header *dmar);
+extern void __noreturn slaunch_txt_reset(void __iomem *txt,
+const char *msg, u64 error);
+extern void slaunch_finalize(int do_sexit);
I think that extern should be avoided.
Perhaps these are in a header file that can be included?
Someone in an earlier review asked me to add the externs. The function
are not in another header, they are in a C module.
+
+#endif /* !__ASSEMBLY */
+
+#else
+
+#define slaunch_setup_txt()do { } while (0)
+#define slaunch_get_flags()0
+#define slaunch_get_dmar_table(d) (d)
+#define slaunch_finalize(d)do { } while (0)
I think it is usual to use static functions for this purpose.
Usually they end up in header files as static inline functions.
Yea we can switch to using static functions.
Thanks
Ross
+
+#endif /* !IS_ENABLED(CONFIG_SECURE_LAUNCH) */
+
+#endif /* _LINUX_SLAUNCH_H */
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 05/14] x86: Secure Launch main header file
On Thu, May 04, 2023 at 02:50:14PM +, Ross Philipson wrote:
> Introduce the main Secure Launch header file used in the early SL stub
> and the early setup code.
>
> Signed-off-by: Ross Philipson
> ---
> include/linux/slaunch.h | 513
>
> 1 file changed, 513 insertions(+)
> create mode 100644 include/linux/slaunch.h
>
> diff --git a/include/linux/slaunch.h b/include/linux/slaunch.h
...
> +/* TXTCR_STS status bits */
> +#define TXT_SENTER_DONE_STS (1<<0)
> +#define TXT_SEXIT_DONE_STS (1<<1)
nit: Please consider using BIT()
...
> +/*
> + * External functions avalailable in mainline kernel.
> + */
> +extern void slaunch_setup_txt(void);
> +extern u32 slaunch_get_flags(void);
> +extern struct sl_ap_wake_info *slaunch_get_ap_wake_info(void);
> +extern struct acpi_table_header *slaunch_get_dmar_table(struct
> acpi_table_header *dmar);
> +extern void __noreturn slaunch_txt_reset(void __iomem *txt,
> + const char *msg, u64 error);
> +extern void slaunch_finalize(int do_sexit);
I think that extern should be avoided.
Perhaps these are in a header file that can be included?
> +
> +#endif /* !__ASSEMBLY */
> +
> +#else
> +
> +#define slaunch_setup_txt() do { } while (0)
> +#define slaunch_get_flags() 0
> +#define slaunch_get_dmar_table(d)(d)
> +#define slaunch_finalize(d) do { } while (0)
I think it is usual to use static functions for this purpose.
Usually they end up in header files as static inline functions.
> +
> +#endif /* !IS_ENABLED(CONFIG_SECURE_LAUNCH) */
> +
> +#endif /* _LINUX_SLAUNCH_H */
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
