Date:   Wed, 8 Feb 2023 15:43:09 -0700
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Seunggyun Lee <sglee97@dankook.ac.kr>, bhelgaas@google.com,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        Cornelia Huck <cohuck@redhat.com>, kvm@vger.kernel.org,
        Leon Romanovsky <leon@kernel.org>
Subject: Re: [PATCH] pci/mmap: add pci device EBUSY check
Message-ID: <20230208154309.2bd82a15.alex.williamson@redhat.com>
In-Reply-To: <20230208221010.GA2489371@bhelgaas>
References: <20230207113949.17943-1-sglee97@dankook.ac.kr>
        <20230208221010.GA2489371@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 8 Feb 2023 16:10:10 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc VFIO folks, Leon]
> 
> On Tue, Feb 07, 2023 at 08:39:49PM +0900, Seunggyun Lee wrote:
> > When using a pci device through the vfio-pci driver, other software was
> > also able to access the pci device memory through sysfs.
> > 
> > To prevent this, when mmap is performed through sysfs, a process of
> > checking whether the device is in use is added.
> > 
> > Signed-off-by: Seunggyun Lee <sglee97@dankook.ac.kr>
> > ---
> >  drivers/pci/mmap.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/pci/mmap.c b/drivers/pci/mmap.c
> > index 4504039056d1..4c9df2e23e03 100644
> > --- a/drivers/pci/mmap.c
> > +++ b/drivers/pci/mmap.c
> > @@ -25,6 +25,8 @@ int pci_mmap_resource_range(struct pci_dev *pdev, int bar,
> >  {
> >  	unsigned long size;
> >  	int ret;
> > +	if (pdev->driver)

Maintain the blank line after variable declarations.

> > +		return -1;

Surely there's a better errno value for this.

> >  
> >  	size = ((pci_resource_len(pdev, bar) - 1) >> PAGE_SHIFT) + 1;
> >  	if (vma->vm_pgoff + vma_pages(vma) > size)

Regardless of the above, what's the point of this?  There are already
checks for LOCKDOWN_PCI_ACCESS in the sysfs and proc interfaces to this
function, so we can already activate restrictions to protect this
scenario via kernel config, kernel cmdline options, or runtime with
securityfs.  This is redundant and a blanket restriction as implemented
here seems liable to break some obscure use case.  Thanks,

Alex