Hello Joel Kitching,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37261
to review the following change.
Change subject: security/vboot: Use persistent context to read GBB flags ......................................................................
security/vboot: Use persistent context to read GBB flags
With the persistent vboot context coreboot no longer needs to read GBB flags from flash itself -- it can just ask vboot for the cached result. This patch removes the existing GBB code and provides gbb_is_flag_set() (with a slightly better namespaced name) as a static inline instead.
Change-Id: Ibc3ed0f3fbeb53d630925d47df4dc474b0ed07ee Signed-off-by: Julius Werner jwerner@chromium.org --- D src/security/vboot/gbb.c D src/security/vboot/gbb.h M src/security/vboot/misc.h M src/security/vboot/vboot_common.c 4 files changed, 13 insertions(+), 121 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/37261/1
diff --git a/src/security/vboot/gbb.c b/src/security/vboot/gbb.c deleted file mode 100644 index 5293033..0000000 --- a/src/security/vboot/gbb.c +++ /dev/null @@ -1,80 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright 2018 Google LLC - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#define NEED_VB20_INTERNALS /* Peeking into vb2_gbb_header */ - -#include <commonlib/region.h> -#include <console/console.h> -#include <fmap.h> -#include <security/vboot/gbb.h> -#include <string.h> -#include <vb2_api.h> - -#define GBB_FMAP_REGION_NAME "GBB" - -/* Copy of GBB header read from boot media. */ -static struct vb2_gbb_header gbb_header; - -/* - * Read "GBB" region from SPI flash to obtain GBB header and validate - * signature. - * - * Return value: - * Success = 0 - * Error = 1 - */ -static int gbb_init(void) -{ - static bool init_done = false; - struct region_device gbb_rdev; - - if (init_done != false) - return 0; - - if (fmap_locate_area_as_rdev(GBB_FMAP_REGION_NAME, &gbb_rdev)) - return 1; - - if (rdev_readat(&gbb_rdev, &gbb_header, 0, - sizeof(struct vb2_gbb_header)) != - sizeof(struct vb2_gbb_header)) { - printk(BIOS_ERR, "%s: Failure to read GBB header!\n", __func__); - return 1; - } - - if (memcmp(gbb_header.signature, VB2_GBB_SIGNATURE, - VB2_GBB_SIGNATURE_SIZE)) { - printk(BIOS_ERR, "%s: Signature check failed!\n", __func__); - return 1; - } - - init_done = true; - return 0; -} - -uint32_t gbb_get_flags(void) -{ - if (gbb_init()) { - printk(BIOS_ERR, - "%s: Failure to initialize GBB. Returning flags as 0!\n", - __func__); - return 0; - } - return gbb_header.flags; -} - -bool gbb_is_flag_set(uint32_t flag) -{ - return !!(gbb_get_flags() & flag); -} diff --git a/src/security/vboot/gbb.h b/src/security/vboot/gbb.h deleted file mode 100644 index 389242a..0000000 --- a/src/security/vboot/gbb.h +++ /dev/null @@ -1,39 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright 2018 Google LLC - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#ifndef __SECURITY_VBOOT_GBB_H__ -#define __SECURITY_VBOOT_GBB_H__ - -#include <stdint.h> - -/* In order to use VB2_GBB_FLAG_* macros from vboot, include vb2_api.h. */ - -/* - * Read flags field from GBB header. - * Return value: - * Success: 32-bit unsigned integer representing flags field from GBB header. - * Error : 0 - */ -uint32_t gbb_get_flags(void); - -/* - * Check if given flag is set in the flags field in GBB header. - * Return value: - * true: Flag is set. - * false: Flag is not set or failure to read GBB flags. - */ -bool gbb_is_flag_set(uint32_t flag); - -#endif /* __SECURITY_VBOOT_GBB_H__ */ diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1b14799..471f838 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -50,6 +50,17 @@ }
/* + * Check if given flag is set in the flags field in GBB header. + * Return value: + * true: Flag is set. + * false: Flag is not set. + */ +static inline bool vboot_is_gbb_flag_set(enum vb2_gbb_flag flag) +{ + return !!(vb2api_gbb_get_flags(vboot_get_context()) & flag); +} + +/* * Locates firmware as a region device. Returns 0 on success, -1 on failure. */ int vboot_locate_firmware(const struct vb2_context *ctx, diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c index a24b220..458ed87 100644 --- a/src/security/vboot/vboot_common.c +++ b/src/security/vboot/vboot_common.c @@ -19,7 +19,7 @@ #include <fmap.h> #include <reset.h> #include <stddef.h> -#include <security/vboot/gbb.h> +#include <security/vboot/misc.h> #include <security/vboot/vboot_common.h> #include <security/vboot/vbnv.h> #include <vb2_api.h> @@ -31,7 +31,7 @@ if (!vboot_developer_mode_enabled()) return 0; /* Enable if GBB flag is set */ - if (gbb_is_flag_set(VB2_GBB_FLAG_ENABLE_UDC)) + if (vboot_is_gbb_flag_set(VB2_GBB_FLAG_ENABLE_UDC)) return 1; /* Enable if VBNV flag is set */ if (vbnv_udc_enable_flag())
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37261 )
Change subject: security/vboot: Use persistent context to read GBB flags ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Awesome! It didn't even occur to me that we could get rid of this with persistent context.
LGTM with nit about bool.
https://review.coreboot.org/c/coreboot/+/37261/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/37261/1/src/security/vboot/misc.h@5... PS1, Line 58: bool Are we doing bools now? I have only ever seen int used for booleans up until this point.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37261 )
Change subject: security/vboot: Use persistent context to read GBB flags ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37261/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/37261/1/src/security/vboot/misc.h@5... PS1, Line 58: bool
Are we doing bools now? I have only ever seen int used for booleans up until this point.
Depends on the repository. coreboot has had bools for a while, libpayload still doesn't have them. I'm just copying the signature from the previous code here.
Hello Aaron Durbin, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37261
to look at the new patch set (#2).
Change subject: security/vboot: Use persistent context to read GBB flags ......................................................................
security/vboot: Use persistent context to read GBB flags
With the persistent vboot context coreboot no longer needs to read GBB flags from flash itself -- it can just ask vboot for the cached result. This patch removes the existing GBB code and provides gbb_is_flag_set() (with a slightly better namespaced name) as a static inline instead.
Change-Id: Ibc3ed0f3fbeb53d630925d47df4dc474b0ed07ee Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Makefile.inc D src/security/vboot/gbb.c D src/security/vboot/gbb.h M src/security/vboot/misc.h M src/security/vboot/vboot_common.c 5 files changed, 13 insertions(+), 123 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/37261/2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37261 )
Change subject: security/vboot: Use persistent context to read GBB flags ......................................................................
Patch Set 2: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37261 )
Change subject: security/vboot: Use persistent context to read GBB flags ......................................................................
security/vboot: Use persistent context to read GBB flags
With the persistent vboot context coreboot no longer needs to read GBB flags from flash itself -- it can just ask vboot for the cached result. This patch removes the existing GBB code and provides gbb_is_flag_set() (with a slightly better namespaced name) as a static inline instead.
Change-Id: Ibc3ed0f3fbeb53d630925d47df4dc474b0ed07ee Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37261 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Joel Kitching kitching@google.com --- M src/security/vboot/Makefile.inc D src/security/vboot/gbb.c D src/security/vboot/gbb.h M src/security/vboot/misc.h M src/security/vboot/vboot_common.c 5 files changed, 13 insertions(+), 123 deletions(-)
Approvals: build bot (Jenkins): Verified Joel Kitching: Looks good to me, approved
diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 010a06c..5292bd1 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -24,8 +24,6 @@
verstage-generic-ccopts += -D__VERSTAGE__
-ramstage-y += gbb.c - bootblock-y += vbnv.c verstage-y += vbnv.c romstage-y += vbnv.c diff --git a/src/security/vboot/gbb.c b/src/security/vboot/gbb.c deleted file mode 100644 index 5293033..0000000 --- a/src/security/vboot/gbb.c +++ /dev/null @@ -1,80 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright 2018 Google LLC - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#define NEED_VB20_INTERNALS /* Peeking into vb2_gbb_header */ - -#include <commonlib/region.h> -#include <console/console.h> -#include <fmap.h> -#include <security/vboot/gbb.h> -#include <string.h> -#include <vb2_api.h> - -#define GBB_FMAP_REGION_NAME "GBB" - -/* Copy of GBB header read from boot media. */ -static struct vb2_gbb_header gbb_header; - -/* - * Read "GBB" region from SPI flash to obtain GBB header and validate - * signature. - * - * Return value: - * Success = 0 - * Error = 1 - */ -static int gbb_init(void) -{ - static bool init_done = false; - struct region_device gbb_rdev; - - if (init_done != false) - return 0; - - if (fmap_locate_area_as_rdev(GBB_FMAP_REGION_NAME, &gbb_rdev)) - return 1; - - if (rdev_readat(&gbb_rdev, &gbb_header, 0, - sizeof(struct vb2_gbb_header)) != - sizeof(struct vb2_gbb_header)) { - printk(BIOS_ERR, "%s: Failure to read GBB header!\n", __func__); - return 1; - } - - if (memcmp(gbb_header.signature, VB2_GBB_SIGNATURE, - VB2_GBB_SIGNATURE_SIZE)) { - printk(BIOS_ERR, "%s: Signature check failed!\n", __func__); - return 1; - } - - init_done = true; - return 0; -} - -uint32_t gbb_get_flags(void) -{ - if (gbb_init()) { - printk(BIOS_ERR, - "%s: Failure to initialize GBB. Returning flags as 0!\n", - __func__); - return 0; - } - return gbb_header.flags; -} - -bool gbb_is_flag_set(uint32_t flag) -{ - return !!(gbb_get_flags() & flag); -} diff --git a/src/security/vboot/gbb.h b/src/security/vboot/gbb.h deleted file mode 100644 index 389242a..0000000 --- a/src/security/vboot/gbb.h +++ /dev/null @@ -1,39 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright 2018 Google LLC - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#ifndef __SECURITY_VBOOT_GBB_H__ -#define __SECURITY_VBOOT_GBB_H__ - -#include <stdint.h> - -/* In order to use VB2_GBB_FLAG_* macros from vboot, include vb2_api.h. */ - -/* - * Read flags field from GBB header. - * Return value: - * Success: 32-bit unsigned integer representing flags field from GBB header. - * Error : 0 - */ -uint32_t gbb_get_flags(void); - -/* - * Check if given flag is set in the flags field in GBB header. - * Return value: - * true: Flag is set. - * false: Flag is not set or failure to read GBB flags. - */ -bool gbb_is_flag_set(uint32_t flag); - -#endif /* __SECURITY_VBOOT_GBB_H__ */ diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1b14799..471f838 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -50,6 +50,17 @@ }
/* + * Check if given flag is set in the flags field in GBB header. + * Return value: + * true: Flag is set. + * false: Flag is not set. + */ +static inline bool vboot_is_gbb_flag_set(enum vb2_gbb_flag flag) +{ + return !!(vb2api_gbb_get_flags(vboot_get_context()) & flag); +} + +/* * Locates firmware as a region device. Returns 0 on success, -1 on failure. */ int vboot_locate_firmware(const struct vb2_context *ctx, diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c index a24b220..458ed87 100644 --- a/src/security/vboot/vboot_common.c +++ b/src/security/vboot/vboot_common.c @@ -19,7 +19,7 @@ #include <fmap.h> #include <reset.h> #include <stddef.h> -#include <security/vboot/gbb.h> +#include <security/vboot/misc.h> #include <security/vboot/vboot_common.h> #include <security/vboot/vbnv.h> #include <vb2_api.h> @@ -31,7 +31,7 @@ if (!vboot_developer_mode_enabled()) return 0; /* Enable if GBB flag is set */ - if (gbb_is_flag_set(VB2_GBB_FLAG_ENABLE_UDC)) + if (vboot_is_gbb_flag_set(VB2_GBB_FLAG_ENABLE_UDC)) return 1; /* Enable if VBNV flag is set */ if (vbnv_udc_enable_flag())
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)