[PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Ryan Roberts]

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

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Kiryl Shutsemau]

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/#u

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Ryan Roberts]

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/#u
> 

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...

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Kiryl Shutsemau]

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/#u
> > 
> 
> 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>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[David Hildenbrand]

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>

-- 
Cheers

David / dhildenb

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Ryan Roberts]

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>
> 

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[David Hildenbrand]

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 :)

-- 
Cheers

David / dhildenb

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Amir Goldstein]

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.

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Amir Goldstein]

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.

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Jan Kara]

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);
>  	if (!ret) {
>  		if (mmap_write_lock_killable(mm))
>  			return -EINTR;
> --
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Ryan Roberts]

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. :-|

> 
> 								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);
>>  	if (!ret) {
>>  		if (mmap_write_lock_killable(mm))
>>  			return -EINTR;
>> --
>> 2.43.0
>>

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Jan Kara]

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);
> >>  	if (!ret) {
> >>  		if (mmap_write_lock_killable(mm))
> >>  			return -EINTR;
> >> --
> >> 2.43.0
> >>
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v1] fsnotify: Pass correct offset to fsnotify_mmap_perm()

[Lorenzo Stoakes]

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.
>

Yikes, that's kind of crazy that has been in place for so long but not picked
up...

> Cc: <stable@vger.kernel.org>
> Fixes: 066e053fe208 ("fsnotify: add pre-content hooks on mmap()")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

Thanks for fixing this!

LGTM, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.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
>