Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31548 )
Change subject: security: Add memory subfolder ......................................................................
Patch Set 4: Code-Review+1
(7 comments)
Just documentation comments and some bikeshedding.
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?
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 mentioning FSP.
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 think this justification is of any value as coreboot docu- mentation.
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 clear which is which.
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.
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 up by the <stdint.h> in `memory.c`, because we have `bool` defined in there. If you prefer to use standard headers, please just add a `stdbool.h`.
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?
-
Nico Huber (Code Review)