fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com --- Applies against today's mm-unstable (aa05a436eca8).
Thanks, Ryan
mm/util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/util.c b/mm/util.c index 6c1d64ed0221..8989d5767528 100644 --- a/mm/util.c +++ b/mm/util.c @@ -566,6 +566,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr, unsigned long len, unsigned long prot, unsigned long flag, unsigned long pgoff) { + loff_t off = (loff_t)pgoff << PAGE_SHIFT; unsigned long ret; struct mm_struct *mm = current->mm; unsigned long populate; @@ -573,7 +574,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
ret = security_mmap_file(file, prot, flag); if (!ret) - ret = fsnotify_mmap_perm(file, prot, pgoff >> PAGE_SHIFT, len); + ret = fsnotify_mmap_perm(file, prot, off, len); if (!ret) { if (mmap_write_lock_killable(mm)) return -EINTR; -- 2.43.0
On Fri, Oct 03, 2025 at 04:52:36PM +0100, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Heh. Just submitted fix for the same issue:
https://lore.kernel.org/all/20251003155804.1571242-1-kirill@shutemov.name/T/...
On 03/10/2025 17:00, Kiryl Shutsemau wrote:
On Fri, Oct 03, 2025 at 04:52:36PM +0100, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Heh. Just submitted fix for the same issue:
https://lore.kernel.org/all/20251003155804.1571242-1-kirill@shutemov.name/T/...
Ha... great minds...
I notice that for your version you're just doing "pgoff << PAGE_SHIFT" without casting pgoff.
I'm not sure if that is safe?
pgoff is unsigned long (so 32 bits on 32 bit systems). loff_t is unsigned long long (so always 64 bits). So is it possible that you shift off the end of 32 bits and lose those bits without a cast to loff_t first?
TBH my knowledge of the exact rules is shaky...
On Fri, Oct 03, 2025 at 05:36:23PM +0100, Ryan Roberts wrote:
On 03/10/2025 17:00, Kiryl Shutsemau wrote:
On Fri, Oct 03, 2025 at 04:52:36PM +0100, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Heh. Just submitted fix for the same issue:
https://lore.kernel.org/all/20251003155804.1571242-1-kirill@shutemov.name/T/...
Ha... great minds...
I notice that for your version you're just doing "pgoff << PAGE_SHIFT" without casting pgoff.
I'm not sure if that is safe?
pgoff is unsigned long (so 32 bits on 32 bit systems). loff_t is unsigned long long (so always 64 bits). So is it possible that you shift off the end of 32 bits and lose those bits without a cast to loff_t first?
TBH my knowledge of the exact rules is shaky...
I think you are right. Missing cast in my patch might be problematic on 32-bit machines.
Reviewed-by: Kiryl Shutsemau kas@kernel.org
On 03.10.25 17:52, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Curious: is there some easy way to write a reproducer? Did you look into that?
LGTM, thanks
Acked-by: David Hildenbrand david@redhat.com
On 06/10/2025 12:36, David Hildenbrand wrote:
On 03.10.25 17:52, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Curious: is there some easy way to write a reproducer? Did you look into that?
I didn't; this was just a drive-by discovery.
It looks like there are some fanotify tests in the filesystems selftests; I guess they could be extended to add a regression test?
But FWIW, I think the kernel is just passing the ofset/length info off to user space and isn't acting on it itself. So there is no kernel vulnerability here.
LGTM, thanks
Acked-by: David Hildenbrand david@redhat.com
On 06.10.25 14:14, Ryan Roberts wrote:
On 06/10/2025 12:36, David Hildenbrand wrote:
On 03.10.25 17:52, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Curious: is there some easy way to write a reproducer? Did you look into that?
I didn't; this was just a drive-by discovery.
It looks like there are some fanotify tests in the filesystems selftests; I guess they could be extended to add a regression test?
But FWIW, I think the kernel is just passing the ofset/length info off to user space and isn't acting on it itself. So there is no kernel vulnerability here.
Right, I'm rather wondering if this could have been caught earlier and how we could have caught it earlier :)
On Mon, Oct 6, 2025 at 3:53 PM David Hildenbrand david@redhat.com wrote:
On 06.10.25 14:14, Ryan Roberts wrote:
On 06/10/2025 12:36, David Hildenbrand wrote:
On 03.10.25 17:52, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Curious: is there some easy way to write a reproducer? Did you look into that?
I didn't; this was just a drive-by discovery.
It looks like there are some fanotify tests in the filesystems selftests; I guess they could be extended to add a regression test?
But FWIW, I think the kernel is just passing the ofset/length info off to user space and isn't acting on it itself. So there is no kernel vulnerability here.
Right, I'm rather wondering if this could have been caught earlier and how we could have caught it earlier :)
Ha! you would have thought we either have no test for it or we test only mmap with offset 0.
But we have LTP test fanotify24 which does mmap with offset page_sz*100 and indeed it prints the info and info says offset 0, only we do not verify the offset info in this test...
Will be fixed.
Thanks Ryan for being alert! Amir.
On Mon, Oct 6, 2025 at 4:40 PM Amir Goldstein amir73il@gmail.com wrote:
On Mon, Oct 6, 2025 at 3:53 PM David Hildenbrand david@redhat.com wrote:
On 06.10.25 14:14, Ryan Roberts wrote:
On 06/10/2025 12:36, David Hildenbrand wrote:
On 03.10.25 17:52, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Curious: is there some easy way to write a reproducer? Did you look into that?
I didn't; this was just a drive-by discovery.
It looks like there are some fanotify tests in the filesystems selftests; I guess they could be extended to add a regression test?
But FWIW, I think the kernel is just passing the ofset/length info off to user space and isn't acting on it itself. So there is no kernel vulnerability here.
Right, I'm rather wondering if this could have been caught earlier and how we could have caught it earlier :)
Ha! you would have thought we either have no test for it or we test only mmap with offset 0.
But we have LTP test fanotify24 which does mmap with offset page_sz*100 and indeed it prints the info and info says offset 0, only we do not verify the offset info in this test...
Will be fixed.
Jan,
FYI test enhanced and verified the bug and the fix: https://github.com/amir73il/ltp/commits/fsnotify-fixes/
Will wait with posting the test until you merge the fix to make sure that the commit id is not changed.
Thanks, Amir.
On Fri 03-10-25 16:52:36, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Thanks Ryan! I've added the patch to my tree. As a side note, I know the callsite is in mm/ but since this is clearly impacting fsnotify, it would be good to add to CC relevant people (I'm not following linux-mm nor linux-kernel) and discovered this only because of Kiryl's link...
Honza
mm/util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/util.c b/mm/util.c index 6c1d64ed0221..8989d5767528 100644 --- a/mm/util.c +++ b/mm/util.c @@ -566,6 +566,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr, unsigned long len, unsigned long prot, unsigned long flag, unsigned long pgoff) {
- loff_t off = (loff_t)pgoff << PAGE_SHIFT; unsigned long ret; struct mm_struct *mm = current->mm; unsigned long populate;
@@ -573,7 +574,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
ret = security_mmap_file(file, prot, flag); if (!ret)
ret = fsnotify_mmap_perm(file, prot, pgoff >> PAGE_SHIFT, len);
ret = fsnotify_mmap_perm(file, prot, off, len);-- 2.43.0
On 06/10/2025 15:55, Jan Kara wrote:
On Fri 03-10-25 16:52:36, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Thanks Ryan! I've added the patch to my tree. As a side note, I know the callsite is in mm/ but since this is clearly impacting fsnotify, it would be good to add to CC relevant people (I'm not following linux-mm nor linux-kernel) and discovered this only because of Kiryl's link...
Ahh good point... Sorry I was sleepwalking through the process on Friday afternoon and blindly sent it to the maintainers and reviewers that get_maintainer.pl spat out. It didn't even occur to me that this wasn't an mm thing. :-|
Honzamm/util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/util.c b/mm/util.c index 6c1d64ed0221..8989d5767528 100644 --- a/mm/util.c +++ b/mm/util.c @@ -566,6 +566,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr, unsigned long len, unsigned long prot, unsigned long flag, unsigned long pgoff) {
- loff_t off = (loff_t)pgoff << PAGE_SHIFT; unsigned long ret; struct mm_struct *mm = current->mm; unsigned long populate;
@@ -573,7 +574,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
ret = security_mmap_file(file, prot, flag); if (!ret)
ret = fsnotify_mmap_perm(file, prot, pgoff >> PAGE_SHIFT, len);
ret = fsnotify_mmap_perm(file, prot, off, len);-- 2.43.0
On Mon 06-10-25 16:04:23, Ryan Roberts wrote:
On 06/10/2025 15:55, Jan Kara wrote:
On Fri 03-10-25 16:52:36, Ryan Roberts wrote:
fsnotify_mmap_perm() requires a byte offset for the file about to be mmap'ed. But it is called from vm_mmap_pgoff(), which has a page offset. Previously the conversion was done incorrectly so let's fix it, being careful not to overflow on 32-bit platforms.
Discovered during code review.
Cc: stable@vger.kernel.org Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()") Signed-off-by: Ryan Roberts ryan.roberts@arm.com
Applies against today's mm-unstable (aa05a436eca8).
Thanks Ryan! I've added the patch to my tree. As a side note, I know the callsite is in mm/ but since this is clearly impacting fsnotify, it would be good to add to CC relevant people (I'm not following linux-mm nor linux-kernel) and discovered this only because of Kiryl's link...
Ahh good point... Sorry I was sleepwalking through the process on Friday afternoon and blindly sent it to the maintainers and reviewers that get_maintainer.pl spat out. It didn't even occur to me that this wasn't an mm thing. :-|
No harm done really. The change is an obvious fix and it would find its way to the kernel sooner or later. As I wrote above, this is just a note for the future to think a bit about patch recipients before hitting send :) It may help to get the patch merged faster.
Honza
mm/util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/util.c b/mm/util.c index 6c1d64ed0221..8989d5767528 100644 --- a/mm/util.c +++ b/mm/util.c @@ -566,6 +566,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr, unsigned long len, unsigned long prot, unsigned long flag, unsigned long pgoff) {
- loff_t off = (loff_t)pgoff << PAGE_SHIFT; unsigned long ret; struct mm_struct *mm = current->mm; unsigned long populate;
@@ -573,7 +574,7 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
ret = security_mmap_file(file, prot, flag); if (!ret)
ret = fsnotify_mmap_perm(file, prot, pgoff >> PAGE_SHIFT, len);
ret = fsnotify_mmap_perm(file, prot, off, len);-- 2.43.0
linux-stable-mirror@lists.linaro.org
-
Amir Goldstein -
David Hildenbrand -
Jan Kara -
Kiryl Shutsemau -
Ryan Roberts