[PATCH] check mapped ranges on sysfs resource files (for 2.6.27)

[PATCH] check mapped ranges on sysfs resource files (for 2.6.27)

[Jesse Barnes]

I'm sending a patch rather than a pull request since it's just this.
Totally up to you (obviously) whether we stuff this into 2.6.27 or hold
off on it until 2.6.28.

It's fairly common for applications to map PCI resources through sysfs.
However, with the current implementation, it's possible for an application
to map far more than the range corresponding to the resourceN file it
opened.  This patch plugs that hole by checking the range at mmap time,
similar to what is done on platforms like sparc64 in their lower level
PCI remapping routines.

It was initially put together to help debug the e1000e NVRAM corruption
problem, since we initially thought an X driver might be walking past the
end of one of its mappings and clobbering the NVRAM.  It now looks like
that's not the case, but doing the check is still important for obvious
reasons.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>


diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9c71858..4d1aa6e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -16,6 +16,7 @@
 
 
 #include <linux/kernel.h>
+#include <linux/sched.h>
 #include <linux/pci.h>
 #include <linux/stat.h>
 #include <linux/topology.h>
@@ -502,6 +503,8 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
 	struct resource *res = (struct resource *)attr->private;
 	enum pci_mmap_state mmap_type;
 	resource_size_t start, end;
+	unsigned long map_len = vma->vm_end - vma->vm_start;
+	unsigned long map_offset = vma->vm_pgoff << PAGE_SHIFT;
 	int i;
 
 	for (i = 0; i < PCI_ROM_RESOURCE; i++)
@@ -510,6 +513,17 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
 	if (i >= PCI_ROM_RESOURCE)
 		return -ENODEV;
 
+	/*
+	 * Make sure the range the user is trying to map falls within
+	 * the resource
+	 */
+	if (map_offset + map_len > pci_resource_len(pdev, i)) {
+		WARN(1, "process \"%s\" tried to map 0x%08lx-0x%08lx on BAR %d (size 0x%08lx)\n",
+		     current->comm, map_offset, map_offset + map_len, i,
+		     (unsigned long)pci_resource_len(pdev, i));
+		return -EINVAL;
+	}
+
 	/* pci_mmap_page_range() expects the same kind of entry as coming
 	 * from /proc/bus/pci/ which is a "user visible" value. If this is
 	 * different from the resource itself, arch will do necessary fixup.

Re: [PATCH] check mapped ranges on sysfs resource files (for 2.6.27)

[Linus Torvalds]

On Thu, 2 Oct 2008, Jesse Barnes wrote:
>
> +	unsigned long map_len = vma->vm_end - vma->vm_start;
> +	unsigned long map_offset = vma->vm_pgoff << PAGE_SHIFT;

This seems broken for big vm_pgoff values, where that shift will 
potentially overflow, no? 

Also, it strikes me that we don't seem to check that the resource start is 
page-aligned. We just do

	vma->vm_pgoff += start >> PAGE_SHIFT;

without checking if we just dropped low bits from 'start'.

Of course, if the length of the resource is bigger than a page, I guess 
the resource is guaranteed to be at least page-aligned, so maybe the 
length check - if it was correct - would be sufficient.

Anyway, it would be *much* better to do the length check in pages rather 
than in bytes, to avoid the overflow condition.

Can somebody test if something like this works? It also prints the actual 
name of the device, not just a random BAR number (but it will print 
everyting in PFN's, I hate potentially losing information).

			Linus

---
 drivers/pci/pci-sysfs.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9c71858..77baff0 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -16,6 +16,7 @@
 
 
 #include <linux/kernel.h>
+#include <linux/sched.h>
 #include <linux/pci.h>
 #include <linux/stat.h>
 #include <linux/topology.h>
@@ -484,6 +485,21 @@ pci_mmap_legacy_mem(struct kobject *kobj, struct bin_attribute *attr,
 #endif /* HAVE_PCI_LEGACY */
 
 #ifdef HAVE_PCI_MMAP
+
+static int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma)
+{
+	unsigned long nr, start, size;
+
+	nr = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+	start = vma->vm_pgoff;
+	size = pci_resource_len(pdev, resno) >> PAGE_SHIFT;
+	if (start < size && size - start >= nr)
+		return 1;
+	WARN(1, "process \"%s\" tried to map 0x%08lx-0x%08lx on %s BAR %d (size 0x%08lx)\n",
+		current->comm, start, start+nr, pci_name(pdev), resno, size);
+	return 0;
+}
+
 /**
  * pci_mmap_resource - map a PCI resource into user memory space
  * @kobj: kobject for mapping
@@ -510,6 +526,9 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
 	if (i >= PCI_ROM_RESOURCE)
 		return -ENODEV;
 
+	if (!pci_mmap_fits(pdev, i, vma))
+		return -EINVAL;
+
 	/* pci_mmap_page_range() expects the same kind of entry as coming
 	 * from /proc/bus/pci/ which is a "user visible" value. If this is
 	 * different from the resource itself, arch will do necessary fixup.

Re: [PATCH] check mapped ranges on sysfs resource files (for 2.6.27)

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

On Thursday, October 2, 2008 4:30 pm Linus Torvalds wrote:
> On Thu, 2 Oct 2008, Jesse Barnes wrote:
> > +	unsigned long map_len = vma->vm_end - vma->vm_start;
> > +	unsigned long map_offset = vma->vm_pgoff << PAGE_SHIFT;
>
> This seems broken for big vm_pgoff values, where that shift will
> potentially overflow, no?

Ah yes, exactly what we want to be checking for in fact.

> Also, it strikes me that we don't seem to check that the resource start is
> page-aligned. We just do
>
> 	vma->vm_pgoff += start >> PAGE_SHIFT;
>
> without checking if we just dropped low bits from 'start'.

Hm, yeah looks like that's a long standing issue...

> Of course, if the length of the resource is bigger than a page, I guess
> the resource is guaranteed to be at least page-aligned, so maybe the
> length check - if it was correct - would be sufficient.
>
> Anyway, it would be *much* better to do the length check in pages rather
> than in bytes, to avoid the overflow condition.
>
> Can somebody test if something like this works? It also prints the actual
> name of the device, not just a random BAR number (but it will print
> everyting in PFN's, I hate potentially losing information).

Yeah, looks much better.  I was using this silly test program to see if the 
earlier code worked.  Just pass in both valid and invalid sizes.

Jesse

[-- Attachment #2: sysfs-mmap-test.c --]
[-- Type: text/x-csrc, Size: 825 bytes --]

#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/types.h>

int main(int argc, char *argv[])
{
	size_t len;
	int fd;
	void *ptr;

	if (argc != 3) {
		fprintf(stderr, "usage: %s <file> <mapsize>\n",
			argv[0]);
		return -1;
	}

	len = atoi(argv[2]);

	fd = open(argv[1], O_RDONLY);
	if (fd == -1) {
		fprintf(stderr, "open failed: %s\n", strerror(errno));
		return errno;
	}

	ptr = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0);
	if (ptr == MAP_FAILED) {
		fprintf(stderr, "mmap failed: %s\n", strerror(errno));
		return errno;
	}

	printf("mmap of %s with size %zd succeeded\n", argv[1], len);

	munmap(ptr, len); /* ignore any errors, we don't care */
	close(fd);
	return 0;
}

Re: [PATCH] check mapped ranges on sysfs resource files (for 2.6.27)

[Linus Torvalds]

On Thu, 2 Oct 2008, Jesse Barnes wrote:
> 
> Yeah, looks much better.  I was using this silly test program to see if the 
> earlier code worked.  Just pass in both valid and invalid sizes.

All right, it seems to pass your test program, so I committed it with your 
ack and quoting your original reason for doing this.

		Linus