linux-kernel - Re: [Patch v3 13/14] PCI: hv: Add hypercalls to read/write MMIO space

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: <Y3ZiAE+t6+ZptUuo@google.com>
Date:   Thu, 17 Nov 2022 16:32:00 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Wei Liu <wei.liu@...nel.org>
Cc:     Michael Kelley <mikelley@...rosoft.com>, hpa@...or.com,
        kys@...rosoft.com, haiyangz@...rosoft.com, decui@...rosoft.com,
        luto@...nel.org, peterz@...radead.org, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        lpieralisi@...nel.org, robh@...nel.org, kw@...ux.com,
        bhelgaas@...gle.com, arnd@...db.de, hch@...radead.org,
        m.szyprowski@...sung.com, robin.murphy@....com,
        thomas.lendacky@....com, brijesh.singh@....com, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        Tianyu.Lan@...rosoft.com, kirill.shutemov@...ux.intel.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, ak@...ux.intel.com,
        isaku.yamahata@...el.com, dan.j.williams@...el.com,
        jane.chu@...cle.com, tony.luck@...el.com, x86@...nel.org,
        linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
        netdev@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-arch@...r.kernel.org, iommu@...ts.linux.dev
Subject: Re: [Patch v3 13/14] PCI: hv: Add hypercalls to read/write MMIO space

On Thu, Nov 17, 2022, Wei Liu wrote:
> On Wed, Nov 16, 2022 at 10:41:36AM -0800, Michael Kelley wrote:
> [...]
> >  
> > +static void hv_pci_read_mmio(struct device *dev, phys_addr_t gpa, int size, u32 *val)
> > +{
> > +	struct hv_mmio_read_input *in;
> > +	struct hv_mmio_read_output *out;
> > +	u64 ret;
> > +
> > +	/*
> > +	 * Must be called with interrupts disabled so it is safe
> > +	 * to use the per-cpu input argument page.  Use it for
> > +	 * both input and output.
> > +	 */

There's no need to require interrupts to be disabled to safely use a per-cpu
variable, simply disabling preemption also provides the necessary protection.
And this_cpu_ptr() will complain with CONFIG_DEBUG_PREEMPT=y if preemption isn't
disabled.

IIUC, based on the existing code, what is really be guarded against is an IRQ arriving
and initiating a different hypercall from IRQ context, and thus corrupting the page
from this function's perspective.

> Perhaps adding something along this line?
> 
> 	WARN_ON(!irqs_disabled());

Given that every use of hyperv_pcpu_input_arg except hv_common_cpu_init() disables
IRQs, what about adding a helper to retrieve the pointer and assert that IRQs are
disabled?  I.e. add the sanity for all usage, not just this one-off case.

And since CPUHP_AP_ONLINE_DYN => hv_common_cpu_init() runs after scheduling is
activated by CPUHP_AP_SCHED_WAIT_EMPTY, I believe that hv_common_cpu_init() is
theoretically broken.  Maybe someone can look at that when fixing he KVM vs.
Hyper-V issue?

https://lore.kernel.org/linux-hyperv/878rkqr7ku.fsf@ovpn-192-136.brq.redhat.com
https://lore.kernel.org/all/87sfikmuop.fsf@redhat.com