lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Nov 2023 09:52:19 +0200 (EET) From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> To: David Thompson <davthompson@...dia.com> cc: Hans de Goede <hdegoede@...hat.com>, markgross@...nel.org, vadimp@...dia.com, platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, kblaiech@...dia.com Subject: Re: [PATCH v1] mlxbf-bootctl: check the secure boot development mode status bit On Mon, 20 Nov 2023, David Thompson wrote: > If the secure boot is enabled with the development key, then print > it to the output buffer when lifecycle_state_show() is invoked. > > Fixes: 79e29cb8fbc5c ("platform/mellanox: Add bootctl driver for Mellanox BlueField Soc") The commit message says nothing that warrants a Fixes tag. Also, the commit message doesn't tell why you need to do this, that is, it doesn't tell what's the current situation and how it's wrong/unwanted. Please amend. > Reviewed-by: Khalil Blaiech <kblaiech@...dia.com> > Signed-off-by: David Thompson <davthompson@...dia.com> > --- > drivers/platform/mellanox/mlxbf-bootctl.c | 24 +++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/mellanox/mlxbf-bootctl.c b/drivers/platform/mellanox/mlxbf-bootctl.c > index 1ac7dab22c63..ed22908d79b9 100644 > --- a/drivers/platform/mellanox/mlxbf-bootctl.c > +++ b/drivers/platform/mellanox/mlxbf-bootctl.c > @@ -20,6 +20,7 @@ > > #define MLXBF_BOOTCTL_SB_SECURE_MASK 0x03 > #define MLXBF_BOOTCTL_SB_TEST_MASK 0x0c > +#define MLXBF_BOOTCTL_SB_DEV_MASK 0x10 BIT(4) (Those other too could be converted to GENMASK() but not in this patch.) > #define MLXBF_SB_KEY_NUM 4 > > @@ -40,11 +41,18 @@ static struct mlxbf_bootctl_name boot_names[] = { > { MLXBF_BOOTCTL_NONE, "none" }, > }; > > +enum { > + MLXBF_BOOTCTL_SB_LIFECYCLE_PRODUCTION = 0, > + MLXBF_BOOTCTL_SB_LIFECYCLE_GA_SECURE = 1, > + MLXBF_BOOTCTL_SB_LIFECYCLE_GA_NON_SECURE = 2, > + MLXBF_BOOTCTL_SB_LIFECYCLE_RMA = 3 > +}; > + > static const char * const mlxbf_bootctl_lifecycle_states[] = { > - [0] = "Production", > - [1] = "GA Secured", > - [2] = "GA Non-Secured", > - [3] = "RMA", > + [MLXBF_BOOTCTL_SB_LIFECYCLE_PRODUCTION] = "Production", > + [MLXBF_BOOTCTL_SB_LIFECYCLE_GA_SECURE] = "GA Secured", > + [MLXBF_BOOTCTL_SB_LIFECYCLE_GA_NON_SECURE] = "GA Non-Secured", > + [MLXBF_BOOTCTL_SB_LIFECYCLE_RMA] = "RMA", > }; > > /* Log header format. */ > @@ -254,8 +262,9 @@ static ssize_t lifecycle_state_show(struct device *dev, > if (lc_state < 0) > return lc_state; > > - lc_state &= > - MLXBF_BOOTCTL_SB_TEST_MASK | MLXBF_BOOTCTL_SB_SECURE_MASK; > + lc_state &= (MLXBF_BOOTCTL_SB_TEST_MASK | > + MLXBF_BOOTCTL_SB_SECURE_MASK | > + MLXBF_BOOTCTL_SB_DEV_MASK); > > /* > * If the test bits are set, we specify that the current state may be > @@ -266,6 +275,9 @@ static ssize_t lifecycle_state_show(struct device *dev, > > return sprintf(buf, "%s(test)\n", > mlxbf_bootctl_lifecycle_states[lc_state]); > + } else if ((lc_state & MLXBF_BOOTCTL_SB_SECURE_MASK) == MLXBF_BOOTCTL_SB_LIFECYCLE_GA_SECURE > + && (lc_state & MLXBF_BOOTCTL_SB_DEV_MASK)) { I cannot review this line until you amend the commit message with the above mentioned details. To be more precise, I'm interested in understanding if you've precedences right here so your commit message should have enough details to support me in that decision, thank you. > + return sprintf(buf, "Secured (development)\n"); > } > > return sprintf(buf, "%s\n", mlxbf_bootctl_lifecycle_states[lc_state]); > -- i.
Powered by blists - more mailing lists