diff mbox series

mm/mmap: Fix MAP_FIXED address return on VMA merge

Message ID 20221018191613.4133459-1-Liam.Howlett@oracle.com
State New
Headers
Series mm/mmap: Fix MAP_FIXED address return on VMA merge |

Commit Message

Liam R. Howlett Oct. 18, 2022, 7:17 p.m. UTC 
  mmap should return the start address of newly mapped area when
successful.  On a successful merge of a VMA, the return address was
changed and thus was violating that expectation from userspace.

This is a restoration of functionality provided by 309d08d9b3a3
(mm/mmap.c: fix mmap return value when vma is merged after call_mmap()).
For completeness of fixing MAP_FIXED, implement the comments from the
previous discussion to never update the address and fail if the address
changes.  Leaving the error as a WARN_ON() to avoid crashing the kernel.

Cc: Liu Zixian <liuzixian4@huawei.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Matthew Wilcox <willy@infradead.org>
Link: https://lore.kernel.org/all/Y06yk66SKxlrwwfb@lakrids/
Link: https://lore.kernel.org/all/20201203085350.22624-1-liuzixian4@huawei.com/
Fixes: 4dd1b84140c1 (mm/mmap: use advanced maple tree API for mmap_region())
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/mmap.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

David Hildenbrand Oct. 18, 2022, 7:27 p.m. UTC | #1 
On 18.10.22 21:17, Liam Howlett wrote:
> mmap should return the start address of newly mapped area when
> successful.  On a successful merge of a VMA, the return address was
> changed and thus was violating that expectation from userspace.
> 

Just wondering, do we have a simple user space reproducer / test?

Do we want to add some more tests for such scenarios?

> This is a restoration of functionality provided by 309d08d9b3a3
> (mm/mmap.c: fix mmap return value when vma is merged after call_mmap()).
> For completeness of fixing MAP_FIXED, implement the comments from the
> previous discussion to never update the address and fail if the address
> changes.  Leaving the error as a WARN_ON() to avoid crashing the kernel.
> 
> Cc: Liu Zixian <liuzixian4@huawei.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Link: https://lore.kernel.org/all/Y06yk66SKxlrwwfb@lakrids/
> Link: https://lore.kernel.org/all/20201203085350.22624-1-liuzixian4@huawei.com/
> Fixes: 4dd1b84140c1 (mm/mmap: use advanced maple tree API for mmap_region())
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
>   mm/mmap.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 42cd2c260898..22010e13f1a1 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2625,14 +2625,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>   		if (error)
>   			goto unmap_and_free_vma;
>   
> -		/* Can addr have changed??
> -		 *
> -		 * Answer: Yes, several device drivers can do it in their
> -		 *         f_op->mmap method. -DaveM
> +		/*
> +		 * Expansion is handled above, merging is handled below.
> +		 * Drivers should not alter the address of the VMA.
>   		 */
> -		WARN_ON_ONCE(addr != vma->vm_start);
> -
> -		addr = vma->vm_start;
> +		if (WARN_ON((addr != vma->vm_start))) {
> +			error = -EINVAL;
> +			goto close_and_free_vma;
> +		}

If this is something that user space can trigger, WARN_* is the wrong 
choice. But what I understand from the comment change is that this must 
not happen at that point unless there is a real issue.

Why not "if (WARN_ON_ONCE)" ?
Matthew Wilcox Oct. 18, 2022, 7:58 p.m. UTC | #2 
On Tue, Oct 18, 2022 at 09:27:11PM +0200, David Hildenbrand wrote:
> > +		if (WARN_ON((addr != vma->vm_start))) {
> > +			error = -EINVAL;
> > +			goto close_and_free_vma;
> > +		}
> 
> If this is something that user space can trigger, WARN_* is the wrong
> choice. But what I understand from the comment change is that this must not
> happen at that point unless there is a real issue.

It's something that a device driver can trigger.  So if userspace calls
mmap(MAP_FIXED) and the driver decides to overwrite the vma->vm_start,
it'll trigger.  I think WARN_ON() is the right choice.

> Why not "if (WARN_ON_ONCE)" ?

Because by it's nature it's not going to trigger a bajillion times in
quick succession.
Liam R. Howlett Oct. 18, 2022, 8:25 p.m. UTC | #3 
* David Hildenbrand <david@redhat.com> [221018 15:27]:
> On 18.10.22 21:17, Liam Howlett wrote:
> > mmap should return the start address of newly mapped area when
> > successful.  On a successful merge of a VMA, the return address was
> > changed and thus was violating that expectation from userspace.
> > 
> 
> Just wondering, do we have a simple user space reproducer / test?

Not today, no.

> 
> Do we want to add some more tests for such scenarios?

Yes, I will code something up for LTP.

> 
> > This is a restoration of functionality provided by 309d08d9b3a3
> > (mm/mmap.c: fix mmap return value when vma is merged after call_mmap()).
> > For completeness of fixing MAP_FIXED, implement the comments from the
> > previous discussion to never update the address and fail if the address
> > changes.  Leaving the error as a WARN_ON() to avoid crashing the kernel.
> > 
> > Cc: Liu Zixian <liuzixian4@huawei.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Jason Gunthorpe <jgg@nvidia.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Link: https://lore.kernel.org/all/Y06yk66SKxlrwwfb@lakrids/
> > Link: https://lore.kernel.org/all/20201203085350.22624-1-liuzixian4@huawei.com/
> > Fixes: 4dd1b84140c1 (mm/mmap: use advanced maple tree API for mmap_region())
> > Reported-by: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > ---
> >   mm/mmap.c | 15 +++++++--------
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 42cd2c260898..22010e13f1a1 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2625,14 +2625,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >   		if (error)
> >   			goto unmap_and_free_vma;
> > -		/* Can addr have changed??
> > -		 *
> > -		 * Answer: Yes, several device drivers can do it in their
> > -		 *         f_op->mmap method. -DaveM
> > +		/*
> > +		 * Expansion is handled above, merging is handled below.
> > +		 * Drivers should not alter the address of the VMA.
> >   		 */
> > -		WARN_ON_ONCE(addr != vma->vm_start);
> > -
> > -		addr = vma->vm_start;
> > +		if (WARN_ON((addr != vma->vm_start))) {
> > +			error = -EINVAL;
> > +			goto close_and_free_vma;
> > +		}
> 
> If this is something that user space can trigger, WARN_* is the wrong
> choice. But what I understand from the comment change is that this must not
> happen at that point unless there is a real issue.

The VMA start address could be changed in call_mmap() which is a driver
call.  I guess someone could write a driver to mmap by a users action?
I don't think it can be reached other ways.  In any case, I'm changing a
WARN_ON_ONCE() to a WARN_ON() and undoing the badness instead of
marching forwards.

> 
> Why not "if (WARN_ON_ONCE)" ?

I was thinking it was harder to ignore if it happen more frequently?
There isn't a driver that does this now, but I'm not picky over which
variant to call.

Thanks,
Liam
David Hildenbrand Oct. 19, 2022, 2:08 p.m. UTC | #4 
>>>    mm/mmap.c | 15 +++++++--------
>>>    1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index 42cd2c260898..22010e13f1a1 100644
>>> --- a/mm/mmap.c
>>> +++ b/mm/mmap.c
>>> @@ -2625,14 +2625,14 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>>>    		if (error)
>>>    			goto unmap_and_free_vma;
>>> -		/* Can addr have changed??
>>> -		 *
>>> -		 * Answer: Yes, several device drivers can do it in their
>>> -		 *         f_op->mmap method. -DaveM
>>> +		/*
>>> +		 * Expansion is handled above, merging is handled below.
>>> +		 * Drivers should not alter the address of the VMA.
>>>    		 */
>>> -		WARN_ON_ONCE(addr != vma->vm_start);
>>> -
>>> -		addr = vma->vm_start;
>>> +		if (WARN_ON((addr != vma->vm_start))) {
>>> +			error = -EINVAL;
>>> +			goto close_and_free_vma;
>>> +		}
>>
>> If this is something that user space can trigger, WARN_* is the wrong
>> choice. But what I understand from the comment change is that this must not
>> happen at that point unless there is a real issue.
> 
> The VMA start address could be changed in call_mmap() which is a driver
> call.  I guess someone could write a driver to mmap by a users action?
> I don't think it can be reached other ways.  In any case, I'm changing a
> WARN_ON_ONCE() to a WARN_ON() and undoing the badness instead of
> marching forwards.

WARN_ON_ONCE() can also be used in conditionals if that's what you were 
concerned about, but ...

> 
>>
>> Why not "if (WARN_ON_ONCE)" ?
> 
> I was thinking it was harder to ignore if it happen more frequently?
> There isn't a driver that does this now, but I'm not picky over which
> variant to call.

.. I think the assumption really is that we won't see (m)any these 
calls. And if we do, it's a bad bad driver. So WARN_ON() might be just fine.

If this would be easy to trigger by any user space, WARN* would have 
been the wrong choice, that's why I was asking.
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 42cd2c260898..22010e13f1a1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2625,14 +2625,14 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 		if (error)
 			goto unmap_and_free_vma;
 
-		/* Can addr have changed??
-		 *
-		 * Answer: Yes, several device drivers can do it in their
-		 *         f_op->mmap method. -DaveM
+		/*
+		 * Expansion is handled above, merging is handled below.
+		 * Drivers should not alter the address of the VMA.
 		 */
-		WARN_ON_ONCE(addr != vma->vm_start);
-
-		addr = vma->vm_start;
+		if (WARN_ON((addr != vma->vm_start))) {
+			error = -EINVAL;
+			goto close_and_free_vma;
+		}
 		mas_reset(&mas);
 
 		/*
@@ -2654,7 +2654,6 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 				vm_area_free(vma);
 				vma = merge;
 				/* Update vm_flags to pick up the change. */
-				addr = vma->vm_start;
 				vm_flags = vma->vm_flags;
 				goto unmap_writable;
 			}