| Message ID | 028fc34e-8a39-b0b5-34bb-2c96a82fa452@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: follow-on to XSA-408 | expand |
On Tue, Dec 6, 2022 at 8:57 AM Jan Beulich <jbeulich@suse.com> wrote: > > Especially with our use of __builtin_memset() to implement memset() the > compiler is free to eliminate instances when it can prove that the > affected object is dead. Introduce a small helper function accompanying > the memset() with a construct forcing the compiler to retain the > clearing of (stack) memory. > > Fixes: c021c95498d9 ("x86: Replace our own specialised versions of memset and memcpy with") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
On 06/12/2022 13:57, Jan Beulich wrote: > Especially with our use of __builtin_memset() to implement memset() the > compiler is free to eliminate instances when it can prove that the > affected object is dead. Introduce a small helper function accompanying > the memset() with a construct forcing the compiler to retain the > clearing of (stack) memory. > > Fixes: c021c95498d9 ("x86: Replace our own specialised versions of memset and memcpy with") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Thanks. I'd noticed this before wanted to do something about it. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c @@ -179,6 +179,17 @@ static void update_iommu_mac(vmac_ctx_t #define is_page_in_use(page) \ (page_state_is(page, inuse) || page_state_is(page, offlining)) +/* Wipe ctx to ensure key is not left in memory. */ +static void wipe_ctx(vmac_ctx_t *ctx) +{ + memset(ctx, 0, sizeof(*ctx)); + /* + * Make sure the compiler won't optimize out the memset(), for the local + * variable (at the call sites) going out of scope right afterwards. + */ + asm volatile ( "" :: "m" (*ctx) ); +} + static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE], vmac_t *mac) { @@ -216,8 +227,7 @@ static void tboot_gen_domain_integrity(c *mac = vmac(NULL, 0, nonce, NULL, &ctx); - /* wipe ctx to ensure key is not left in memory */ - memset(&ctx, 0, sizeof(ctx)); + wipe_ctx(&ctx); } /* @@ -278,8 +288,7 @@ static void tboot_gen_xenheap_integrity( } *mac = vmac(NULL, 0, nonce, NULL, &ctx); - /* wipe ctx to ensure key is not left in memory */ - memset(&ctx, 0, sizeof(ctx)); + wipe_ctx(&ctx); } static void tboot_gen_frametable_integrity(const uint8_t key[TB_KEY_SIZE], @@ -307,8 +316,7 @@ static void tboot_gen_frametable_integri *mac = vmac(NULL, 0, nonce, NULL, &ctx); - /* wipe ctx to ensure key is not left in memory */ - memset(&ctx, 0, sizeof(ctx)); + wipe_ctx(&ctx); } void tboot_shutdown(uint32_t shutdown_type)
Especially with our use of __builtin_memset() to implement memset() the compiler is free to eliminate instances when it can prove that the affected object is dead. Introduce a small helper function accompanying the memset() with a construct forcing the compiler to retain the clearing of (stack) memory. Fixes: c021c95498d9 ("x86: Replace our own specialised versions of memset and memcpy with") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- The Fixes: tag names the commit which broke the wrong assumption made by 6deab1ae316b ("txt: perform per-domain (and frametable and xenheap) MAC on entry into").