Received-SPF: pass (google.com: domain of linux-kernel+bounces-154057-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 22 Apr 2024 14:32:13 -0700
From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To: "Chang S. Bae" <chang.seok.bae@intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, dan.j.williams@intel.com,
	bernie.keany@intel.com, charishma1.gairuboyina@intel.com,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	daniel.sneddon@linux.intel.com,
	antonio.gomez.iglesias@linux.intel.com
Subject: Re: [PATCH 15/14] x86/gds: Lock GDS mitigation when keylocker
 feature is present
Message-ID: <20240422213213.7og6adib7zcbpnys@desk>
References: <20240407230432.912290-1-chang.seok.bae@intel.com>
 <20240419-gds-lock-v1-1-adcbef6ce24b@linux.intel.com>
 <142380d5-27bd-4ddc-8c33-03a2f8701fa2@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <142380d5-27bd-4ddc-8c33-03a2f8701fa2@intel.com>

On Mon, Apr 22, 2024 at 12:35:45AM -0700, Chang S. Bae wrote:
> On 4/19/2024 10:47 AM, Pawan Gupta wrote:
> >   	/*
> > @@ -840,6 +843,11 @@ static void __init gds_select_mitigation(void)
> >   		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> >   	}
> > +	/* Keylocker can only be enabled when GDS mitigation is locked */
> > +	if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
> > +	    gds_mitigation == GDS_MITIGATION_FULL)
> > +		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> > +
> 
> I'm having trouble understanding this change:
> 
> gds_select_mitigation()
> {
> 	...
> 	if (gds_mitigation == GDS_MITIGATION_FORCE)
> 		gds_mitigation = GDS_MITIGATION_FULL;
> 
> 	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
> 	if (mcu_ctrl & GDS_MITG_LOCKED) {
> 		...
> 		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> 	}
> 
> 	if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
> 	    gds_mitigation == GDS_MITIGATION_FULL)
> 		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> 
> As I understand it, gds_mitigation is set to GDS_MITIGATION_FULL only if the
> gds force option is enabled but IA32_MCU_OPT_CTRL[GDS_MITG_LOCK] is not set.

Not true, GDS_MITIGATION_FULL is the default. Cmdline
gather_data_sampling=force deploys a software fallback mitigation when
the microcode mitigation is not present. But, when microcode mitigation
is present, mitigation is set to GDS_MITIGATION_FULL.

> Then, if the CPU has Key Locker, this code sets gds_mitigation to
> GDS_MITIGATION_FULL_LOCKED, which seems contradictory. I'm not sure why this
> change is necessary.
>
> I'm also not convinced that the Key Locker series needs to modify this
> function. The Key Locker setup code should simply check the current
> mitigation status and enable the feature only if proper mitigation is in
> place. Am I missing something here?

To enable Key Locker feature, "proper mitigation" is microcode mitigation
enabled and the GDS_MITG_LOCK bit set in MSR_IA32_MCU_OPT_CTRL. Do you
agree?

If not via this patch, how is GDS_MITG_LOCK going to be set?

Below is from Intel's documentation:

  "Intel recommends that system software does not enable Key Locker (by
  setting CR4.KL) unless the GDS mitigation is enabled (IA32_MCU_OPT_CTRL
  [GDS_MITG_DIS] (bit 4) is 0) and locked (IA32_MCU_OPT_CTRL
  [GDS_MITG_LOCK](bit 5) is 1). This will prevent an adversary that takes
  control of the system from turning off the mitigation in order to infer
  the keys behind Key Locker handles.

  To support GDS mitigation locking for Key Locker, microcode updates
  for Tiger Lake systems enable the following model-specific behavior
  for GDS_MITG_LOCK. On these systems, a write to IA32_MCU_OPT_CTRL MSR
  with GDS_MITG_DIS (bit 4) value 0 and GDS_MITG_LOCK (bit 5) value 1
  will lock both bits at these values until reset."