[PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[wangzijie]

Clearing FMODE_LSEEK flag should not rely on whether proc_open ops exists, fix it.

Fixed: ad7f4ea6e36e ("proc: avoid use-after-free in proc_reg_open()")
Signed-off-by: wangzijie <wangzijie1@honor.com>
---
Based on mm-nonmm-unstable
---
 fs/proc/inode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 8de0af8c3..10a8481cc 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -474,12 +474,11 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 	struct pde_opener *pdeo;
 
 	if (pde_is_permanent(pde)) {
+		if (!pde->proc_ops->proc_lseek)
+			file->f_mode &= ~FMODE_LSEEK;
 		open = pde->proc_ops->proc_open;
-		if (open) {
-			if (!pde->proc_ops->proc_lseek)
-				file->f_mode &= ~FMODE_LSEEK;
+		if (open)
 			rv = open(inode, file);
-		}
 		return rv;
 	}
 
-- 
2.25.1

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[Andrew Morton]

On Thu, 5 Jun 2025 14:52:52 +0800 wangzijie <wangzijie1@honor.com> wrote:

> Clearing FMODE_LSEEK flag should not rely on whether proc_open ops exists,

Why is this?

> fix it.

What are the consequences of the fix?  Is there presently some kernel
misbehavior?

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[Al Viro]

On Thu, Jun 05, 2025 at 02:44:15PM -0700, Andrew Morton wrote:
> On Thu, 5 Jun 2025 14:52:52 +0800 wangzijie <wangzijie1@honor.com> wrote:
> 
> > Clearing FMODE_LSEEK flag should not rely on whether proc_open ops exists,
> 
> Why is this?
> 
> > fix it.
> 
> What are the consequences of the fix?  Is there presently some kernel
> misbehavior?

At a guess, that would be an oops due to this:
        if (pde_is_permanent(pde)) {
		return pde->proc_ops->proc_lseek(file, offset, whence);
	} else if (use_pde(pde)) {
		rv = pde->proc_ops->proc_lseek(file, offset, whence);
		unuse_pde(pde);
	}
in proc_reg_llseek().  No FMODE_LSEEK == "no seeks for that file, just
return -ESPIPE".  It is set by do_dentry_open() if you have NULL
->llseek() in ->f_op; the reason why procfs needs to adjust that is
the it has uniform ->llseek, calling the underlying method for that 
proc_dir_entry.  So if it's NULL, we need ->open() (also uniform,
proc_reg_open() for all of those) to clear FMODE_LSEEK from ->f_mode.

The thing I don't understand is where the hell had proc_reg_open()
changed in that way - commit refered in the patch doesn't exist in
mainline, doesn't seem to be in -next or -stable either.

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[wangzijie]

>On Thu, Jun 05, 2025 at 02:44:15PM -0700, Andrew Morton wrote:
>> On Thu, 5 Jun 2025 14:52:52 +0800 wangzijie <wangzijie1@honor.com> wrote:
>> 
>> > Clearing FMODE_LSEEK flag should not rely on whether proc_open ops exists,
>> 
>> Why is this?
>> 
>> > fix it.
>> 
>> What are the consequences of the fix?  Is there presently some kernel
>> misbehavior?
>
>At a guess, that would be an oops due to this:
>        if (pde_is_permanent(pde)) {
>		return pde->proc_ops->proc_lseek(file, offset, whence);
>	} else if (use_pde(pde)) {
>		rv = pde->proc_ops->proc_lseek(file, offset, whence);
>		unuse_pde(pde);
>	}
>in proc_reg_llseek().  No FMODE_LSEEK == "no seeks for that file, just
>return -ESPIPE".  It is set by do_dentry_open() if you have NULL
>->llseek() in ->f_op; the reason why procfs needs to adjust that is
>the it has uniform ->llseek, calling the underlying method for that 
>proc_dir_entry.  So if it's NULL, we need ->open() (also uniform,
>proc_reg_open() for all of those) to clear FMODE_LSEEK from ->f_mode.
>
>The thing I don't understand is where the hell had proc_reg_open()
>changed in that way - commit refered in the patch doesn't exist in
>mainline, doesn't seem to be in -next or -stable either.

My bad for making this misbehavior, thank you for helping explain it.
commit 654b33ada4ab("proc: fix UAF in proc_get_inode()") is in -stable, 
I refered v1 just for showing race in rmmod scenario, it's my bad.

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[wangzijie]

>On Thu, Jun 05, 2025 at 02:44:15PM -0700, Andrew Morton wrote:
>> On Thu, 5 Jun 2025 14:52:52 +0800 wangzijie <wangzijie1@honor.com> wrote:
>> 
>> > Clearing FMODE_LSEEK flag should not rely on whether proc_open ops exists,
>> 
>> Why is this?
>> 
>> > fix it.
>> 
>> What are the consequences of the fix?  Is there presently some kernel
>> misbehavior?
>
>At a guess, that would be an oops due to this:
>        if (pde_is_permanent(pde)) {
>		return pde->proc_ops->proc_lseek(file, offset, whence);
>	} else if (use_pde(pde)) {
>		rv = pde->proc_ops->proc_lseek(file, offset, whence);
>		unuse_pde(pde);
>	}
>in proc_reg_llseek().  No FMODE_LSEEK == "no seeks for that file, just
>return -ESPIPE".  It is set by do_dentry_open() if you have NULL
>->llseek() in ->f_op; the reason why procfs needs to adjust that is
>the it has uniform ->llseek, calling the underlying method for that 
>proc_dir_entry.  So if it's NULL, we need ->open() (also uniform,
>proc_reg_open() for all of those) to clear FMODE_LSEEK from ->f_mode.
>
>The thing I don't understand is where the hell had proc_reg_open()
>changed in that way - commit refered in the patch doesn't exist in
>mainline, doesn't seem to be in -next or -stable either.

I think maybe I misunderstand what you mean, this hell misbehavior(commit I refered)
is in mm-nonmm-unstable, so this fix is based on mm-nonmm-unstable.

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[Andrew Morton]

On Fri, 6 Jun 2025 02:56:21 +0100 Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Thu, Jun 05, 2025 at 02:44:15PM -0700, Andrew Morton wrote:
> > On Thu, 5 Jun 2025 14:52:52 +0800 wangzijie <wangzijie1@honor.com> wrote:
> > 
> > > Clearing FMODE_LSEEK flag should not rely on whether proc_open ops exists,
> > 
> > Why is this?
> > 
> > > fix it.
> > 
> > What are the consequences of the fix?  Is there presently some kernel
> > misbehavior?
> 
> At a guess, that would be an oops due to this:
>         if (pde_is_permanent(pde)) {
> 		return pde->proc_ops->proc_lseek(file, offset, whence);
> 	} else if (use_pde(pde)) {
> 		rv = pde->proc_ops->proc_lseek(file, offset, whence);
> 		unuse_pde(pde);
> 	}
> in proc_reg_llseek().  No FMODE_LSEEK == "no seeks for that file, just
> return -ESPIPE".  It is set by do_dentry_open() if you have NULL
> ->llseek() in ->f_op; the reason why procfs needs to adjust that is
> the it has uniform ->llseek, calling the underlying method for that 
> proc_dir_entry.  So if it's NULL, we need ->open() (also uniform,
> proc_reg_open() for all of those) to clear FMODE_LSEEK from ->f_mode.
> 
> The thing I don't understand is where the hell had proc_reg_open()
> changed in that way - commit refered in the patch doesn't exist in
> mainline, doesn't seem to be in -next or -stable either.

It's a fix against the very recently merged
https://lkml.kernel.org/r/20250528034756.4069180-1-wangzijie1@honor.com.

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[Al Viro]

On Fri, Jun 06, 2025 at 10:37:35AM +0800, wangzijie wrote:

> My bad for making this misbehavior, thank you for helping explain it.
> commit 654b33ada4ab("proc: fix UAF in proc_get_inode()") is in -stable, 
> I refered v1 just for showing race in rmmod scenario, it's my bad.

I still don't understand what's going on.  654b33ada4ab is both in
mainline and in stable, but proc_reg_open() is nowhere near the
shape your patch would imply.

The best reconstruction I can come up with is that an earlier patch
in whatever tree you are talking about has moved

        if (!pde->proc_ops->proc_lseek)
		file->f_mode &= ~FMODE_LSEEK;

down, separately into permanent and non-permanent cases, after use_pde()
in the latter case.  And the author of that earlier patch has moved
the check under if (open) in permanent case, which would warrant that
kind of fixup.

However,
	* why is that earlier patch sitting someplace that is *NOT*
in -next?
	* why bother with those games at all?  Just nick another bit
from pde->flags (let's call it PROC_ENTRY_proc_lseek for consistency
sake), set it in same pde_set_flags() where other flags are dealt with
and just turn that thing into
        if (!pde_has_proc_lseek(pde))
		file->f_mode &= ~FMODE_LSEEK;
leaving it where it is.  Less intrusive and easier to follow...

	Call it something like

check proc_lseek needs the same treatment as ones for proc_read_iter et.al.

and describe it as a gap in "proc: fix UAF in proc_get_inode()",
fixed in exact same manner...

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[Al Viro]

On Thu, Jun 05, 2025 at 08:44:42PM -0700, Andrew Morton wrote:

> It's a fix against the very recently merged
> https://lkml.kernel.org/r/20250528034756.4069180-1-wangzijie1@honor.com.

IMO it's a wrong way to handle that - see upthread for a simpler
approach...

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[Andrew Morton]

On Fri, 6 Jun 2025 04:57:14 +0100 Al Viro <viro@zeniv.linux.org.uk> wrote:

> 	* why is that earlier patch sitting someplace that is *NOT*
> in -next?

I recently tenitatively added it to mm.git and haven't pushed that out
yet.

> 	* why bother with those games at all?  Just nick another bit
> from pde->flags (let's call it PROC_ENTRY_proc_lseek for consistency
> sake), set it in same pde_set_flags() where other flags are dealt with
> and just turn that thing into
>         if (!pde_has_proc_lseek(pde))
> 		file->f_mode &= ~FMODE_LSEEK;
> leaving it where it is.  Less intrusive and easier to follow...

Thanks, I'll drop both patches.

Re: [PATCH] proc: clear FMODE_LSEEK flag correctly for permanent pde

[wangzijie]

>On Fri, Jun 06, 2025 at 10:37:35AM +0800, wangzijie wrote:
>
>> My bad for making this misbehavior, thank you for helping explain it.
>> commit 654b33ada4ab("proc: fix UAF in proc_get_inode()") is in -stable, 
>> I refered v1 just for showing race in rmmod scenario, it's my bad.
>
>I still don't understand what's going on.  654b33ada4ab is both in
>mainline and in stable, but proc_reg_open() is nowhere near the
>shape your patch would imply.
>
>The best reconstruction I can come up with is that an earlier patch
>in whatever tree you are talking about has moved
>
>        if (!pde->proc_ops->proc_lseek)
>		file->f_mode &= ~FMODE_LSEEK;
>
>down, separately into permanent and non-permanent cases, after use_pde()
>in the latter case.  And the author of that earlier patch has moved
>the check under if (open) in permanent case, which would warrant that
>kind of fixup.
>
>However,
>	* why is that earlier patch sitting someplace that is *NOT*
>in -next?
>	* why bother with those games at all?  Just nick another bit
>from pde->flags (let's call it PROC_ENTRY_proc_lseek for consistency
>sake), set it in same pde_set_flags() where other flags are dealt with
>and just turn that thing into
>        if (!pde_has_proc_lseek(pde))
>		file->f_mode &= ~FMODE_LSEEK;
>leaving it where it is.  Less intrusive and easier to follow...
>
>	Call it something like
>
>check proc_lseek needs the same treatment as ones for proc_read_iter et.al.
>
>and describe it as a gap in "proc: fix UAF in proc_get_inode()",
>fixed in exact same manner...

Thank you very much for your suggestion! I will follow that and submit patch later.