[PATCH] pci/mmap: add pci device EBUSY check

[PATCH] pci/mmap: add pci device EBUSY check

[Seunggyun Lee]

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)
+		return -1;
 
 	size = ((pci_resource_len(pdev, bar) - 1) >> PAGE_SHIFT) + 1;
 	if (vma->vm_pgoff + vma_pages(vma) > size)
-- 
2.25.1

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

[Leon Romanovsky]

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.

And why is it wrong?

> 
> 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)
> +		return -1;

I doubt that it is correct/needed as every call to
pci_mmap_resource_range() is guarded by iomem_is_exclusive() check.

Also I'm not sure that pdev->driver can be accessed without any lock in
this flow.

Thanks

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

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

[Bjorn Helgaas]

[+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)
> +		return -1;
>  
>  	size = ((pci_resource_len(pdev, bar) - 1) >> PAGE_SHIFT) + 1;
>  	if (vma->vm_pgoff + vma_pages(vma) > size)
> -- 
> 2.25.1
> 

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

[Alex Williamson]

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