Message-ID: <20220330231609.ymxekzgdp2pd7gfy@black.fi.intel.com>
Date:   Thu, 31 Mar 2022 02:16:09 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.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 Mon, Mar 28, 2022 at 12:17:35PM -0700, Dave Hansen wrote:
> 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?

I didn't find anything specific.

But I tried to simulate similar scenario by returning -EIO just after
writing wake up command in acpi_wakeup_cpu(). System booted, but there is
a warning in the dmesg:

WARNING: CPU: 0 PID: 1 at kernel/irq/chip.c:210 irq_startup+0x2a3/0x2b0

System seems recovered fine, but I'm not sure what will happen if
wake up is delayed by VMM.

> > 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.

VMM cannot wake up secondary CPU on its own. Guest has to initiate it by
writing into the mailbox. Reader of the mailbox command is also within
trust boundary -- it loops inside virtual BIOS code. But VMM can
*postpone* reacting to the command by not scheduling the secondary.

> >>>         /*
> >>>          * 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.

Sathya pointed out that this protection was added based on request[1] by
Rafael.

TDX should be safe from re-wakeups as we forbid offlining CPUs, but the
wake up method suppose to be generic, not limited to TDX.

[1] https://lore.kernel.org/all/CAJZ5v0jonk2Pb2HUHMoe69vnCV4+EV9XZpo2LwKrnYPF3CxD_A@mail.gmail.com/

-- 
 Kirill A. Shutemov

Powered by blists - more mailing lists