[patch] mmap bug with drivers that adjust vm_start

[patch] mmap bug with drivers that adjust vm_start

[Benjamin LaHaise]

Hello all,

The patch below fixes a problem whereby a vma which has its vm_start 
address changed by the file's mmap operation can result in the vma 
being inserted into the wrong location within the vma tree.  This 
results in page faults not being handled correctly leading to SEGVs, 
as well as various BUG()s hitting on exit of the mm.  The fix is to 
recalculate the insertion point when we know the address has changed.  
Comments?  Patch is against 2.4.19-pre4.

		-ben
-- 
"A man with a bass just walked in,
 and he's putting it down
 on the floor."

:r ~/patches/v2.4.19-pre4-mmap_fix.diff
--- retest.3/mm/mmap.c.org	Mon Mar 25 19:38:10 2002
+++ retest.3/mm/mmap.c	Mon Mar 25 22:40:40 2002
@@ -548,7 +548,14 @@
 	 * Answer: Yes, several device drivers can do it in their
 	 *         f_op->mmap method. -DaveM
 	 */
-	addr = vma->vm_start;
+	if (addr != vma->vm_start) {
+		/* Since addr changed, we rely on the mmap op to prevent 
+		 * collisions with existing vmas and just use find_vma_prepare 
+		 * to update the tree pointers.
+		 */
+		addr = vma->vm_start;
+		find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
+	}
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	if (correct_wcount)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [patch] mmap bug with drivers that adjust vm_start

[David S. Miller]

   The patch below fixes a problem whereby a vma which has its vm_start 
   address changed by the file's mmap operation can result in the vma 
   being inserted into the wrong location within the vma tree.  This 
   results in page faults not being handled correctly leading to SEGVs, 
   as well as various BUG()s hitting on exit of the mm.  The fix is to 
   recalculate the insertion point when we know the address has changed.  
   Comments?  Patch is against 2.4.19-pre4.

Good catch.  Most of the time this happened to work because the driver
filled in the page tables completely (as is the case for most video
etc. drivers which use {io_,}remap_page_range et al..)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [patch] mmap bug with drivers that adjust vm_start

[Andrea Arcangeli]

On Mon, Mar 25, 2002 at 11:00:47PM -0500, Benjamin LaHaise wrote:
> Hello all,
> 
> The patch below fixes a problem whereby a vma which has its vm_start 
> address changed by the file's mmap operation can result in the vma 
> being inserted into the wrong location within the vma tree.  This 
> results in page faults not being handled correctly leading to SEGVs, 
> as well as various BUG()s hitting on exit of the mm.  The fix is to 
> recalculate the insertion point when we know the address has changed.  
> Comments?  Patch is against 2.4.19-pre4.

The patch is obviously safe.

However if the patch is needed it means the ->mmap also must do the
do_munmap stuff by hand internally, which is very ugly given we also did
our own do_munmap in a completly different region (the one requested by
the user). Our do_munmap should not happen if we place the mapping
elsewhere. If possible I would prefer to change those drivers to
advertise their enforced vm_start with a proper callback, the current
way is halfway broken still. BTW, which are those drivers, and why they
needs to enforce a certain vm_start (also despite MAP_FIXED that they
cannot check within the ->mmap callback)?

Andrea
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [patch] mmap bug with drivers that adjust vm_start

[Benjamin LaHaise]

On Tue, Mar 26, 2002 at 05:42:36PM +0100, Andrea Arcangeli wrote:
> However if the patch is needed it means the ->mmap also must do the
> do_munmap stuff by hand internally, which is very ugly given we also did
> our own do_munmap in a completly different region (the one requested by
> the user).

At least my own code checks for that and fails if there is a mapping 
already placed at the fixed address it needs to use.  If we're paranoid, 
we could BUG() on getting a vma back from the new find_vma_prepare call.

> Our do_munmap should not happen if we place the mapping
> elsewhere. If possible I would prefer to change those drivers to
> advertise their enforced vm_start with a proper callback, the current
> way is halfway broken still. BTW, which are those drivers, and why they
> needs to enforce a certain vm_start (also despite MAP_FIXED that they
> cannot check within the ->mmap callback)?

Video drivers, others that require specific alignment (4MB pages for 
example).  Historically, the mmap call has been the hook for doing this, 
hence the comment in do_mmap from davem.  Unless there's a really good 
reason for changing the hook, I don't see doing so as providing much 
benefit other than making source compatibility hard.

		-ben
-- 
"A man with a bass just walked in,
 and he's putting it down
 on the floor."
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [patch] mmap bug with drivers that adjust vm_start

[Andrea Arcangeli]

On Tue, Mar 26, 2002 at 01:57:03PM -0500, Benjamin LaHaise wrote:
> On Tue, Mar 26, 2002 at 05:42:36PM +0100, Andrea Arcangeli wrote:
> > However if the patch is needed it means the ->mmap also must do the
> > do_munmap stuff by hand internally, which is very ugly given we also did
> > our own do_munmap in a completly different region (the one requested by
> > the user).
> 
> At least my own code checks for that and fails if there is a mapping 
> already placed at the fixed address it needs to use.  If we're paranoid, 

Ok, so it's safe.

> we could BUG() on getting a vma back from the new find_vma_prepare call.

yes, it sounds a good idea to verify there's no other mapping in the way
of the relocation (until a better fix is implemented), it's a slow path
so we won't hurt performance.

> 
> > Our do_munmap should not happen if we place the mapping
> > elsewhere. If possible I would prefer to change those drivers to
> > advertise their enforced vm_start with a proper callback, the current
> > way is halfway broken still. BTW, which are those drivers, and why they
> > needs to enforce a certain vm_start (also despite MAP_FIXED that they
> > cannot check within the ->mmap callback)?
> 
> Video drivers, others that require specific alignment (4MB pages for 
> example).  Historically, the mmap call has been the hook for doing this, 
> hence the comment in do_mmap from davem.  Unless there's a really good 
> reason for changing the hook, I don't see doing so as providing much 
> benefit other than making source compatibility hard.

The good reason, is that currently we're literally corrupting the
userspace with the senseless do_munmap call in the add<->addr+len area
before the ->mmap lowlevel callback. And such an munmap is certainly not
required to maintain source and binary compatibility (otherwise it would
be insane in the first place :).

Andrea
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [patch] mmap bug with drivers that adjust vm_start

[Benjamin LaHaise]

On Tue, Mar 26, 2002 at 08:15:02PM +0100, Andrea Arcangeli wrote:
> > we could BUG() on getting a vma back from the new find_vma_prepare call.
> 
> yes, it sounds a good idea to verify there's no other mapping in the way
> of the relocation (until a better fix is implemented), it's a slow path
> so we won't hurt performance.

Okay, updated patch for this is below.  I also added a printk to give us 
which mmap operation was invoked to aid in debugging.

> The good reason, is that currently we're literally corrupting the
> userspace with the senseless do_munmap call in the add<->addr+len area
> before the ->mmap lowlevel callback. And such an munmap is certainly not
> required to maintain source and binary compatibility (otherwise it would
> be insane in the first place :).

Ah, right.  I disallow MAP_FIXED addresses that are not the "correct" offset, 
so this case would fail, albeit with the do_munmap occurring.  Personally, 
I would rather see an mmap fail if it would collide with an existing mapping, 
but that might break some applications.

Thanks for looking this over.  Cheers,

		-ben
-- 
"A man with a bass just walked in,
 and he's putting it down
 on the floor."

:r ~/patches/v2.4.19-pre4-mmap_fix-2.diff
--- retest.3/mm/mmap.c.org	Mon Mar 25 19:38:10 2002
+++ retest.3/mm/mmap.c	Tue Mar 26 15:01:47 2002
@@ -14,6 +14,7 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/personality.h>
+#include <linux/compiler.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgalloc.h>
@@ -548,7 +549,19 @@
 	 * Answer: Yes, several device drivers can do it in their
 	 *         f_op->mmap method. -DaveM
 	 */
-	addr = vma->vm_start;
+	if (addr != vma->vm_start) {
+		/* Since addr changed, we rely on the mmap op to prevent 
+		 * collisions with existing vmas and just use find_vma_prepare 
+		 * to update the tree pointers.
+		 */
+		addr = vma->vm_start;
+		if (unlikely(NULL != find_vma_prepare(mm, addr, &prev,
+						&rb_link, &rb_parent))) {
+			printk(KERN_ERR "buggy mmap operation: [<%p>]\n",
+				file ? file->f_op->mmap : NULL);
+			BUG();
+		}
+	}
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	if (correct_wcount)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: [patch] mmap bug with drivers that adjust vm_start

[Andrea Arcangeli]

On Tue, Mar 26, 2002 at 03:43:45PM -0500, Benjamin LaHaise wrote:
> I would rather see an mmap fail if it would collide with an existing mapping, 
> but that might break some applications.

That doesn't make it easier though, the time we should fail is still way
earlier the relocation of the virtual address space area.

> Thanks for looking this over.  Cheers,

You're welcome, thanks for fixing it.

> +		if (unlikely(NULL != find_vma_prepare(mm, addr, &prev,
> +						&rb_link, &rb_parent))) {

this looks wrong there may very well be a vma after our mapping but it
doesn't necessairly overlap with us (also the unlikely would make more
sense in the previous if). I hacked a bit on top of your previous patch,
this looks ok to me but it's untested:

diff -urN 2.4.19pre4aa1/mm/mmap.c vma/mm/mmap.c
--- 2.4.19pre4aa1/mm/mmap.c	Tue Mar 26 20:43:07 2002
+++ vma/mm/mmap.c	Tue Mar 26 20:48:24 2002
@@ -554,7 +554,26 @@
 	 * Answer: Yes, several device drivers can do it in their
 	 *         f_op->mmap method. -DaveM
 	 */
-	addr = vma->vm_start;
+	if (unlikely(addr != vma->vm_start)) {
+		/*
+		 * It is a bit too late to pretend changing the virtual
+		 * area of the mapping, we just corrupted userspace
+		 * in the do_munmap, so FIXME (not in 2.4 to avoid breaking
+		 * the driver API).
+		 */
+		struct vm_area_struct * stale_vma;
+		/* Since addr changed, we rely on the mmap op to prevent 
+		 * collisions with existing vmas and just use find_vma_prepare 
+		 * to update the tree pointers.
+		 */
+		addr = vma->vm_start;
+		stale_vma = find_vma_prepare(mm, addr, &prev, &rb_link, &rb_parent);
+		/*
+		 * Make sure the lowlevel driver did its job right.
+		 */
+		if (unlikely(stale_vma && stale_vma->vm_start < vma->vm_end))
+			BUG();
+	}
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 	if (correct_wcount)


your printk also is valid debugging trick, I think it won't be necessary
most of the time but feel free to re-add it if you like.

Andrea
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/