From: Dan Williams <email@example.com> To: Greg Kroah-Hartman <firstname.lastname@example.org> Cc: Bjorn Helgaas <email@example.com>, Bjorn Helgaas <firstname.lastname@example.org>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, Linux PCI <email@example.com>, Linux Kernel Mailing List <firstname.lastname@example.org>, Christoph Hellwig <email@example.com> Subject: Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions Date: Thu, 13 May 2021 14:28:01 -0700 Message-ID: <CAPcyv4iepHgyfruQVi9xNYfrD=7fAQfU=mCeYzcfDSNkASz5vQ@mail.gmail.com> (raw) In-Reply-To: <YF8NGeGv9vYcMfTV@kroah.com> On Sat, Mar 27, 2021 at 3:47 AM Greg Kroah-Hartman <firstname.lastname@example.org> wrote: > > On Fri, Mar 26, 2021 at 11:12:47AM -0500, Bjorn Helgaas wrote: > > [+cc Christoph] > > > > On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote: > > > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over > > > configuration cycles. It assumes one initiator at a time is > > > reading/writing the data registers. If userspace reads from the response > > > data payload it may steal data that a kernel driver was expecting to > > > read. If userspace writes to the request payload it may corrupt the > > > request a driver was trying to send. > > > > IIUC the problem we're talking about is that userspace config access, > > e.g., via "lspci" or "setpci" may interfere with kernel usage of DOE. > > I attached what I think are the relevant bits from the spec. > > > > It looks to me like config *reads* should not be a problem: A read of > > Write Data Mailbox always returns 0 and looks innocuous. A userspace > > read of Read Data Mailbox may return a DW of the data object, but it > > doesn't advance the cursor, so it shouldn't interfere with a kernel > > read. > > > > A write to Write Data Mailbox could obviously corrupt an object being > > written to the device. A config write to Read Data Mailbox *does* > > advance the cursor, so that would definitely interfere with a kernel > > user. > > > > So I think we're really talking about an issue with "setpci" and I > > don't expect "lspci" to be a problem. "setpci" is a valuable tool, > > and the fact that it can hose your system is not really news. I don't > > know how hard we should work to protect against that. > > Thanks for looking this up and letting us know. > > So this should be fine, reads are ok, it's not as crazy of a protocol > design as Dan alluded to, so the kernel should be ok. No need to add > additional "protection" here at all, if you run setpci from userspace, > you get what you asked for :) > Circling back to this after thinking of the implications and looking at the review of the DOE code, this situation is different than your typical "userspace gets to keep the pieces if it does a configuration write". If we assume well behaved non-malicious userpace, it has no reason to muck with critical config registers. If userspace changes a BAR value and the system fails, yup, that's its own fault. The DOE mailbox is different. There are legitimate reasons why non-broken userspace would want to read some DOE payloads while the kernel is retrieving its payloads. It also simplifies the kernel implementation if it does need to worry about other agents interrupting its transfers. My mistake was making this restriction apply to reads, but I'm not on the same page that blocking writes is fruitless just because userspace can do other damage with config writes. So either the kernel DOE driver needs to use pci_cfg_access_lock() and revalidate the state of the DOE after acquiring that lock, or it needs to claim the interface completely and provide a driver for userspace to submit requests that can be scheduled in the kernel's DOE queue.
next prev parent reply index Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-25 1:23 Dan Williams 2021-03-25 6:54 ` Greg Kroah-Hartman 2021-03-25 8:29 ` Christoph Hellwig 2021-03-25 17:55 ` Dan Williams 2021-03-26 9:18 ` Greg Kroah-Hartman 2021-03-25 17:43 ` Dan Williams 2021-03-26 9:28 ` Greg Kroah-Hartman 2021-03-26 16:12 ` Bjorn Helgaas 2021-03-27 10:46 ` Greg Kroah-Hartman 2021-05-13 21:28 ` Dan Williams [this message] 2021-03-29 16:46 ` Dan Williams 2021-03-30 12:20 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAPcyv4iepHgyfruQVi9xNYfrD=7fAQfU=mCeYzcfDSNkASz5vQ@mail.gmail.com' \ --email@example.com \ --cc=Jonathan.Cameron@huawei.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-PCI Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \ firstname.lastname@example.org public-inbox-index linux-pci Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci AGPL code for this site: git clone https://public-inbox.org/public-inbox.git