linux-kernel - Re: [PATCHv7 21/30] x86/acpi, x86/boot: Add multiprocessor wake-up support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c0c05a8-08f2-7d5d-6746-a92d0d3c5ef0@intel.com>
Date:   Mon, 28 Mar 2022 12:17:35 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        luto@...nel.org, peterz@...radead.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, aarcange@...hat.com,
        ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
        hpa@...or.com, jgross@...e.com, jmattson@...gle.com,
        joro@...tes.org, jpoimboe@...hat.com, knsathya@...nel.org,
        pbonzini@...hat.com, sdeep@...are.com, seanjc@...gle.com,
        tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
        thomas.lendacky@....com, brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCHv7 21/30] x86/acpi, x86/boot: Add multiprocessor wake-up
 support

On 3/24/22 08:24, Kirill A. Shutemov wrote:
> On Fri, Mar 18, 2022 at 11:23:59AM -0700, Dave Hansen wrote:
>> On 3/18/22 08:30, Kirill A. Shutemov wrote:
>>> +	/*
>>> +	 * After writing the wakeup command, wait for maximum timeout of 0xFF
>>> +	 * for firmware to reset the command address back zero to indicate
>>> +	 * the successful reception of command.
>>> +	 * NOTE: 0xFF as timeout value is decided based on our experiments.
>>> +	 *
>>> +	 * XXX: Change the timeout once ACPI specification comes up with
>>> +	 *      standard maximum timeout value.
>>> +	 */
>>> +	timeout = 0xFF;
>>> +	while (READ_ONCE(acpi_mp_wake_mailbox->command) && --timeout)
>>> +		cpu_relax();
>>
>> I don't feel like this was ever actually resolved.  This timeout
>> basically boiled down to "this value seems to work for us".  There are
>> also *SURELY* timeouts that are going to happen here.
> 
> It makes me think if it can be an attack vector: once kernel initiated
> wake up of a secondary vCPU it has no control on how long it takes.

I don't think that's a meaningful attack vector for TDX.  Hosts always
have a choice to not run the guest.  Sure, they can hang the boot CPU
forever if they don't run the secondary CPU.  But, they can also do that
by just not ever running the boot CPU in the first place.

> I worry that malicious VMM can induce timeout intentionally, but then wake
> up the secondary CPU when kernel doesn't expect it. After quick look I was
> not able to convince myself that kernel can deal with this without a
> problem.
> 
> Is it legitimate concern?

The only thing I'd be worried about would be a secondary CPU going off
and doing things that it's not supposed to because *it* thinks it is in
boot code and the other CPUs are off doing normal runtime things.

I don't know the boot code well enough to guess where it would hang or
what damage it could do.  I suspect it would get hung up on something
like 'cpu_callout_mask'.

Is there anything in the trampoline code or start_secondary() that would
cause damage if it got run later than the kernel expects?

> Patch below drops timeout handling completely. Any opinions?
> 
> Other option would be to check in the trampoline code that initiated wake
> up is legitimate. But it should only be untrue if VMM acting weird (or
> virtual BIOS is buggy). I don't think it's right side to deal with the
> problem.

Yeah, that would just be a band-aid.  If we are worried about the
secondary CPU doing some damage at a random time, the host can just wait
until *after* the wakeup check.

>>>         /*
>>>          * If the CPU wakeup process is successful, store the
>>>          * status in apic_id_wakemap to prevent re-wakeup
>>>          * requests.
>>>          */
>>>         physid_set(apicid, apic_id_wakemap);
>>>
>>>         return 0;
>>> }
>>
>> If this goes wrong, won't the new wakeup just timeout?  Do we really
>> need a dedicated mechanism to stop re-wakeups?  How much of a problem is
>> this going to be?
> 
> Well, it can provide a proper diagnostics and a distinct error code. If
> you think it is unneeded we can drop it.

It looks like debugging that you add when you're bringing something up.
 I'm not sure of its utility going forward.  I'd just zap it for now and
bring it back later if it's really needed.