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  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <DM6PR12MB55341582E9127EEB70613B71C782A@DM6PR12MB5534.namprd12.prod.outlook.com>
Date:   Thu, 30 Nov 2023 18:24:47 +0000
From:   David Thompson <davthompson@...dia.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     Hans de Goede <hdegoede@...hat.com>,
        "markgross@...nel.org" <markgross@...nel.org>,
        Vadim Pasternak <vadimp@...dia.com>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Khalil Blaiech <kblaiech@...dia.com>
Subject: RE: [PATCH v2] mlxbf-bootctl: correctly identify secure boot with
 development keys

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Sent: Wednesday, November 22, 2023 5:49 AM
> To: David Thompson <davthompson@...dia.com>
> Cc: Hans de Goede <hdegoede@...hat.com>; markgross@...nel.org; Vadim
> Pasternak <vadimp@...dia.com>; platform-driver-x86@...r.kernel.org; LKML
> <linux-kernel@...r.kernel.org>; Khalil Blaiech <kblaiech@...dia.com>
> Subject: Re: [PATCH v2] mlxbf-bootctl: correctly identify secure boot with
> development keys
> 
> On Tue, 21 Nov 2023, David Thompson wrote:
> 
> > The secure boot state of the BlueField SoC is represented by two bits:
> >                 0 = production state
> >                 1 = secure boot enabled
> >                 2 = non-secure (secure boot disabled)
> >                 3 = RMA state
> > There is also a single bit to indicate whether production keys or
> > development keys are being used when secure boot is enabled.
> 
> Thanks for the extra details but there are more bits that come into play here and
> you mention anything about them. More about this below with the relevant code.
> 
> > The current logic in "lifecycle_state_show()" does not handle the case
> > where the SoC is configured for secure boot and is using development
> > keys.
> 
> This still doesn't state why the current state is a problem. That is, why "GA
> Secured" is a problem.
> 

"GA secured" is when secure boot is enabled with official production keys.
"Secured (development)" is when secure boot is enabled with development keys.
Without this fix "GA Secured" is displayed on development cards which is misleading.

> > This patch updates the logic in "lifecycle_state_show()" to support
> > this combination and properly report this state.
> >
> > Fixes: 79e29cb8fbc5c ("platform/mellanox: Add bootctl driver for
> > Mellanox BlueField Soc")
> > Reviewed-by: Khalil Blaiech <kblaiech@...dia.com>
> > Signed-off-by: David Thompson <davthompson@...dia.com>
> > ---
> > v1->v2
> > a) commit message was expanded and re-worded for clarity
> > b) replaced use of hardcoded 0x10 with BIT(4) for
> > MLXBF_BOOTCTL_SB_DEV_MASK
> > ---
> >  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..13c62a97a6f7 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		BIT(4)
> 
> You only covered MLXBF_BOOTCTL_SB_SECURE_MASK and
> MLXBF_BOOTCTL_SB_DEV_MASK in your description above, is that correct?
> 

When the chip lifecycle is 0 (production), test lifecycle bits can be used to simulate secure boot without having to burn the fuses.
The bits are OR-ed to the real lifecycle bits; thus, we are adding an extra mask to indicated whether we are test mode.

> >  #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);
> >
> > @@ -266,6 +275,9 @@ static ssize_t lifecycle_state_show(struct device
> > *dev,
> 
> I'm quoting some extra code not fully visible in the contexts:
> 
>         /*
>          * If the test bits are set, we specify that the current state may be
>          * due to using the test bits.
>          */
>         if (lc_state & MLXBF_BOOTCTL_SB_TEST_MASK) {
>                 lc_state &= MLXBF_BOOTCTL_SB_SECURE_MASK;
> 
> Here what is output also depends on MLXBF_BOOTCTL_SB_TEST_MASK, right?
> And those bits even takes precedence over the code you're adding into else if
> branch. So your description in commit message seems quite inadequate to me.
> 

Please see above for description of how the test bits are used.

> Note that you've also added an out-of-bound accesses here since only
> MLXBF_BOOTCTL_SB_SECURE_MASK gets cleared from lc_state:
> 

The next version of the patch (v3) will clarify this logic and will prevent
any out-of-bound accesses.

> >
> >  		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)) {
> > +		return sprintf(buf, "Secured (development)\n");
> >  	}
> >
> >  	return sprintf(buf, "%s\n",
> > mlxbf_bootctl_lifecycle_states[lc_state]);
> 
> Here's another potential out-of-bound access if the holes in the above if logic
> aligns.
> 
> --
>  i.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ