From: Luxiao Xu <redacted>
Hi all,
This patch addresses an issue in generic_access_phys() where accessing memory
across page boundaries in PFNMAP VMAs assumes physical pages are contiguous,
which can lead to accessing unintended physical memory or exceeding VMA boundaries.
In v3, we address review feedback from David Hildenbrand on v2:
1. Revert changes to __access_remote_vm(): Capping the access length to
(PAGE_SIZE - offset) inside generic_access_phys() is completely sufficient,
as __access_remote_vm() already loops over the requested length and handles
short transfers.
2. Drop the redundant 'len <= 0' check.
3. Add an explanatory comment clarifying that accesses are limited to one page
at a time because follow_pfnmap_start() only validates a single PTE.
4. Regarding min() vs min_t(): min_t(int, ...) is retained because len is a
signed int while PAGE_SIZE is unsigned long ((1UL) << PAGE_SHIFT), which
triggers a compile-time "signedness error" if plain min() is used.
Thanks for the reviews and guidance.
v2 -> v3:
- Drop modification to __access_remote_vm(); capping in generic_access_phys()
is sufficient because the caller loop already handles partial transfers
(David Hildenbrand).
- Drop unnecessary 'len <= 0' check.
- Add comment explaining the single-page limitation (David Hildenbrand).
- v2 Link: https://lore.kernel.org/all/cover.1788531737.git.rakukuip@gmail.com/
v1 -> v2:
- Drop internal multi-page loop in generic_access_phys(); clamp the chunk
size leveraging the existing caller loop (David Hildenbrand).
- Map only PAGE_SIZE in generic_access_phys().
- Add missing (resource_size_t) cast when checking args.pfn (Andrew Morton).
Luxiao Xu (1):
mm/memory: constrain generic_access_phys() to page boundary
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
--
2.43.0
From: Luxiao Xu <redacted>
generic_access_phys() improperly validates the memory access range: it
only validates the start address using follow_pfnmap_start() and passes
PAGE_ALIGN(len + offset) to ioremap_prot().
This poses two problems:
1. In PFNMAP VMAs, consecutive virtual pages are not guaranteed to be
physically contiguous, and individual PTEs may have different access
permissions or writability.
2. The mapping may cross VMA boundaries if len extends beyond vma->vm_end.
Constrain the access in generic_access_phys() to at most the current page
boundary (PAGE_SIZE - offset) and map only a single PAGE_SIZE via
ioremap_prot(). Since the caller __access_remote_vm() already loops over
the requested length and handles partial transfers, it will naturally
iterate over the remaining pages.
Also add a missing (resource_size_t) cast during PFN re-validation to avoid
truncation on 32-bit PAE systems.
Fixes: 9cb12d7b4cca ("mm/memory.c: actually remap enough memory")
Cc: stable@vger.kernel.org
Reported-by: Vega <redacted>
Assisted-by: LLM
Suggested-by: David Hildenbrand <david@kernel.org>
Signed-off-by: Luxiao Xu <redacted>
Signed-off-by: Ren Wei <redacted>
---
v2 -> v3:
- Do not modify __access_remote_vm(); capping in generic_access_phys() is
sufficient because the caller loop already handles partial transfers
(David Hildenbrand).
- Drop unnecessary 'len <= 0' check.
- Add comment explaining the single-page limitation (David Hildenbrand).
- v2 Link: https://lore.kernel.org/all/cover.1788531737.git.rakukuip@gmail.com/
v1 -> v2:
- Drop internal multi-page loop in generic_access_phys(); clamp the chunk
size leveraging the existing caller loop (David Hildenbrand).
- Map only PAGE_SIZE in generic_access_phys().
- Add missing (resource_size_t) cast when checking args.pfn (Andrew Morton).
---
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
From: Luxiao Xu <redacted>
generic_access_phys() improperly validates the memory access range: it
only validates the start address using follow_pfnmap_start() and passes
PAGE_ALIGN(len + offset) to ioremap_prot().
This poses two problems:
1. In PFNMAP VMAs, consecutive virtual pages are not guaranteed to be
physically contiguous, and individual PTEs may have different access
permissions or writability.
2. The mapping may cross VMA boundaries if len extends beyond vma->vm_end.
Constrain the access in generic_access_phys() to at most the current page
boundary (PAGE_SIZE - offset) and map only a single PAGE_SIZE via
ioremap_prot(). Since the caller __access_remote_vm() already loops over
the requested length and handles partial transfers, it will naturally
iterate over the remaining pages.
Also add a missing (resource_size_t) cast during PFN re-validation to avoid
truncation on 32-bit PAE systems.
Thanks.
When fixing a bug, please ensure that the changelog always clearly
describes the userspace-visible runtime effects of that bug.
Especially when proposing a backport.
The cover letter tells us a bit more:
This patch addresses an issue in generic_access_phys() where
accessing memory across page boundaries in PFNMAP VMAs assumes
physical pages are contiguous, which can lead to accessing unintended
physical memory or exceeding VMA boundaries.
But how does this manifest? What does the user see? Has it ever
happened? Is there a Closes:? Any reproducer?
Reported-by: Vega <redacted>
Assisted-by: LLM
Please update your LLM prompts with my above sentence "When fixing...".
Let's get this fixed for the future.
hoo boy we've made a mess of the types in there, but I don't see a
feasible improvement in the context of this patch.
Also, a [0/N] isn't needed or desirable when N==1! I'll consolidate
both into a singleton patch.
From: Luxiao Xu <redacted>
generic_access_phys() improperly validates the memory access range: it
only validates the start address using follow_pfnmap_start() and passes
PAGE_ALIGN(len + offset) to ioremap_prot().
This poses two problems:
1. In PFNMAP VMAs, consecutive virtual pages are not guaranteed to be
physically contiguous, and individual PTEs may have different access
permissions or writability.
2. The mapping may cross VMA boundaries if len extends beyond vma->vm_end.
Constrain the access in generic_access_phys() to at most the current page
boundary (PAGE_SIZE - offset) and map only a single PAGE_SIZE via
ioremap_prot(). Since the caller __access_remote_vm() already loops over
the requested length and handles partial transfers, it will naturally
iterate over the remaining pages.
Also add a missing (resource_size_t) cast during PFN re-validation to avoid
truncation on 32-bit PAE systems.
Fixes: 9cb12d7b4cca ("mm/memory.c: actually remap enough memory")
Cc: stable@vger.kernel.org
Reported-by: Vega <redacted>
Assisted-by: LLM
Suggested-by: David Hildenbrand <david@kernel.org>
Signed-off-by: Luxiao Xu <redacted>
Signed-off-by: Ren Wei <redacted>
---
Yes, LGTM!
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
--
Cheers,
David
From: Luxiao Xu <redacted>
generic_access_phys() improperly validates the memory access range: it
only validates the start address using follow_pfnmap_start() and passes
PAGE_ALIGN(len + offset) to ioremap_prot().
This poses two problems:
1. In PFNMAP VMAs, consecutive virtual pages are not guaranteed to be
physically contiguous, and individual PTEs may have different access
permissions or writability.
2. The mapping may cross VMA boundaries if len extends beyond vma->vm_end.
Constrain the access in generic_access_phys() to at most the current page
boundary (PAGE_SIZE - offset) and map only a single PAGE_SIZE via
ioremap_prot(). Since the caller __access_remote_vm() already loops over
the requested length and handles partial transfers, it will naturally
iterate over the remaining pages.
Also add a missing (resource_size_t) cast during PFN re-validation to avoid
truncation on 32-bit PAE systems.
Thanks.
When fixing a bug, please ensure that the changelog always clearly
describes the userspace-visible runtime effects of that bug.
Especially when proposing a backport.
The cover letter tells us a bit more:
This patch addresses an issue in generic_access_phys() where
accessing memory across page boundaries in PFNMAP VMAs assumes
physical pages are contiguous, which can lead to accessing unintended
physical memory or exceeding VMA boundaries.
But how does this manifest? What does the user see? Has it ever
happened? Is there a Closes:? Any reproducer?
quoted
Reported-by: Vega <redacted>
Assisted-by: LLM
Please update your LLM prompts with my above sentence "When fixing...".
Let's get this fixed for the future.
hoo boy we've made a mess of the types in there, but I don't see a
feasible improvement in the context of this patch.
Also, a [0/N] isn't needed or desirable when N==1! I'll consolidate
both into a singleton patch.
Hi,
Luxiao is helping us fix some of the bugs found by our bug finding tool.
Normally, our patch series includes a reproducer in the cover letter, so
we usually send these patches with a cover letter. I believe Luxiao
simply forgot to include the reproducer in this version. The reproducer
for this bug can be found here:
https://lore.kernel.org/all/cover.1784428532.git.rakukuip@gmail.com/
For future patches, if we are sending a single patch without a cover
letter, what would be the preferred way to include the reproducer?
Should we put it directly in the commit message, or place it below the
--- separator so that the reproducer itself does not become part of the
permanent git history?
Also, I have been collecting the bug report by llm into a syzbot-like
tracking system[1].
The tracker aggregates bug reports from multiple sources, including
Sashiko, and then attempts to validate the reports and generate
reproducers to make sure it is not a false positive . It also
automatically tracks whether a bug has been fixed.
For the networking side, I have already imported the Sashiko reports
into the bug tracker and shared it with the net maintainers. I am
planning to do the same for mm, but haven't gotten to it yet. There are
already a few mm bugs in the tracker that were found by our own tool,
and none of them seem to have security implications.
If you have a chance, I'd be interested to hear what you think. Any
suggestions on how to make it more useful for mm maintainers would be
very welcome.
Thanks,
Yuan
[1]
https://lore.kernel.org/all/20260830115546.3942129-1-yuantan098@gmail.com/
hoo boy we've made a mess of the types in there, but I don't see a
feasible improvement in the context of this patch.
Also, a [0/N] isn't needed or desirable when N==1! I'll consolidate
both into a singleton patch.
Hi,
Luxiao is helping us fix some of the bugs found by our bug finding tool.
Normally, our patch series includes a reproducer in the cover letter, so
we usually send these patches with a cover letter. I believe Luxiao
simply forgot to include the reproducer in this version. The reproducer
for this bug can be found here:
https://lore.kernel.org/all/cover.1784428532.git.rakukuip@gmail.com/
Great, thanks.
See, what I'm always looking for in bug fixes is a solid description
of the significance/impact/risk which that bug has upon our users.
This impact ranges between "Impossibly improbable thing which some AI
scan found but which nobody is ever going to hit" through to "this
crashes our entire fleet 12 times per day".
The range really is that broad and this information matters. And
people care about it. I look to authors to help us understand and
assess this impact. So please do whatever you can (organization-wide)
to ensure that this info is made available to its potential audience.
quoted hunk
For future patches, if we are sending a single patch without a cover
letter, what would be the preferred way to include the reproducer?
Should we put it directly in the commit message, or place it below the
--- separator so that the reproducer itself does not become part of the
permanent git history?
It depends.
Formally, the reproducer should be made part of selftests/, so the bug
can never reappear. I think that's excessive for this project and the
risk of reintroduction is so low that this isn't worthwhile.
Pasting it in to the formal changelog is reasonable. Putting it below
the --- is probably better, as long as the permanent changelog mentions
its existence. Then highly motivated people can follow the Link: and
find the reproducer.
The main value in the reproducer is knowing that it exists! That this
bug can really be hit from userspace.
Also, I have been collecting the bug report by llm into a syzbot-like
tracking system[1].
The tracker aggregates bug reports from multiple sources, including
Sashiko, and then attempts to validate the reports and generate
reproducers to make sure it is not a false positive . It also
automatically tracks whether a bug has been fixed.
That's a great initiative, thanks.
For the networking side, I have already imported the Sashiko reports
into the bug tracker and shared it with the net maintainers. I am
planning to do the same for mm, but haven't gotten to it yet. There are
already a few mm bugs in the tracker that were found by our own tool,
and none of them seem to have security implications.
If you have a chance, I'd be interested to hear what you think. Any
suggestions on how to make it more useful for mm maintainers would be
very welcome.
I saw. I'd love to spend time with this but you know how it is. We're
all overwhelmed by the bot invasion at present, and none more than me.
I view your contributions as "parallelization of effort". Keep the
fixes coming and I'll do my best to get them through our pipeline and
out to our users.
This is the friendly patch-bot of Lorenzo Stoakes.
You have sent him a patch that has triggered this response.
He used to manually respond to these common problems, but in order to save
his sanity (he kept writing the same thing over and over, yet to different
people), I was created.
Hopefully you will not take offence and will fix the problem in your patch
and resubmit it so that it can be accepted into the Linux kernel tree.
When sending emails to mm:
1. Patch series with 1 patch sent with cover letter
Please send single patches without a cover letter.
The easiest way of accomplishing this (+ our preference) is to use
b4 [0], otherwise format patches like this:
$ git format-patch HEAD~1
[0]: https://b4.docs.kernel.org/en/latest/contributor/send.html
2. Missing cc's
You are missing cc's. Fixing this is easy with b4 [0] (the recommended way
of sending patches to mm):
$ b4 prep --auto-to-cc
Alternatively, you can use scripts/get_maintainer.pl:
$ scripts/get_maintainer.pl --nogit-fallback <files-or-patches>
[0]: https://b4.docs.kernel.org/en/latest/contributor/send.html
Specifically, the following appear to be missing:
linux-kernel@vger.kernel.org
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Lorenzo will reply once he has dug out from the pending patches received
from other developers.
thanks,
Lorenzo's patch email bot
[ Idea shamelessly stolen from greg-kh ]
--
Cheers, Lorenzo