Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31548 )
Change subject: security: Add memory subfolder ......................................................................
Patch Set 5:
(7 comments)
https://review.coreboot.org/#/c/31548/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31548/3//COMMIT_MSG@17 PS3, Line 17: TEE
What is that?
explained
https://review.coreboot.org/#/c/31548/3/Documentation/security/memory_cleari... File Documentation/security/memory_clearing.md:
https://review.coreboot.org/#/c/31548/3/Documentation/security/memory_cleari... PS3, Line 19: 2. Code that is placed in DRAM might be skipped (FSP1.0) : 3. Stack that is placed in DRAM might be skipped (FSP1.0)
Maybe highlight that these are workarounds instead of […]
Done
https://review.coreboot.org/#/c/31548/3/Documentation/security/memory_cleari... PS3, Line 37: As those regions are written on early boot and marked reserved from : OS point of view, it is unlikely that they contain secrets. :
Maybe keep it to technical/implementation details. I don't […]
Done
https://review.coreboot.org/#/c/31548/3/src/security/memory/Kconfig File src/security/memory/Kconfig:
https://review.coreboot.org/#/c/31548/3/src/security/memory/Kconfig@3 PS3, Line 3: ## Copyright (C) 2019 Facebook Inc. : ## Copyright (C) 2019 9elements Agency GmbH
Would be nice to split this into two patches to make […]
That's difficult as I've split the original proof of concept code into smaller patches, not taking care of the authors. If you don't beat me to I'd leave it as is.
https://review.coreboot.org/#/c/31548/3/src/security/memory/memory.h File src/security/memory/memory.h:
https://review.coreboot.org/#/c/31548/3/src/security/memory/memory.h@4 PS3, Line 4: * Copyright (C) 2019 9elements Agency GmbH : * Copyright (C) 2019 Facebook Inc.
This might be a little hard to differentiate.
See other copyright comment
https://review.coreboot.org/#/c/31548/3/src/security/memory/memory.h@17 PS3, Line 17: #include <stddef.h>
<stdbool.h> maybe? But we don't have it. This is covered […]
Done
https://review.coreboot.org/#/c/31548/3/src/security/memory/memory.c File src/security/memory/memory.c:
https://review.coreboot.org/#/c/31548/3/src/security/memory/memory.c@22 PS3, Line 22: USER
why all caps?
Done
-
Patrick Rudolph (Code Review)