* [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge
@ 2022-10-18 19:17 Liam Howlett
2022-10-18 19:27 ` David Hildenbrand
0 siblings, 1 reply; 5+ messages in thread
From: Liam Howlett @ 2022-10-18 19:17 UTC (permalink / raw)
To: maple-tree, linux-mm, linux-kernel, Andrew Morton
Cc: Liam Howlett, Liu Zixian, David Hildenbrand, Jason Gunthorpe,
Matthew Wilcox, Mark Rutland
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(-)
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;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge
2022-10-18 19:17 [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge Liam Howlett
@ 2022-10-18 19:27 ` David Hildenbrand
2022-10-18 19:58 ` Matthew Wilcox
2022-10-18 20:25 ` Liam Howlett
0 siblings, 2 replies; 5+ messages in thread
From: David Hildenbrand @ 2022-10-18 19:27 UTC (permalink / raw)
To: Liam Howlett, maple-tree, linux-mm, linux-kernel, Andrew Morton
Cc: Liu Zixian, Jason Gunthorpe, Matthew Wilcox, Mark Rutland
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)" ?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge
2022-10-18 19:27 ` David Hildenbrand
@ 2022-10-18 19:58 ` Matthew Wilcox
2022-10-18 20:25 ` Liam Howlett
1 sibling, 0 replies; 5+ messages in thread
From: Matthew Wilcox @ 2022-10-18 19:58 UTC (permalink / raw)
To: David Hildenbrand
Cc: Liam Howlett, maple-tree, linux-mm, linux-kernel, Andrew Morton,
Liu Zixian, Jason Gunthorpe, Mark Rutland
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge
2022-10-18 19:27 ` David Hildenbrand
2022-10-18 19:58 ` Matthew Wilcox
@ 2022-10-18 20:25 ` Liam Howlett
2022-10-19 14:08 ` David Hildenbrand
1 sibling, 1 reply; 5+ messages in thread
From: Liam Howlett @ 2022-10-18 20:25 UTC (permalink / raw)
To: David Hildenbrand
Cc: maple-tree, linux-mm, linux-kernel, Andrew Morton, Liu Zixian,
Jason Gunthorpe, Matthew Wilcox, Mark Rutland
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge
2022-10-18 20:25 ` Liam Howlett
@ 2022-10-19 14:08 ` David Hildenbrand
0 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2022-10-19 14:08 UTC (permalink / raw)
To: Liam Howlett
Cc: maple-tree, linux-mm, linux-kernel, Andrew Morton, Liu Zixian,
Jason Gunthorpe, Matthew Wilcox, Mark Rutland
>>> 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.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-19 14:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-18 19:17 [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge Liam Howlett
2022-10-18 19:27 ` David Hildenbrand
2022-10-18 19:58 ` Matthew Wilcox
2022-10-18 20:25 ` Liam Howlett
2022-10-19 14:08 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).