linux-kernel - Re: [PATCH] pci/mmap: add pci device EBUSY check

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

Date:   Wed, 8 Feb 2023 15:43:09 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Seunggyun Lee <sglee97@...kook.ac.kr>, bhelgaas@...gle.com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Cornelia Huck <cohuck@...hat.com>, kvm@...r.kernel.org,
        Leon Romanovsky <leon@...nel.org>
Subject: Re: [PATCH] pci/mmap: add pci device EBUSY check

On Wed, 8 Feb 2023 16:10:10 -0600
Bjorn Helgaas <helgaas@...nel.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@...kook.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