Differential D22374 Diff 64405 sys/x86/x86/cpu_machdep.c

Changeset View

Standalone View

sys/x86/x86/cpu_machdep.c

	Show First 20 Lines • Show All 1,129 Lines • ▼ Show 20 Lines
	}			}

	SYSCTL_PROC(_hw, OID_AUTO, mds_disable, CTLTYPE_INT \|			SYSCTL_PROC(_hw, OID_AUTO, mds_disable, CTLTYPE_INT \|
	CTLFLAG_RWTUN \| CTLFLAG_NOFETCH \| CTLFLAG_MPSAFE, NULL, 0,			CTLFLAG_RWTUN \| CTLFLAG_NOFETCH \| CTLFLAG_MPSAFE, NULL, 0,
	sysctl_mds_disable_handler, "I",			sysctl_mds_disable_handler, "I",
	"Microarchitectural Data Sampling Mitigation "			"Microarchitectural Data Sampling Mitigation "
	"(0 - off, 1 - on VERW, 2 - on SW, 3 - on AUTO");			"(0 - off, 1 - on VERW, 2 - on SW, 3 - on AUTO");


				/*
				* Intel Transactional Memory Asynchronous Abort Mitigation
				* CVE-2019-11135
				*/
				int x86_taa_disable;
				int x86_taa_state;
				enum {
				kibUnsubmitted Done Inline Actions yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as well. kib: yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as…
				TAA_NONE = 0,
				TAA_TSX_DISABLE = 1,
				TAA_VERW = 2,
				TAA_AUTO = 3
				};

				static void
				taa_set_one(bool enable)
				{
				uint64_t v;

				v = rdmsr(MSR_IA32_TSX_CTRL);
				if (enable)
				v \|= (uint64_t)(IA32_TSX_CTRL_RTM_DISABLE \|
				IA32_TSX_CTRL_TSX_CPUID_CLEAR);
				kibUnsubmitted Not Done Inline Actions I think you need to check the existing value of hw_mds_disable. If it is non-zero, do not touch it. If it is zero, setting it to 1 (VERW) might not work simply because the right microcode is not loaded. 3 (auto) might be more reasonable value. kib: I think you need to check the existing value of hw_mds_disable. If it is non-zero, do not…
				scottlAuthorUnsubmitted Done Inline Actions My intent was to assume that `hw_mds_recalculate()` would determine the correct state. If this code asks for VERW but that can't be done, then it'll just fail anyways. scottl: My intent was to assume that `hw_mds_recalculate()` would determine the correct state. If this…
				else
				v &= ~(uint64_t)(IA32_TSX_CTRL_RTM_DISABLE \|
				IA32_TSX_CTRL_TSX_CPUID_CLEAR);

				wrmsr(MSR_IA32_TSX_CTRL, v);
				}

				static void
				taa_set(bool enable, bool all)
				{
				struct thread *td;
				int bound_cpu, i, is_bound;

				if (all) {
				td = curthread;
				thread_lock(td);
				is_bound = sched_is_bound(td);
				bound_cpu = td->td_oncpu;
				CPU_FOREACH(i) {
				sched_bind(td, i);
				taa_set_one(enable);
				}
				if (is_bound)
				sched_bind(td, bound_cpu);
				else
				sched_unbind(td);
				thread_unlock(td);
				} else
				taa_set_one(enable);
				}

				void
				x86_taa_recalculate(void)
				{
				static int taa_saved_mds_disable = 0;
				int taa_need = 0, taa_state = 0;
				int mds_disable = 0, need_mds_recalc = 0;

				/* Check CPUID.07h.EBX.HLE and RTM for the presence of TSX */
				if ((cpu_stdext_feature & CPUID_STDEXT_HLE) == 0 \|\|
				(cpu_stdext_feature & CPUID_STDEXT_RTM) == 0) {
				/* TSX is not present */
				x86_taa_state = 0;
				return;
				}
				emasteUnsubmitted Done Inline Actions maybe `/* CPU is not susceptible to TAA /` emaste:* maybe `/* CPU is not susceptible to TAA */`

				/* Check to see what mitigation options the CPU gives us */
				emasteUnsubmitted Done Inline Actions `/* CPU is susceptible to TAA and has the ability to disable TSX /` emaste:* `/* CPU is susceptible to TAA and has the ability to disable TSX */`
				scottlAuthorUnsubmitted Done Inline Actions What you've described are two states that can be mutually exclusive. scottl: What you've described are two states that can be mutually exclusive.
				emasteUnsubmitted Done Inline Actions But this case here (where we set tsx_need = TSX_TAA_DISABLE) is exactly that - the CPU is susceptible to TAA, and can turn off TSX. emaste: But this case here (where we set tsx_need = TSX_TAA_DISABLE) is exactly that - the CPU is…
				if (cpu_ia32_arch_caps & IA32_ARCH_CAP_TAA_NO)
				/* CPU is not suseptible to TAA */
				taa_need = TAA_NONE;
				else if (cpu_ia32_arch_caps & IA32_ARCH_CAP_TSX_CTRL)
				/*
				* CPU can turn off TSX. This is the next best option
				* if TAA_NO hardware mitigation isn't present
				*/
				taa_need = TAA_TSX_DISABLE;
				else {
				/* No TSX/TAA specific remedies are available. */
				if (x86_taa_disable == TAA_TSX_DISABLE) {
				if (bootverbose)
				printf("TSX control not available\n");
				return;
				} else
				taa_need = TAA_VERW;
				}

				/* Can we automatically take action, or are we being forced? */
				if (x86_taa_disable == TAA_AUTO)
				taa_state = taa_need;
				else
				taa_state = x86_taa_disable;

				/* No state change, nothing to do */
				if (taa_state == x86_taa_state) {
				if (bootverbose)
				kibUnsubmitted Done Inline Actions Why use ints and implicit convertions ? We have true/false constant symbols in C99. kib: Why use ints and implicit convertions ? We have true/false constant symbols in C99.
				printf("No TSX change made\n");
				return;
				}

				/* Does the MSR need to be turned on or off? */
				if (taa_state == TAA_TSX_DISABLE)
				taa_set(true, true);
				else if (x86_taa_state == TAA_TSX_DISABLE)
				taa_set(false, true);

				kibUnsubmitted Not Done Inline Actions hw_mds_disable is user-controlled, and user settings should be honored IMO. This is one of the reason why I proposed to merge taa and mds recalculation functions. kib: hw_mds_disable is user-controlled, and user settings should be honored IMO. This is one of the…
				scottlAuthorUnsubmitted Done Inline Actions How would it work if a user specified VERW for TAA and SW for MDS? Maybe these can be merged in the future, but for now I would like to keep them separate, and enforce that TAA is a superset of MDS, as stated in the review summary. scottl: How would it work if a user specified VERW for TAA and SW for MDS? Maybe these can be merged…
				kibUnsubmitted Not Done Inline Actions In this case you need somehow merge them, and this is easier to do in the merged function. But I think that you should either not allow that fine control over taa (e.g. only provide disabled/mds/tsx variants of mitigation). Or even better, further along lines of merged mds/taa handling, do not introduce separate knob to taa. Consider taa action as continuation of mds. For instance, auto would mean: if both MDS_NO and TAA_NO, do nothing. If MDS requires VERW, _or_ TAA requires VERW, it is VERW. If VERW is not available, it is microarch-specific code sequences. kib: In this case you need somehow merge them, and this is easier to do in the merged function. But…
				scottlAuthorUnsubmitted Done Inline Actions I agree that this maybe could be better, but trying to handle MDS_NO, TAA_NO, TSX_CTRL feature bits (not to mention the MDS and RTM/HLE presence bits) as well as VERW vs software mitigation, is a very complex logic table. I'd like to keep it more simple for now, and consider combining in the future. scottl: I agree that this maybe could be better, but trying to handle MDS_NO, TAA_NO, TSX_CTRL feature…
				/* Does MDS need to be set to turn on VERW? */
				if (taa_state == TAA_VERW) {
				taa_saved_mds_disable = hw_mds_disable;
				mds_disable = hw_mds_disable = 1;
				need_mds_recalc = 1;
				} else if (x86_taa_state == TAA_VERW) {
				mds_disable = hw_mds_disable = taa_saved_mds_disable;
				need_mds_recalc = 1;
				}
				if (need_mds_recalc) {
				hw_mds_recalculate();
				if (mds_disable != hw_mds_disable) {
				if (bootverbose)
				printf("Cannot change MDS state for TAA\n");
				/* Don't update our state */
				return;
				}
				}

				x86_taa_state = taa_state;
				return;
				}

				static void
				taa_recalculate_boot(void * arg __unused)
				{

				x86_taa_recalculate();
				}
				SYSINIT(taa_recalc, SI_SUB_SMP, SI_ORDER_ANY, taa_recalculate_boot, NULL);

				static int
				sysctl_taa_handler(SYSCTL_HANDLER_ARGS)
				{
				int error, val;

				val = x86_taa_disable;
				error = sysctl_handle_int(oidp, &val, 0, req);
				if (error != 0 \|\| req->newptr == NULL)
				return (error);
				if (val < 0 \|\| val > 3)
				return (EINVAL);
				x86_taa_disable = val;
				x86_taa_recalculate();
				return (0);
				}

				SYSCTL_PROC(_machdep_mitigations, OID_AUTO, taa, CTLTYPE_INT \|
				CTLFLAG_RWTUN \| CTLFLAG_NOFETCH \| CTLFLAG_MPSAFE, NULL, 0,
				sysctl_taa_handler, "I",
				"TSX Asynchronous Abort Mitigation "
				"(0 - off, 1 - disable TSX, 2 - VERW, 3 - on AUTO");

				static int
				emasteUnsubmitted Not Done Inline Actions I'm not sure that "inactive" is the right name for this, should probably be "TSX not disabled"? emaste: I'm not sure that "inactive" is the right name for this, should probably be "TSX not disabled"?
				scottlAuthorUnsubmitted Done Inline Actions I think that this is handled by the OID renaming scottl: I think that this is handled by the OID renaming
				sysctl_taa_state_handler(SYSCTL_HANDLER_ARGS)
				{
				const char *state;

				switch (x86_taa_state) {
				case TAA_NONE:
				emasteUnsubmitted Done Inline Actions Just "VERW"? emaste: Just "VERW"?
				state = "inactive";
				break;
				case TAA_TSX_DISABLE:
				state = "TSX disabled";
				break;
				case TAA_VERW:
				state = "VERW";
				break;
				default:
				state = "unknown";
				}

				return (SYSCTL_OUT(req, state, strlen(state)));
				}

				SYSCTL_PROC(_machdep_mitigations, OID_AUTO, taa_state,
				CTLTYPE_STRING \| CTLFLAG_RD \| CTLFLAG_MPSAFE, NULL, 0,
				sysctl_taa_state_handler, "A",
				"Transactional Memory Asynchronous Abort Mitigation state");

	/*			/*
	* Enable and restore kernel text write permissions.			* Enable and restore kernel text write permissions.
	* Callers must ensure that disable_wp()/restore_wp() are executed			* Callers must ensure that disable_wp()/restore_wp() are executed
	* without rescheduling on the same core.			* without rescheduling on the same core.
	*/			*/
	bool			bool
	disable_wp(void)			disable_wp(void)
	{			{
	Show All 37 Lines