From owner-freebsd-bugs@freebsd.org Mon Mar 26 20:54:58 2018 Return-Path: Delivered-To: freebsd-bugs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7E8F0F6B0C0 for ; Mon, 26 Mar 2018 20:54:58 +0000 (UTC) (envelope-from bugzilla-noreply@freebsd.org) Received: from mxrelay.ysv.freebsd.org (mxrelay.ysv.freebsd.org [IPv6:2001:1900:2254:206a::19:3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mxrelay.ysv.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1034F73B87 for ; Mon, 26 Mar 2018 20:54:58 +0000 (UTC) (envelope-from bugzilla-noreply@freebsd.org) Received: from kenobi.freebsd.org (kenobi.freebsd.org [IPv6:2001:1900:2254:206a::16:76]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.ysv.freebsd.org (Postfix) with ESMTPS id 6AAF221490 for ; Mon, 26 Mar 2018 20:54:57 +0000 (UTC) (envelope-from bugzilla-noreply@freebsd.org) Received: from kenobi.freebsd.org ([127.0.1.118]) by kenobi.freebsd.org (8.15.2/8.15.2) with ESMTP id w2QKsvdm085909 for ; Mon, 26 Mar 2018 20:54:57 GMT (envelope-from bugzilla-noreply@freebsd.org) Received: (from www@localhost) by kenobi.freebsd.org (8.15.2/8.15.2/Submit) id w2QKsvP7085908 for freebsd-bugs@FreeBSD.org; Mon, 26 Mar 2018 20:54:57 GMT (envelope-from bugzilla-noreply@freebsd.org) X-Authentication-Warning: kenobi.freebsd.org: www set sender to bugzilla-noreply@freebsd.org using -f From: bugzilla-noreply@freebsd.org To: freebsd-bugs@FreeBSD.org Subject: [Bug 192487] cpucontrol uses unsafe procedure to detect current microcode version Date: Mon, 26 Mar 2018 20:54:57 +0000 X-Bugzilla-Reason: AssignedTo X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: Base System X-Bugzilla-Component: bin X-Bugzilla-Version: CURRENT X-Bugzilla-Keywords: X-Bugzilla-Severity: Affects Many People X-Bugzilla-Who: dan+freebsd.org@obluda.cz X-Bugzilla-Status: New X-Bugzilla-Resolution: X-Bugzilla-Priority: --- X-Bugzilla-Assigned-To: freebsd-bugs@FreeBSD.org X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: cc Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: https://bugs.freebsd.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Mar 2018 20:54:58 -0000 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D192487 Dan Lukes changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dan+freebsd.org@obluda.cz --- Comment #4 from Dan Lukes --- Well, no one has provided the patch, so I will try one. Let's allow me to summarize first. --- ------------ --- part of=20 --- Intel=C2=AE 64 and IA-32 Architectures Software Developer=E2=80=99s Ma= nual Volume 3A: --- System Programming Guide, Part 1 --- paragraph 9.11.7.1 Determining the Signature --- ------------ CPUID returns a value in a model specific register in addition to its usual register return values. The semantics of CPUID cause it to deposit an updat= e ID value in the 64-bit model-specific register at address 08BH (IA32_BIOS_SIGN_ID). If no update is present in the processor, the value in= the MSR remains unmodified. The BIOS must pre-load a zero into the MSR before executing CPUID. If a read of the MSR at 8BH still returns zero after execu= ting CPUID, this indicates that no update is present. --- ------------ It's clear description - IA32_BIOS_SIGN_ID needs to be initialized to zero before CPUID or it's value can't be trusted after CPUID.=20 =3D=3D=3D=3D=3D CPUCONTROL =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- ------------ --- releng/11.1/usr.sbin/cpucontrol/intel.c 245491 2013-01-16 05:00:51Z ea= dler --- intel_update() --- ------------ Such function calls CPUID (line 116) with no prior initialization of IA32_BIOS_SIGN_ID, then read MSR_BIOS_SIGN (line 133) considering returned value "revision". Such revision is later compared with the version of patch= in attempt to decide the particular patch is newer than the one installed in C= PU. This implementation is INCORRECT. On unpatched procesor, the value of IA32_BIOS_SIGN_ID remain unchanged, thus the 'revision' is undefined random value. Because of it, the patch candidate may be considered old. In such ca= se, CPU remain in virgin state, with no attempt to update. I would like to note that the comment on sys/dev/cpuctl/cpuctl.c line 259 is unrelated to the issue we are speaking on. It's probably just copy&paste ec= ho of line 214 Suggested patch: initialize IA32_BIOS_SIGN_ID to 0 before CPUID; patch atta= ched Note - IA32_BIOS_SIGN_ID shall be initialized just before CPUID and read ju= st after it - to be on safe site. Or we are in risk someone else will modify IA32_BIOS_SIGN_ID in the mean time. Userland code can't do it reliably. So = the proposed patch need to be considered workaround. Final solution require new CPUCTL_x that will do WRMSR+CPUID+RDMSR sequence enclosed in critical_enter= () and critical_exit()=20=20 =3D=3D=3D=3D=3D CPUCTL_UPDATE =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Despite this PR is not related to CPUCTL_UPDATE, I would like to make short notice claiming the implementation is broken as well and Mitchell Horne is = not true. But this broken implementation doesn't cause the eligible update is skipped. --- ------------ --- releng/11.1/sys/dev/cpuctl/cpuctl.c 330908 2018-03-14 04:00:00Z gordon --- ------------ CPUCTL_UPDATE, e.g. cpuctl_do_update() starts with CPUID (line 311) with no prior initialization of IA32_BIOS_SIGN_ID. Then it reads such MSR (line 365) into rev0 variable. On virgin CPU it will receive random garbage. Then the code do update (line 370) then it try to read IA32_BIOS_SIGN_ID into rev1 value. Unfortunately it uses CPUID with EAX=3D0 instead of (see example 9-9= in Intel's SDK) CPUID with EAX=3D1. So even the rev1 content is undefined. Fi= nally, it return 0 if rev1 > 0, otherwise it returns EEXIT.=20 Garbage in rev0 and rev1 may cause EEXIST error even in the case the update= has succeeded. Suggested patch: we can't initialize IA32_BIOS_SIGN_ID before CPUID (line 3= 11) as it's called in Intel/AMD common code and must not touch Intel-only MSR h= ere. Thus we need initialize initialize IA32_BIOS_SIGN_ID then call CPUID again = in update_intel(), CPUID with EAX=3D1 needs to be called to read rev1, and rel= ated RDMSR should be part of critical section; patch attached --=20 You are receiving this mail because: You are the assignee for the bug.=