[PATCH] file: Call security_file_alloc() after initializing the filp

[PATCH] file: Call security_file_alloc() after initializing the filp

[Tianjia Zhang]

When developing a dedicated LSM module, we need to operate on the
file object within the LSM function, such as retrieving the path.
However, in `security_file_alloc()`, the passed-in `filp` is
only a valid pointer; the content of `filp` is completely
uninitialized and entirely random, which confuses the LSM function.

Therefore, it is necessary to call `security_file_alloc()` only
after the main fields of the `filp` object have been initialized.
This patch only moves the call to `security_file_alloc()` to the
end of the `init_file()` function.

Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
 fs/file_table.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 81c72576e548..e66531a629aa 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -156,11 +156,6 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
 	int error;
 
 	f->f_cred = get_cred(cred);
-	error = security_file_alloc(f);
-	if (unlikely(error)) {
-		put_cred(f->f_cred);
-		return error;
-	}
 
 	spin_lock_init(&f->f_lock);
 	/*
@@ -202,6 +197,14 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
 	 * They may be enabled later by fsnotify_open_perm_and_set_mode().
 	 */
 	file_set_fsnotify_mode(f, FMODE_NONOTIFY_PERM);
+
+	error = security_file_alloc(f);
+	if (unlikely(error)) {
+		mutex_destroy(&f->f_pos_lock);
+		put_cred(f->f_cred);
+		return error;
+	}
+
 	return 0;
 }
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH] file: Call security_file_alloc() after initializing the filp

[Mateusz Guzik]

On Tue, Dec 09, 2025 at 03:53:47PM +0800, Tianjia Zhang wrote:
> When developing a dedicated LSM module, we need to operate on the
> file object within the LSM function, such as retrieving the path.
> However, in `security_file_alloc()`, the passed-in `filp` is
> only a valid pointer; the content of `filp` is completely
> uninitialized and entirely random, which confuses the LSM function.
> 

I take it you have some underlying routine called by other hooks as well
which ends up looking at ->f_path.

Given that f_path *is not valid* to begin with, memsetted or not, your
file_alloc_security hoook should not be looking at it to begin with.

So I don't think this patch has merit.

> Therefore, it is necessary to call `security_file_alloc()` only
> after the main fields of the `filp` object have been initialized.
> This patch only moves the call to `security_file_alloc()` to the
> end of the `init_file()` function.
> 
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> ---
>  fs/file_table.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 81c72576e548..e66531a629aa 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -156,11 +156,6 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
>  	int error;
>  
>  	f->f_cred = get_cred(cred);
> -	error = security_file_alloc(f);
> -	if (unlikely(error)) {
> -		put_cred(f->f_cred);
> -		return error;
> -	}
>  
>  	spin_lock_init(&f->f_lock);
>  	/*
> @@ -202,6 +197,14 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
>  	 * They may be enabled later by fsnotify_open_perm_and_set_mode().
>  	 */
>  	file_set_fsnotify_mode(f, FMODE_NONOTIFY_PERM);
> +
> +	error = security_file_alloc(f);
> +	if (unlikely(error)) {
> +		mutex_destroy(&f->f_pos_lock);
> +		put_cred(f->f_cred);
> +		return error;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.39.5 (Apple Git-154)
> 

Re: [PATCH] file: Call security_file_alloc() after initializing the filp

[tianjia.zhang]

On 12/9/25 4:22 PM, Mateusz Guzik wrote:
> On Tue, Dec 09, 2025 at 03:53:47PM +0800, Tianjia Zhang wrote:
>> When developing a dedicated LSM module, we need to operate on the
>> file object within the LSM function, such as retrieving the path.
>> However, in `security_file_alloc()`, the passed-in `filp` is
>> only a valid pointer; the content of `filp` is completely
>> uninitialized and entirely random, which confuses the LSM function.
>>
> 
> I take it you have some underlying routine called by other hooks as well
> which ends up looking at ->f_path.
> 
> Given that f_path *is not valid* to begin with, memsetted or not, your
> file_alloc_security hoook should not be looking at it to begin with.
> 
> So I don't think this patch has merit.
> 

The scenario is as follows: I have hooked all LSM functions and
abstracted struct file into an object using higher-level logic. In my
handler functions, I need to print the file path of this object for
debugging purposes. However, doing so will cause a crash unless I
explicitly know that handler in the file_alloc_security context—which,
in my case, I don't.

Of course, obtaining the path isn't strictly required; I understand that
in certain situations—such as during initialization—there may be no
valid path at all. Even so, it would be acceptable if I could reliably
determine from filp->f_path that fetching the path is inappropriate. The
problem is that, without knowing whether I'm in the file_alloc_security
context, I have no reliable way to decide whether it's safe to attempt
retrieving the path.

Re: [PATCH] file: Call security_file_alloc() after initializing the filp

[Mateusz Guzik]

On Fri, Dec 12, 2025 at 11:01 AM tianjia.zhang
<tianjia.zhang@linux.alibaba.com> wrote:
>
>
>
> On 12/9/25 4:22 PM, Mateusz Guzik wrote:
> > On Tue, Dec 09, 2025 at 03:53:47PM +0800, Tianjia Zhang wrote:
> >> When developing a dedicated LSM module, we need to operate on the
> >> file object within the LSM function, such as retrieving the path.
> >> However, in `security_file_alloc()`, the passed-in `filp` is
> >> only a valid pointer; the content of `filp` is completely
> >> uninitialized and entirely random, which confuses the LSM function.
> >>
> >
> > I take it you have some underlying routine called by other hooks as well
> > which ends up looking at ->f_path.
> >
> > Given that f_path *is not valid* to begin with, memsetted or not, your
> > file_alloc_security hoook should not be looking at it to begin with.
> >
> > So I don't think this patch has merit.
> >
>
> The scenario is as follows: I have hooked all LSM functions and
> abstracted struct file into an object using higher-level logic. In my
> handler functions, I need to print the file path of this object for
> debugging purposes. However, doing so will cause a crash unless I
> explicitly know that handler in the file_alloc_security context—which,
> in my case, I don't.
>

Per my previous e-mail the real bug is that you are accessing a field
which at the time does not have a legitimate value.

> Of course, obtaining the path isn't strictly required; I understand that
> in certain situations—such as during initialization—there may be no
> valid path at all. Even so, it would be acceptable if I could reliably
> determine from filp->f_path that fetching the path is inappropriate. The
> problem is that, without knowing whether I'm in the file_alloc_security
> context, I have no reliable way to decide whether it's safe to attempt
> retrieving the path.

For the sake of argument let's say the patch or an equivalent went in
and you no longer crash on f_path.

The only legally populated field at the time is f_cred.

Later someone might get an idea to look at other fields and instead of
crashing get bogus results.

Or to put it differently, it's a design issue in your code. When
called from a hook where mucking with 'file' is illegal, it needs to
know to refrain from doing it.

Re: [PATCH] file: Call security_file_alloc() after initializing the filp

[Al Viro]

On Fri, Dec 12, 2025 at 06:01:53PM +0800, tianjia.zhang wrote:

> The scenario is as follows: I have hooked all LSM functions and
> abstracted struct file into an object using higher-level logic. In my
> handler functions, I need to print the file path of this object for
> debugging purposes. However, doing so will cause a crash unless I
> explicitly know that handler in the file_alloc_security context—which,
> in my case, I don't.
> 
> Of course, obtaining the path isn't strictly required; I understand that
> in certain situations—such as during initialization—there may be no
> valid path at all. Even so, it would be acceptable if I could reliably
> determine from filp->f_path that fetching the path is inappropriate. The
> problem is that, without knowing whether I'm in the file_alloc_security
> context, I have no reliable way to decide whether it's safe to attempt
> retrieving the path.

<sarcasm>

"I can't figure out which of the functions in my code is calling (directly)
this function in my code; there's a predicate that might allow me to do
that, but it doesn't really work without this change to function outside
of my code.  With this change I can make the things work; no, I won't
tell you which predicate it is, you'll just have to avoid any changes
in the area in the future, lest my code breaks".

</sarcasm>

In case it's not obvious from the above, your reasoning is unconvincing.

Re: [PATCH] file: Call security_file_alloc() after initializing the filp

[Al Viro]

On Tue, Dec 09, 2025 at 03:53:47PM +0800, Tianjia Zhang wrote:
> When developing a dedicated LSM module, we need to operate on the
> file object within the LSM function, such as retrieving the path.
> However, in `security_file_alloc()`, the passed-in `filp` is
> only a valid pointer; the content of `filp` is completely
> uninitialized and entirely random, which confuses the LSM function.
> 
> Therefore, it is necessary to call `security_file_alloc()` only
> after the main fields of the `filp` object have been initialized.
> This patch only moves the call to `security_file_alloc()` to the
> end of the `init_file()` function.

Which fields would those be and why would ->file_alloc(), which is
not called anywhere else, depend on any values being stored there?
And how would init_file() know which path we are going to use
that struct file for, anyway, considering that file is allocated
and init_file() called *before* we get around to resolving the pathname?