Re: mtd: kernel BUG at arch/x86/mm/pat.c:279!

[Linus Torvalds <torvalds@linux-foundation.org>]

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

On Fri, Sep 7, 2012 at 4:54 PM, Suresh Siddha <suresh.b.siddha@intel.com> wrote:
>
> Essentially the user is trying to mmap at a very large offset (from the
> oops it appears "vma->vm_pgoff << PAGE_SHIFT + start" ends up to
> "0xfffffffffffff000").

Ok, Sasha confirmed that.

> So it appears that the condition "(vma->vm_end - vma->vm_start + off) >
> len" might be false because of the wraparound? and doesn't return
> -EINVAL.

Ack.

Anyway, that means that the BUG_ON() is likely bogus, but so is the
whole calling convention.

The 4kB range starting at 0xfffffffffffff000 sounds like a *valid*
range, but that requires that we fix the calling convention to not
have that "end" (exclusive) thing. It should either be "end"
(inclusive), or just "len".

So it should either be start=0xfffffffffffff000 end=0xffffffffffffffff
or it should be start=0xfffffffffffff000 len=0x1000.

Or we need to say that we must never accept things at the end of the
64-bit range.

Whatever. Something like this (TOTALLY UNTESTED) attached patch should
get the mtdchar overflows to go away, but it does *not* fix the fact
that the MTRR start/end model is broken. It really is technically
valid to have a resource_size_t range of 0xfffffffffffff000+0x1000,
and right now it causes a BUG_ON() in pat.c.

Suresh?

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 2201 bytes --]

 drivers/mtd/mtdchar.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index f2f482bec573..83dd68b80e8e 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -1123,6 +1123,33 @@ static unsigned long mtdchar_get_unmapped_area(struct file *file,
 }
 #endif
 
+static inline unsigned long get_vm_size(struct vm_area_struct *vma)
+{
+	return vma->vm_end - vma->vm_start;
+}
+
+static inline resource_size_t get_vm_offset(struct vm_area_struct *vma)
+{
+	return (resource_size_t) vma->vm_pgoff << PAGE_SHIFT;
+}
+
+/*
+ * Set a new vm offset.
+ *
+ * Verify that the incoming offset really works as a page offset,
+ * and that the offset and size fit in a resource_size_t.
+ */
+static inlint int set_vm_offset(struct vm_area_struct *vma, resource_size_t off)
+{
+	pgoff_t pgoff = off >> PAGE_SHIFT;
+	if (off != (resource_size_t) pgoff << PAGE_SHIFT)
+		return -EINVAL;
+	if (off + get_vm_size(vma) - 1 < off)
+		return -EINVAL;
+	vma->vm_pgoff = pgoff;
+	return 0;
+}
+
 /*
  * set up a mapping for shared memory segments
  */
@@ -1132,20 +1159,29 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma)
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
 	struct map_info *map = mtd->priv;
-	unsigned long start;
-	unsigned long off;
-	u32 len;
+	resource_size_t start, off;
+	unsigned long len, vma_len;
 
 	if (mtd->type == MTD_RAM || mtd->type == MTD_ROM) {
-		off = vma->vm_pgoff << PAGE_SHIFT;
+		off = get_vm_offset(vma);
 		start = map->phys;
 		len = PAGE_ALIGN((start & ~PAGE_MASK) + map->size);
 		start &= PAGE_MASK;
-		if ((vma->vm_end - vma->vm_start + off) > len)
+		vma_len = get_vm_size(vma);
+
+		/* Overflow in off+len? */
+		if (vma_len + off < off)
+			return -EINVAL;
+		/* Does it fit in the mapping? */
+		if (vma_len + off > len)
 			return -EINVAL;
 
 		off += start;
-		vma->vm_pgoff = off >> PAGE_SHIFT;
+		/* Did that overflow? */
+		if (off < start)
+			return -EINVAL;
+		if (set_vm_offset(vma, off) < 0)
+			return -EINVAL;
 		vma->vm_flags |= VM_IO | VM_RESERVED;
 
 #ifdef pgprot_noncached