[v6,1/5] security: create file_truncate hook from path_truncate hook

Commit Message

Günther Noack Sept. 8, 2022, 7:58 p.m. UTC

Like path_truncate, the file_truncate hook also restricts file
truncation, but is called in the cases where truncation is attempted
on an already-opened file.

This is required in a subsequent commit to handle ftruncate()
operations differently to truncate() operations.

Signed-off-by: Günther Noack <[e-mail]>
---
 fs/namei.c                    |  6 +++---
 fs/open.c                     |  4 ++--
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  6 ++++++
 security/apparmor/lsm.c       |  6 ++++++
 security/security.c           |  5 +++++
 security/tomoyo/tomoyo.c      | 13 +++++++++++++
 7 files changed, 36 insertions(+), 5 deletions(-)

Comments

Paul Moore Sept. 8, 2022, 8:09 p.m. UTC | #1

On Thu, Sep 8, 2022 at 3:58 PM Günther Noack <[e-mail]> wrote:
>
> Like path_truncate, the file_truncate hook also restricts file
> truncation, but is called in the cases where truncation is attempted
> on an already-opened file.
>
> This is required in a subsequent commit to handle ftruncate()
> operations differently to truncate() operations.
>
> Signed-off-by: Günther Noack <[e-mail]>
> ---
>  fs/namei.c                    |  6 +++---
>  fs/open.c                     |  4 ++--
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  security/apparmor/lsm.c       |  6 ++++++
>  security/security.c           |  5 +++++
>  security/tomoyo/tomoyo.c      | 13 +++++++++++++
>  7 files changed, 36 insertions(+), 5 deletions(-)

We need to get John and Tetsuo's ACKs on this patch, but in addition
to that I have two small comments (below).

> diff --git a/fs/namei.c b/fs/namei.c
> index 53b4bc094db2..52105873d1f8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
>   * The new code replaces the old recursive symlink resolution with
>   * an iterative one (in case of non-nested symlink chains).  It does
>   * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all

Since this patch(set) is likely to go in via the Landlock tree, it is
very important to keep changes outside of security/landlock to a bare
minimum of what is required both to reduce merge conflicts and
highlight the significant changes.  This change doesn't appear
necessary ... and I'm having a hard time spotting the difference in
the lines.

> diff --git a/fs/open.c b/fs/open.c
> index 8a813fa5ca56..0831433e493a 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
>  {
>         struct filename *name = getname_kernel(filename);
>         struct file *file = ERR_CAST(name);
> -
> +

See my comment above about unnecessary changes.

Günther Noack Sept. 8, 2022, 8:28 p.m. UTC | #2

Adding Namjae Jeon and David Howells as authors of the respective
files in fs/ksmbd and fs/cachefiles -- do you happen to know whether
these vfs_truncate() calls are using 'struct file's that are opened by
normal userspace processes, where LSM policies may apply?

P.S. In this patch I have looked for all places where the
security_path_truncate() hook was called, to see which of these should
rather use security_file_truncate() (and I made sure that it does the
same thing for all the LSMs that use it).

I'm confident that this does the right thing when truncate() or
ftruncate() are called from userspace, but one of the places that
still calls the path-based hook is vfs_truncate(), and this is called
from more places in the kernel than just from userspace:

init/initramfs.c
387:				vfs_truncate(&wfile->f_path, body_len);

security/keys/big_key.c
172:		vfs_truncate(&payload->path, 0);

fs/cachefiles/interface.c
242:		ret = vfs_truncate(&file->f_path, dio_size);

fs/cachefiles/namei.c
497:			ret = vfs_truncate(&path, ni_size);

fs/ksmbd/smb2pdu.c
2350:	int rc = vfs_truncate(path, 0);

fs/ksmbd/vfs.c
874:	err = vfs_truncate(&filp->f_path, size);

I suspect that these are benign but am not familiar with all of these
corners of the codebase. -- The question is: Some of these call
vfs_truncate() on the f_path of an existing struct file -- should
these rather be calling the security_file_truncate() than the
security_path_truncate() hook to authorize the truncation?

Specifically, I think:

* initramfs happens at system startup and LSMs should not interfere at
  this point yet
* security/keys does not use an opened struct file, so calling the
  path-based hook through vfs_truncate() is correct
* fs/cachefiles and fs/ksmbd use the file system from the kernel to
  expose it as another file system (in a cached form for cachefiles,
  and over the network for ksmbd). I suspect that these file systems
  are not handling 'struct file's which are opened in contexts where a
  LSM applies? It that a reasonable assumption?

Thanks,
Günther

On Thu, Sep 08, 2022 at 09:58:01PM +0200, Günther Noack wrote:
> Like path_truncate, the file_truncate hook also restricts file
> truncation, but is called in the cases where truncation is attempted
> on an already-opened file.
>
> This is required in a subsequent commit to handle ftruncate()
> operations differently to truncate() operations.
>
> Signed-off-by: Günther Noack <[e-mail]>
> ---
>  fs/namei.c                    |  6 +++---
>  fs/open.c                     |  4 ++--
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  security/apparmor/lsm.c       |  6 ++++++
>  security/security.c           |  5 +++++
>  security/tomoyo/tomoyo.c      | 13 +++++++++++++
>  7 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 53b4bc094db2..52105873d1f8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
>   * The new code replaces the old recursive symlink resolution with
>   * an iterative one (in case of non-nested symlink chains).  It does
>   * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all
>   * the special cases of the former code.
>   *
>   * With the new dcache, the pathname is stored at each inode, at least as
> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
>  	if (error)
>  		return error;
>
> -	error = security_path_truncate(path);
> +	error = security_file_truncate(filp);
>  	if (!error) {
>  		error = do_truncate(mnt_userns, path->dentry, 0,
>  				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> diff --git a/fs/open.c b/fs/open.c
> index 8a813fa5ca56..0831433e493a 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>  	if (IS_APPEND(file_inode(f.file)))
>  		goto out_putf;
>  	sb_start_write(inode->i_sb);
> -	error = security_path_truncate(&f.file->f_path);
> +	error = security_file_truncate(f.file);
>  	if (!error)
>  		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>  				    ATTR_MTIME | ATTR_CTIME, f.file);
> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
>  {
>  	struct filename *name = getname_kernel(filename);
>  	struct file *file = ERR_CAST(name);
> -
> +
>  	if (!IS_ERR(name)) {
>  		file = file_open_name(name, flags, mode);
>  		putname(name);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 60fff133c0b1..dee35ab253ba 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
>  	 struct fown_struct *fown, int sig)
>  LSM_HOOK(int, 0, file_receive, struct file *file)
>  LSM_HOOK(int, 0, file_open, struct file *file)
> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>  LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>  	 unsigned long clone_flags)
>  LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 7bd0c490703d..f80b23382dd9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>  				 struct fown_struct *fown, int sig);
>  int security_file_receive(struct file *file);
>  int security_file_open(struct file *file);
> +int security_file_truncate(struct file *file);
>  int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>  void security_task_free(struct task_struct *task);
>  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
>  	return 0;
>  }
>
> +static inline int security_file_truncate(struct file *file)
> +{
> +	return 0;
> +}
> +
>  static inline int security_task_alloc(struct task_struct *task,
>  				      unsigned long clone_flags)
>  {
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index e29cade7b662..98ecb7f221b8 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
>  	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
>  }
>
> +static int apparmor_file_truncate(struct file *file)
> +{
> +	return apparmor_path_truncate(&file->f_path);
> +}
> +
>  static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
>  				 const char *old_name)
>  {
> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>  	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>  	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>
>  	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>  	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
> diff --git a/security/security.c b/security/security.c
> index 4b95de24bc8d..e491120c48ba 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path *path)
>  	return call_int_hook(path_truncate, 0, path);
>  }
>
> +int security_file_truncate(struct file *file)
> +{
> +	return call_int_hook(file_truncate, 0, file);
> +}
> +
>  int security_path_chmod(const struct path *path, umode_t mode)
>  {
>  	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 71e82d855ebf..af04a7b7eb28 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
>  	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>  }
>
> +/**
> + * tomoyo_file_truncate - Target for security_file_truncate().
> + *
> + * @file: Pointer to "struct file".
> + *
> + * Returns 0 on success, negative value otherwise.
> + */
> +static int tomoyo_file_truncate(struct file *file)
> +{
> +	return tomoyo_path_truncate(&file->f_path);
> +}
> +
>  /**
>   * tomoyo_path_unlink - Target for security_path_unlink().
>   *
> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>  	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>  	LSM_HOOK_INIT(file_open, tomoyo_file_open),
> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>  	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>  	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>  	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),
> --
> 2.37.3
>

--

Günther Noack Sept. 8, 2022, 8:50 p.m. UTC | #3

On Thu, Sep 08, 2022 at 04:09:06PM -0400, Paul Moore wrote:
> On Thu, Sep 8, 2022 at 3:58 PM Günther Noack <[e-mail]> wrote:
> >
> > Like path_truncate, the file_truncate hook also restricts file
> > truncation, but is called in the cases where truncation is attempted
> > on an already-opened file.
> >
> > This is required in a subsequent commit to handle ftruncate()
> > operations differently to truncate() operations.
> >
> > Signed-off-by: Günther Noack <[e-mail]>
> > ---
> >  fs/namei.c                    |  6 +++---
> >  fs/open.c                     |  4 ++--
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  6 ++++++
> >  security/apparmor/lsm.c       |  6 ++++++
> >  security/security.c           |  5 +++++
> >  security/tomoyo/tomoyo.c      | 13 +++++++++++++
> >  7 files changed, 36 insertions(+), 5 deletions(-)
>
> We need to get John and Tetsuo's ACKs on this patch, but in addition
> to that I have two small comments (below).

+CC: John Johansen and Tetsuo Handa -- this change is splitting up the
path_truncate LSM hook into a path_truncate and file_truncate variant,
one operating on the path as before, and one operating on a struct
file*. As a result, AppArmor and TOMOYO need to implement the
file-based hook as well and treat it the same as before by looking at
the file's ->f_path. Does this change seem reasonable to you?


> > diff --git a/fs/namei.c b/fs/namei.c
> > index 53b4bc094db2..52105873d1f8 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -53,8 +53,8 @@
> >   * The new code replaces the old recursive symlink resolution with
> >   * an iterative one (in case of non-nested symlink chains).  It does
> >   * this with calls to <fs>_follow_link().
> > - * As a side effect, dir_namei(), _namei() and follow_link() are now
> > - * replaced with a single function lookup_dentry() that can handle all
> > + * As a side effect, dir_namei(), _namei() and follow_link() are now
> > + * replaced with a single function lookup_dentry() that can handle all
>
> Since this patch(set) is likely to go in via the Landlock tree, it is
> very important to keep changes outside of security/landlock to a bare
> minimum of what is required both to reduce merge conflicts and
> highlight the significant changes.  This change doesn't appear
> necessary ... and I'm having a hard time spotting the difference in
> the lines.

Thanks for pointing this out, I fully agree!

Both edits that you flagged were unintentional, I should have double
checked this patch better. I'll address them in the next version.

(You can't see the difference here because my editor trimmed trailing
whitespace on save (*facepalm*) - I turned this off now.)


> > diff --git a/fs/open.c b/fs/open.c
> > index 8a813fa5ca56..0831433e493a 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
> >  {
> >         struct filename *name = getname_kernel(filename);
> >         struct file *file = ERR_CAST(name);
> > -
> > +
>
> See my comment above about unnecessary changes.

Same here, I'm fixing that for the next version.

Thanks for the review,
Günther

--

Tetsuo Handa Sept. 8, 2022, 10:04 p.m. UTC | #4

On 2022/09/09 5:50, Günther Noack wrote:
> On Thu, Sep 08, 2022 at 04:09:06PM -0400, Paul Moore wrote:
>> On Thu, Sep 8, 2022 at 3:58 PM Günther Noack <[e-mail]> wrote:
>>>
>>> Like path_truncate, the file_truncate hook also restricts file
>>> truncation, but is called in the cases where truncation is attempted
>>> on an already-opened file.
>>>
>>> This is required in a subsequent commit to handle ftruncate()
>>> operations differently to truncate() operations.
>>>
>>> Signed-off-by: Günther Noack <[e-mail]>
>>
>> We need to get John and Tetsuo's ACKs on this patch, but in addition
>> to that I have two small comments (below).
> 
> +CC: John Johansen and Tetsuo Handa -- this change is splitting up the
> path_truncate LSM hook into a path_truncate and file_truncate variant,
> one operating on the path as before, and one operating on a struct
> file*. As a result, AppArmor and TOMOYO need to implement the
> file-based hook as well and treat it the same as before by looking at
> the file's ->f_path. Does this change seem reasonable to you?

Regarding TOMOYO part,

Acked-by: Tetsuo Handa <[e-mail]>

John Johansen Sept. 9, 2022, 3:37 a.m. UTC | #5

On 9/8/22 12:58, Günther Noack wrote:
> Like path_truncate, the file_truncate hook also restricts file
> truncation, but is called in the cases where truncation is attempted
> on an already-opened file.
> 
> This is required in a subsequent commit to handle ftruncate()
> operations differently to truncate() operations.
> 
> Signed-off-by: Günther Noack <[e-mail]>

Acked-by: John Johansen <[e-mail]>

> ---
>   fs/namei.c                    |  6 +++---
>   fs/open.c                     |  4 ++--
>   include/linux/lsm_hook_defs.h |  1 +
>   include/linux/security.h      |  6 ++++++
>   security/apparmor/lsm.c       |  6 ++++++
>   security/security.c           |  5 +++++
>   security/tomoyo/tomoyo.c      | 13 +++++++++++++
>   7 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 53b4bc094db2..52105873d1f8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
>    * The new code replaces the old recursive symlink resolution with
>    * an iterative one (in case of non-nested symlink chains).  It does
>    * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all
>    * the special cases of the former code.
>    *
>    * With the new dcache, the pathname is stored at each inode, at least as
> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
>   	if (error)
>   		return error;
>   
> -	error = security_path_truncate(path);
> +	error = security_file_truncate(filp);
>   	if (!error) {
>   		error = do_truncate(mnt_userns, path->dentry, 0,
>   				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> diff --git a/fs/open.c b/fs/open.c
> index 8a813fa5ca56..0831433e493a 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>   	if (IS_APPEND(file_inode(f.file)))
>   		goto out_putf;
>   	sb_start_write(inode->i_sb);
> -	error = security_path_truncate(&f.file->f_path);
> +	error = security_file_truncate(f.file);
>   	if (!error)
>   		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>   				    ATTR_MTIME | ATTR_CTIME, f.file);
> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
>   {
>   	struct filename *name = getname_kernel(filename);
>   	struct file *file = ERR_CAST(name);
> -	
> +
>   	if (!IS_ERR(name)) {
>   		file = file_open_name(name, flags, mode);
>   		putname(name);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 60fff133c0b1..dee35ab253ba 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
>   	 struct fown_struct *fown, int sig)
>   LSM_HOOK(int, 0, file_receive, struct file *file)
>   LSM_HOOK(int, 0, file_open, struct file *file)
> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>   LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>   	 unsigned long clone_flags)
>   LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 7bd0c490703d..f80b23382dd9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>   				 struct fown_struct *fown, int sig);
>   int security_file_receive(struct file *file);
>   int security_file_open(struct file *file);
> +int security_file_truncate(struct file *file);
>   int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>   void security_task_free(struct task_struct *task);
>   int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
>   	return 0;
>   }
>   
> +static inline int security_file_truncate(struct file *file)
> +{
> +	return 0;
> +}
> +
>   static inline int security_task_alloc(struct task_struct *task,
>   				      unsigned long clone_flags)
>   {
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index e29cade7b662..98ecb7f221b8 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
>   	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
>   }
>   
> +static int apparmor_file_truncate(struct file *file)
> +{
> +	return apparmor_path_truncate(&file->f_path);
> +}
> +
>   static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
>   				 const char *old_name)
>   {
> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>   	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>   	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>   
>   	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>   	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
> diff --git a/security/security.c b/security/security.c
> index 4b95de24bc8d..e491120c48ba 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path *path)
>   	return call_int_hook(path_truncate, 0, path);
>   }
>   
> +int security_file_truncate(struct file *file)
> +{
> +	return call_int_hook(file_truncate, 0, file);
> +}
> +
>   int security_path_chmod(const struct path *path, umode_t mode)
>   {
>   	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 71e82d855ebf..af04a7b7eb28 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
>   	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>   }
>   
> +/**
> + * tomoyo_file_truncate - Target for security_file_truncate().
> + *
> + * @file: Pointer to "struct file".
> + *
> + * Returns 0 on success, negative value otherwise.
> + */
> +static int tomoyo_file_truncate(struct file *file)
> +{
> +	return tomoyo_path_truncate(&file->f_path);
> +}
> +
>   /**
>    * tomoyo_path_unlink - Target for security_path_unlink().
>    *
> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>   	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>   	LSM_HOOK_INIT(file_open, tomoyo_file_open),
> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>   	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>   	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>   	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),

Mickaël Salaün Sept. 9, 2022, 1:50 p.m. UTC | #6

Great! Just the unintended hunks to remove as pointed by Paul.


On 08/09/2022 21:58, Günther Noack wrote:
> Like path_truncate, the file_truncate hook also restricts file
> truncation, but is called in the cases where truncation is attempted
> on an already-opened file.
> 
> This is required in a subsequent commit to handle ftruncate()
> operations differently to truncate() operations.
> 
> Signed-off-by: Günther Noack <[e-mail]>
> ---
>   fs/namei.c                    |  6 +++---
>   fs/open.c                     |  4 ++--
>   include/linux/lsm_hook_defs.h |  1 +
>   include/linux/security.h      |  6 ++++++
>   security/apparmor/lsm.c       |  6 ++++++
>   security/security.c           |  5 +++++
>   security/tomoyo/tomoyo.c      | 13 +++++++++++++
>   7 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 53b4bc094db2..52105873d1f8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -53,8 +53,8 @@
>    * The new code replaces the old recursive symlink resolution with
>    * an iterative one (in case of non-nested symlink chains).  It does
>    * this with calls to <fs>_follow_link().
> - * As a side effect, dir_namei(), _namei() and follow_link() are now
> - * replaced with a single function lookup_dentry() that can handle all
> + * As a side effect, dir_namei(), _namei() and follow_link() are now
> + * replaced with a single function lookup_dentry() that can handle all
>    * the special cases of the former code.
>    *
>    * With the new dcache, the pathname is stored at each inode, at least as
> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
>   	if (error)
>   		return error;
>   
> -	error = security_path_truncate(path);
> +	error = security_file_truncate(filp);
>   	if (!error) {
>   		error = do_truncate(mnt_userns, path->dentry, 0,
>   				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> diff --git a/fs/open.c b/fs/open.c
> index 8a813fa5ca56..0831433e493a 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>   	if (IS_APPEND(file_inode(f.file)))
>   		goto out_putf;
>   	sb_start_write(inode->i_sb);
> -	error = security_path_truncate(&f.file->f_path);
> +	error = security_file_truncate(f.file);
>   	if (!error)
>   		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>   				    ATTR_MTIME | ATTR_CTIME, f.file);
> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
>   {
>   	struct filename *name = getname_kernel(filename);
>   	struct file *file = ERR_CAST(name);
> -	
> +
>   	if (!IS_ERR(name)) {
>   		file = file_open_name(name, flags, mode);
>   		putname(name);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 60fff133c0b1..dee35ab253ba 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
>   	 struct fown_struct *fown, int sig)
>   LSM_HOOK(int, 0, file_receive, struct file *file)
>   LSM_HOOK(int, 0, file_open, struct file *file)
> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>   LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>   	 unsigned long clone_flags)
>   LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 7bd0c490703d..f80b23382dd9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>   				 struct fown_struct *fown, int sig);
>   int security_file_receive(struct file *file);
>   int security_file_open(struct file *file);
> +int security_file_truncate(struct file *file);
>   int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>   void security_task_free(struct task_struct *task);
>   int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
>   	return 0;
>   }
>   
> +static inline int security_file_truncate(struct file *file)
> +{
> +	return 0;
> +}
> +
>   static inline int security_task_alloc(struct task_struct *task,
>   				      unsigned long clone_flags)
>   {
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index e29cade7b662..98ecb7f221b8 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
>   	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
>   }
>   
> +static int apparmor_file_truncate(struct file *file)
> +{
> +	return apparmor_path_truncate(&file->f_path);
> +}
> +
>   static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
>   				 const char *old_name)
>   {
> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>   	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>   	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>   
>   	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>   	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
> diff --git a/security/security.c b/security/security.c
> index 4b95de24bc8d..e491120c48ba 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path *path)
>   	return call_int_hook(path_truncate, 0, path);
>   }
>   
> +int security_file_truncate(struct file *file)
> +{
> +	return call_int_hook(file_truncate, 0, file);
> +}
> +
>   int security_path_chmod(const struct path *path, umode_t mode)
>   {
>   	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 71e82d855ebf..af04a7b7eb28 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
>   	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>   }
>   
> +/**
> + * tomoyo_file_truncate - Target for security_file_truncate().
> + *
> + * @file: Pointer to "struct file".
> + *
> + * Returns 0 on success, negative value otherwise.
> + */
> +static int tomoyo_file_truncate(struct file *file)
> +{
> +	return tomoyo_path_truncate(&file->f_path);
> +}
> +
>   /**
>    * tomoyo_path_unlink - Target for security_path_unlink().
>    *
> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>   	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>   	LSM_HOOK_INIT(file_open, tomoyo_file_open),
> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>   	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>   	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>   	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),

Mickaël Salaün Sept. 16, 2022, 5:30 p.m. UTC | #7

We may indeed need to change fs/open.c:vfs_truncate() because of these 
different call sites. I'm not sure how these subsystems work though.


On 08/09/2022 22:28, Günther Noack wrote:
> Adding Namjae Jeon and David Howells as authors of the respective
> files in fs/ksmbd and fs/cachefiles -- do you happen to know whether
> these vfs_truncate() calls are using 'struct file's that are opened by
> normal userspace processes, where LSM policies may apply?
> 
> P.S. In this patch I have looked for all places where the
> security_path_truncate() hook was called, to see which of these should
> rather use security_file_truncate() (and I made sure that it does the
> same thing for all the LSMs that use it).
> 
> I'm confident that this does the right thing when truncate() or
> ftruncate() are called from userspace, but one of the places that
> still calls the path-based hook is vfs_truncate(), and this is called
> from more places in the kernel than just from userspace:
> 
> init/initramfs.c
> 387:				vfs_truncate(&wfile->f_path, body_len);
> 
> security/keys/big_key.c
> 172:		vfs_truncate(&payload->path, 0);
> 
> fs/cachefiles/interface.c
> 242:		ret = vfs_truncate(&file->f_path, dio_size);
> 
> fs/cachefiles/namei.c
> 497:			ret = vfs_truncate(&path, ni_size); >
> fs/ksmbd/smb2pdu.c
> 2350:	int rc = vfs_truncate(path, 0);
> 
> fs/ksmbd/vfs.c
> 874:	err = vfs_truncate(&filp->f_path, size);
> 
> I suspect that these are benign but am not familiar with all of these
> corners of the codebase. -- The question is: Some of these call
> vfs_truncate() on the f_path of an existing struct file -- should
> these rather be calling the security_file_truncate() than the
> security_path_truncate() hook to authorize the truncation?
> 
> Specifically, I think:
> 
> * initramfs happens at system startup and LSMs should not interfere at
>    this point yet
> * security/keys does not use an opened struct file, so calling the
>    path-based hook through vfs_truncate() is correct
> * fs/cachefiles and fs/ksmbd use the file system from the kernel to
>    expose it as another file system (in a cached form for cachefiles,
>    and over the network for ksmbd). I suspect that these file systems
>    are not handling 'struct file's which are opened in contexts where a
>    LSM applies? It that a reasonable assumption?

I think you're right but I have some doubts about the cachefiles subsystem.
I don't know how ksmb deals with these file descriptors but changing 
such call sites (where there is a struct file) could improve API 
consistency though.
Any though?


> 
> Thanks,
> Günther
> 
> On Thu, Sep 08, 2022 at 09:58:01PM +0200, Günther Noack wrote:
>> Like path_truncate, the file_truncate hook also restricts file
>> truncation, but is called in the cases where truncation is attempted
>> on an already-opened file.
>>
>> This is required in a subsequent commit to handle ftruncate()
>> operations differently to truncate() operations.
>>
>> Signed-off-by: Günther Noack <[e-mail]>
>> ---
>>   fs/namei.c                    |  6 +++---
>>   fs/open.c                     |  4 ++--
>>   include/linux/lsm_hook_defs.h |  1 +
>>   include/linux/security.h      |  6 ++++++
>>   security/apparmor/lsm.c       |  6 ++++++
>>   security/security.c           |  5 +++++
>>   security/tomoyo/tomoyo.c      | 13 +++++++++++++
>>   7 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 53b4bc094db2..52105873d1f8 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -53,8 +53,8 @@
>>    * The new code replaces the old recursive symlink resolution with
>>    * an iterative one (in case of non-nested symlink chains).  It does
>>    * this with calls to <fs>_follow_link().
>> - * As a side effect, dir_namei(), _namei() and follow_link() are now
>> - * replaced with a single function lookup_dentry() that can handle all
>> + * As a side effect, dir_namei(), _namei() and follow_link() are now
>> + * replaced with a single function lookup_dentry() that can handle all
>>    * the special cases of the former code.
>>    *
>>    * With the new dcache, the pathname is stored at each inode, at least as
>> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
>>   	if (error)
>>   		return error;
>>
>> -	error = security_path_truncate(path);
>> +	error = security_file_truncate(filp);
>>   	if (!error) {
>>   		error = do_truncate(mnt_userns, path->dentry, 0,
>>   				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
>> diff --git a/fs/open.c b/fs/open.c
>> index 8a813fa5ca56..0831433e493a 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>>   	if (IS_APPEND(file_inode(f.file)))
>>   		goto out_putf;
>>   	sb_start_write(inode->i_sb);
>> -	error = security_path_truncate(&f.file->f_path);
>> +	error = security_file_truncate(f.file);
>>   	if (!error)
>>   		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>>   				    ATTR_MTIME | ATTR_CTIME, f.file);
>> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
>>   {
>>   	struct filename *name = getname_kernel(filename);
>>   	struct file *file = ERR_CAST(name);
>> -
>> +
>>   	if (!IS_ERR(name)) {
>>   		file = file_open_name(name, flags, mode);
>>   		putname(name);
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index 60fff133c0b1..dee35ab253ba 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
>>   	 struct fown_struct *fown, int sig)
>>   LSM_HOOK(int, 0, file_receive, struct file *file)
>>   LSM_HOOK(int, 0, file_open, struct file *file)
>> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>>   LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>>   	 unsigned long clone_flags)
>>   LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 7bd0c490703d..f80b23382dd9 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>>   				 struct fown_struct *fown, int sig);
>>   int security_file_receive(struct file *file);
>>   int security_file_open(struct file *file);
>> +int security_file_truncate(struct file *file);
>>   int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>>   void security_task_free(struct task_struct *task);
>>   int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
>>   	return 0;
>>   }
>>
>> +static inline int security_file_truncate(struct file *file)
>> +{
>> +	return 0;
>> +}
>> +
>>   static inline int security_task_alloc(struct task_struct *task,
>>   				      unsigned long clone_flags)
>>   {
>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>> index e29cade7b662..98ecb7f221b8 100644
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
>> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
>>   	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
>>   }
>>
>> +static int apparmor_file_truncate(struct file *file)
>> +{
>> +	return apparmor_path_truncate(&file->f_path);
>> +}
>> +
>>   static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
>>   				 const char *old_name)
>>   {
>> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>>   	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>>   	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>>   	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
>> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>>
>>   	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>>   	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
>> diff --git a/security/security.c b/security/security.c
>> index 4b95de24bc8d..e491120c48ba 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path *path)
>>   	return call_int_hook(path_truncate, 0, path);
>>   }
>>
>> +int security_file_truncate(struct file *file)
>> +{
>> +	return call_int_hook(file_truncate, 0, file);
>> +}
>> +
>>   int security_path_chmod(const struct path *path, umode_t mode)
>>   {
>>   	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>> index 71e82d855ebf..af04a7b7eb28 100644
>> --- a/security/tomoyo/tomoyo.c
>> +++ b/security/tomoyo/tomoyo.c
>> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
>>   	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>>   }
>>
>> +/**
>> + * tomoyo_file_truncate - Target for security_file_truncate().
>> + *
>> + * @file: Pointer to "struct file".
>> + *
>> + * Returns 0 on success, negative value otherwise.
>> + */
>> +static int tomoyo_file_truncate(struct file *file)
>> +{
>> +	return tomoyo_path_truncate(&file->f_path);
>> +}
>> +
>>   /**
>>    * tomoyo_path_unlink - Target for security_path_unlink().
>>    *
>> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>>   	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>>   	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>>   	LSM_HOOK_INIT(file_open, tomoyo_file_open),
>> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>>   	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>>   	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>>   	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),
>> --
>> 2.37.3
>>
> 
> --

Günther Noack Sept. 26, 2022, 4:07 p.m. UTC | #8

On Fri, Sep 16, 2022 at 07:30:24PM +0200, Mickaël Salaün wrote:
> We may indeed need to change fs/open.c:vfs_truncate() because of these
> different call sites. I'm not sure how these subsystems work though.

I thought about this some more, and I'm coming around to the
conclusion that we should not block the truncate patch set on changes
in ksmbd and cachefiles.

The reasoning is:

* Landlock does already work for ksmbd and cachefiles. vfs_truncate
  does call the security_path_truncate() hook in the background.

* ksmbd and cachefiles using vfs_truncate() in kernel space is roughly
  equivalent to a user space program using truncate(2) in a place
  where ftruncate(2) is possible. It might not be the most elegant
  approach, but it's legitimate to do.

* Like with any userspace program that is supposed to run under
  Landlock, ksmbd and cachefiles both may need to be adapted slightly
  to work well with Landlock enforcement. It is up to the person
  adding the Landlock enforcement to double check that the program
  works correctly under the enforced ruleset. This is true for both
  programs running in user space and kernel space.

So yes, to run ksmbd and cachefiles under Landlock, we may need to
extract a fs/open.c:vfs_ftruncate() in addition to vfs_truncate(), but
I don't think it should be part of this patch set.

So my proposal would be to:

* not do the ksmbd and cachefiles changes now,

* but leave them for later when someone actually tries to run ksmbd or
  cachefiles under Landlock.

If these components never get executed in a Landlocked context, all
the better - we can spare ourselves a more complicated refactoring in
a core part of the kernel.

FWIW, I've played around with it yesterday and found that the change
to extract a new "vfs_ftruncate()" next to vfs_truncate() is
reasonably self-contained. But I'm not a file system expert either,
it's well possible that I'm overlooking something.

Let me know what you think!

> On 08/09/2022 22:28, Günther Noack wrote:
> > Adding Namjae Jeon and David Howells as authors of the respective
> > files in fs/ksmbd and fs/cachefiles -- do you happen to know whether
> > these vfs_truncate() calls are using 'struct file's that are opened by
> > normal userspace processes, where LSM policies may apply?
> > 
> > P.S. In this patch I have looked for all places where the
> > security_path_truncate() hook was called, to see which of these should
> > rather use security_file_truncate() (and I made sure that it does the
> > same thing for all the LSMs that use it).
> > 
> > I'm confident that this does the right thing when truncate() or
> > ftruncate() are called from userspace, but one of the places that
> > still calls the path-based hook is vfs_truncate(), and this is called
> > from more places in the kernel than just from userspace:
> > 
> > init/initramfs.c
> > 387:				vfs_truncate(&wfile->f_path, body_len);
> > 
> > security/keys/big_key.c
> > 172:		vfs_truncate(&payload->path, 0);
> > 
> > fs/cachefiles/interface.c
> > 242:		ret = vfs_truncate(&file->f_path, dio_size);
> > 
> > fs/cachefiles/namei.c
> > 497:			ret = vfs_truncate(&path, ni_size); >
> > fs/ksmbd/smb2pdu.c
> > 2350:	int rc = vfs_truncate(path, 0);
> > 
> > fs/ksmbd/vfs.c
> > 874:	err = vfs_truncate(&filp->f_path, size);
> > 
> > I suspect that these are benign but am not familiar with all of these
> > corners of the codebase. -- The question is: Some of these call
> > vfs_truncate() on the f_path of an existing struct file -- should
> > these rather be calling the security_file_truncate() than the
> > security_path_truncate() hook to authorize the truncation?
> > 
> > Specifically, I think:
> > 
> > * initramfs happens at system startup and LSMs should not interfere at
> >    this point yet
> > * security/keys does not use an opened struct file, so calling the
> >    path-based hook through vfs_truncate() is correct
> > * fs/cachefiles and fs/ksmbd use the file system from the kernel to
> >    expose it as another file system (in a cached form for cachefiles,
> >    and over the network for ksmbd). I suspect that these file systems
> >    are not handling 'struct file's which are opened in contexts where a
> >    LSM applies? It that a reasonable assumption?
> 
> I think you're right but I have some doubts about the cachefiles subsystem.
> I don't know how ksmb deals with these file descriptors but changing such
> call sites (where there is a struct file) could improve API consistency
> though.
> Any though?

My conclusion is already summarized above, and I've tried to abstract
away from the concrete use cases. For completeness, I've also looked
into ksmbd and cachefiles specifically though so see whether
security_path_truncate and security_file_truncate would make a
difference.

For ksmbd, I strongly suspect it does not make a difference (90%
confidence) -- the files are getting opened by the same request
handler context which is also truncating the files later on behalf of
a truncation operation in the SMB protocol. It's anyway unclear to me
whether the kernel tasks executing this can be put under Landlock
enforcement at all..?

fs/cachefiles is a more layered system and uses some
cachefiles-independent caching structures with void* pointers, whose
values I found difficult to trace. I'm less certain about this one as
well, but as discussed above, it does not make a difference as long as
none of the cachefiles code executes in a Landlock context. I'm still
in favor of decoupling potential ksmbd and cachefiles changes from
this patch set.

—Günther

> 
> 
> > 
> > Thanks,
> > Günther
> > 
> > On Thu, Sep 08, 2022 at 09:58:01PM +0200, Günther Noack wrote:
> > > Like path_truncate, the file_truncate hook also restricts file
> > > truncation, but is called in the cases where truncation is attempted
> > > on an already-opened file.
> > > 
> > > This is required in a subsequent commit to handle ftruncate()
> > > operations differently to truncate() operations.
> > > 
> > > Signed-off-by: Günther Noack <[e-mail]>
> > > ---
> > >   fs/namei.c                    |  6 +++---
> > >   fs/open.c                     |  4 ++--
> > >   include/linux/lsm_hook_defs.h |  1 +
> > >   include/linux/security.h      |  6 ++++++
> > >   security/apparmor/lsm.c       |  6 ++++++
> > >   security/security.c           |  5 +++++
> > >   security/tomoyo/tomoyo.c      | 13 +++++++++++++
> > >   7 files changed, 36 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 53b4bc094db2..52105873d1f8 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -53,8 +53,8 @@
> > >    * The new code replaces the old recursive symlink resolution with
> > >    * an iterative one (in case of non-nested symlink chains).  It does
> > >    * this with calls to <fs>_follow_link().
> > > - * As a side effect, dir_namei(), _namei() and follow_link() are now
> > > - * replaced with a single function lookup_dentry() that can handle all
> > > + * As a side effect, dir_namei(), _namei() and follow_link() are now
> > > + * replaced with a single function lookup_dentry() that can handle all
> > >    * the special cases of the former code.
> > >    *
> > >    * With the new dcache, the pathname is stored at each inode, at least as
> > > @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
> > >   	if (error)
> > >   		return error;
> > > 
> > > -	error = security_path_truncate(path);
> > > +	error = security_file_truncate(filp);
> > >   	if (!error) {
> > >   		error = do_truncate(mnt_userns, path->dentry, 0,
> > >   				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> > > diff --git a/fs/open.c b/fs/open.c
> > > index 8a813fa5ca56..0831433e493a 100644
> > > --- a/fs/open.c
> > > +++ b/fs/open.c
> > > @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> > >   	if (IS_APPEND(file_inode(f.file)))
> > >   		goto out_putf;
> > >   	sb_start_write(inode->i_sb);
> > > -	error = security_path_truncate(&f.file->f_path);
> > > +	error = security_file_truncate(f.file);
> > >   	if (!error)
> > >   		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
> > >   				    ATTR_MTIME | ATTR_CTIME, f.file);
> > > @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
> > >   {
> > >   	struct filename *name = getname_kernel(filename);
> > >   	struct file *file = ERR_CAST(name);
> > > -
> > > +
> > >   	if (!IS_ERR(name)) {
> > >   		file = file_open_name(name, flags, mode);
> > >   		putname(name);
> > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > > index 60fff133c0b1..dee35ab253ba 100644
> > > --- a/include/linux/lsm_hook_defs.h
> > > +++ b/include/linux/lsm_hook_defs.h
> > > @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
> > >   	 struct fown_struct *fown, int sig)
> > >   LSM_HOOK(int, 0, file_receive, struct file *file)
> > >   LSM_HOOK(int, 0, file_open, struct file *file)
> > > +LSM_HOOK(int, 0, file_truncate, struct file *file)
> > >   LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
> > >   	 unsigned long clone_flags)
> > >   LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 7bd0c490703d..f80b23382dd9 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
> > >   				 struct fown_struct *fown, int sig);
> > >   int security_file_receive(struct file *file);
> > >   int security_file_open(struct file *file);
> > > +int security_file_truncate(struct file *file);
> > >   int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
> > >   void security_task_free(struct task_struct *task);
> > >   int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> > > @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
> > >   	return 0;
> > >   }
> > > 
> > > +static inline int security_file_truncate(struct file *file)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > >   static inline int security_task_alloc(struct task_struct *task,
> > >   				      unsigned long clone_flags)
> > >   {
> > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > index e29cade7b662..98ecb7f221b8 100644
> > > --- a/security/apparmor/lsm.c
> > > +++ b/security/apparmor/lsm.c
> > > @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
> > >   	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
> > >   }
> > > 
> > > +static int apparmor_file_truncate(struct file *file)
> > > +{
> > > +	return apparmor_path_truncate(&file->f_path);
> > > +}
> > > +
> > >   static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
> > >   				 const char *old_name)
> > >   {
> > > @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> > >   	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
> > >   	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
> > >   	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
> > > +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
> > > 
> > >   	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
> > >   	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
> > > diff --git a/security/security.c b/security/security.c
> > > index 4b95de24bc8d..e491120c48ba 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path *path)
> > >   	return call_int_hook(path_truncate, 0, path);
> > >   }
> > > 
> > > +int security_file_truncate(struct file *file)
> > > +{
> > > +	return call_int_hook(file_truncate, 0, file);
> > > +}
> > > +
> > >   int security_path_chmod(const struct path *path, umode_t mode)
> > >   {
> > >   	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
> > > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> > > index 71e82d855ebf..af04a7b7eb28 100644
> > > --- a/security/tomoyo/tomoyo.c
> > > +++ b/security/tomoyo/tomoyo.c
> > > @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
> > >   	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
> > >   }
> > > 
> > > +/**
> > > + * tomoyo_file_truncate - Target for security_file_truncate().
> > > + *
> > > + * @file: Pointer to "struct file".
> > > + *
> > > + * Returns 0 on success, negative value otherwise.
> > > + */
> > > +static int tomoyo_file_truncate(struct file *file)
> > > +{
> > > +	return tomoyo_path_truncate(&file->f_path);
> > > +}
> > > +
> > >   /**
> > >    * tomoyo_path_unlink - Target for security_path_unlink().
> > >    *
> > > @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
> > >   	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
> > >   	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
> > >   	LSM_HOOK_INIT(file_open, tomoyo_file_open),
> > > +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
> > >   	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
> > >   	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
> > >   	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),
> > > --
> > > 2.37.3
> > > 
> > 
> > --

--

Mickaël Salaün Sept. 28, 2022, 8:04 p.m. UTC | #9

On 26/09/2022 18:07, Günther Noack wrote:
> On Fri, Sep 16, 2022 at 07:30:24PM +0200, Mickaël Salaün wrote:
>> We may indeed need to change fs/open.c:vfs_truncate() because of these
>> different call sites. I'm not sure how these subsystems work though.
> 
> I thought about this some more, and I'm coming around to the
> conclusion that we should not block the truncate patch set on changes
> in ksmbd and cachefiles.
> 
> The reasoning is:
> 
> * Landlock does already work for ksmbd and cachefiles. vfs_truncate
>    does call the security_path_truncate() hook in the background.
> 
> * ksmbd and cachefiles using vfs_truncate() in kernel space is roughly
>    equivalent to a user space program using truncate(2) in a place
>    where ftruncate(2) is possible. It might not be the most elegant
>    approach, but it's legitimate to do.
> 
> * Like with any userspace program that is supposed to run under
>    Landlock, ksmbd and cachefiles both may need to be adapted slightly
>    to work well with Landlock enforcement. It is up to the person
>    adding the Landlock enforcement to double check that the program
>    works correctly under the enforced ruleset. This is true for both
>    programs running in user space and kernel space.
> 
> So yes, to run ksmbd and cachefiles under Landlock, we may need to
> extract a fs/open.c:vfs_ftruncate() in addition to vfs_truncate(), but
> I don't think it should be part of this patch set.
> 
> So my proposal would be to:
> 
> * not do the ksmbd and cachefiles changes now,
> 
> * but leave them for later when someone actually tries to run ksmbd or
>    cachefiles under Landlock.
> 
> If these components never get executed in a Landlocked context, all
> the better - we can spare ourselves a more complicated refactoring in
> a core part of the kernel.

 From my understanding, ksmbd should be treated as a process, but 
without file descriptors, which excludes it from calling ftruncate-like 
interfaces. Furthermore, I think ksmbd cannot be sandboxed because it 
calls prepare_kernel_cred(NULL) which then uses init_cred.

As a side node, using current_user_ns() in this context looks like a 
bug… I think it should be &init_user_ns instead. Any though Namjae, 
Steve or Hyunchul?

About cachefiles, I think it should be OK to ignore it, but I'd really 
like to get some input from file system folks. Any though David or 
Christian?


> 
> FWIW, I've played around with it yesterday and found that the change
> to extract a new "vfs_ftruncate()" next to vfs_truncate() is
> reasonably self-contained. But I'm not a file system expert either,
> it's well possible that I'm overlooking something.
> 
> Let me know what you think!
> 
>> On 08/09/2022 22:28, Günther Noack wrote:
>>> Adding Namjae Jeon and David Howells as authors of the respective
>>> files in fs/ksmbd and fs/cachefiles -- do you happen to know whether
>>> these vfs_truncate() calls are using 'struct file's that are opened by
>>> normal userspace processes, where LSM policies may apply?
>>>
>>> P.S. In this patch I have looked for all places where the
>>> security_path_truncate() hook was called, to see which of these should
>>> rather use security_file_truncate() (and I made sure that it does the
>>> same thing for all the LSMs that use it).
>>>
>>> I'm confident that this does the right thing when truncate() or
>>> ftruncate() are called from userspace, but one of the places that
>>> still calls the path-based hook is vfs_truncate(), and this is called
>>> from more places in the kernel than just from userspace:
>>>
>>> init/initramfs.c
>>> 387:				vfs_truncate(&wfile->f_path, body_len);
>>>
>>> security/keys/big_key.c
>>> 172:		vfs_truncate(&payload->path, 0);
>>>
>>> fs/cachefiles/interface.c
>>> 242:		ret = vfs_truncate(&file->f_path, dio_size);
>>>
>>> fs/cachefiles/namei.c
>>> 497:			ret = vfs_truncate(&path, ni_size); >
>>> fs/ksmbd/smb2pdu.c
>>> 2350:	int rc = vfs_truncate(path, 0);
>>>
>>> fs/ksmbd/vfs.c
>>> 874:	err = vfs_truncate(&filp->f_path, size);
>>>
>>> I suspect that these are benign but am not familiar with all of these
>>> corners of the codebase. -- The question is: Some of these call
>>> vfs_truncate() on the f_path of an existing struct file -- should
>>> these rather be calling the security_file_truncate() than the
>>> security_path_truncate() hook to authorize the truncation?
>>>
>>> Specifically, I think:
>>>
>>> * initramfs happens at system startup and LSMs should not interfere at
>>>     this point yet
>>> * security/keys does not use an opened struct file, so calling the
>>>     path-based hook through vfs_truncate() is correct
>>> * fs/cachefiles and fs/ksmbd use the file system from the kernel to
>>>     expose it as another file system (in a cached form for cachefiles,
>>>     and over the network for ksmbd). I suspect that these file systems
>>>     are not handling 'struct file's which are opened in contexts where a
>>>     LSM applies? It that a reasonable assumption?
>>
>> I think you're right but I have some doubts about the cachefiles subsystem.
>> I don't know how ksmb deals with these file descriptors but changing such
>> call sites (where there is a struct file) could improve API consistency
>> though.
>> Any though?
> 
> My conclusion is already summarized above, and I've tried to abstract
> away from the concrete use cases. For completeness, I've also looked
> into ksmbd and cachefiles specifically though so see whether
> security_path_truncate and security_file_truncate would make a
> difference.
> 
> For ksmbd, I strongly suspect it does not make a difference (90%
> confidence) -- the files are getting opened by the same request
> handler context which is also truncating the files later on behalf of
> a truncation operation in the SMB protocol. It's anyway unclear to me
> whether the kernel tasks executing this can be put under Landlock
> enforcement at all..?
> 
> fs/cachefiles is a more layered system and uses some
> cachefiles-independent caching structures with void* pointers, whose
> values I found difficult to trace. I'm less certain about this one as
> well, but as discussed above, it does not make a difference as long as
> none of the cachefiles code executes in a Landlock context. I'm still
> in favor of decoupling potential ksmbd and cachefiles changes from
> this patch set.
> 
> —Günther
> 
>>
>>
>>>
>>> Thanks,
>>> Günther
>>>
>>> On Thu, Sep 08, 2022 at 09:58:01PM +0200, Günther Noack wrote:
>>>> Like path_truncate, the file_truncate hook also restricts file
>>>> truncation, but is called in the cases where truncation is attempted
>>>> on an already-opened file.
>>>>
>>>> This is required in a subsequent commit to handle ftruncate()
>>>> operations differently to truncate() operations.
>>>>
>>>> Signed-off-by: Günther Noack <[e-mail]>
>>>> ---
>>>>    fs/namei.c                    |  6 +++---
>>>>    fs/open.c                     |  4 ++--
>>>>    include/linux/lsm_hook_defs.h |  1 +
>>>>    include/linux/security.h      |  6 ++++++
>>>>    security/apparmor/lsm.c       |  6 ++++++
>>>>    security/security.c           |  5 +++++
>>>>    security/tomoyo/tomoyo.c      | 13 +++++++++++++
>>>>    7 files changed, 36 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/namei.c b/fs/namei.c
>>>> index 53b4bc094db2..52105873d1f8 100644
>>>> --- a/fs/namei.c
>>>> +++ b/fs/namei.c
>>>> @@ -53,8 +53,8 @@
>>>>     * The new code replaces the old recursive symlink resolution with
>>>>     * an iterative one (in case of non-nested symlink chains).  It does
>>>>     * this with calls to <fs>_follow_link().
>>>> - * As a side effect, dir_namei(), _namei() and follow_link() are now
>>>> - * replaced with a single function lookup_dentry() that can handle all
>>>> + * As a side effect, dir_namei(), _namei() and follow_link() are now
>>>> + * replaced with a single function lookup_dentry() that can handle all
>>>>     * the special cases of the former code.
>>>>     *
>>>>     * With the new dcache, the pathname is stored at each inode, at least as
>>>> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
>>>>    	if (error)
>>>>    		return error;
>>>>
>>>> -	error = security_path_truncate(path);
>>>> +	error = security_file_truncate(filp);
>>>>    	if (!error) {
>>>>    		error = do_truncate(mnt_userns, path->dentry, 0,
>>>>    				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
>>>> diff --git a/fs/open.c b/fs/open.c
>>>> index 8a813fa5ca56..0831433e493a 100644
>>>> --- a/fs/open.c
>>>> +++ b/fs/open.c
>>>> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>>>>    	if (IS_APPEND(file_inode(f.file)))
>>>>    		goto out_putf;
>>>>    	sb_start_write(inode->i_sb);
>>>> -	error = security_path_truncate(&f.file->f_path);
>>>> +	error = security_file_truncate(f.file);
>>>>    	if (!error)
>>>>    		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>>>>    				    ATTR_MTIME | ATTR_CTIME, f.file);
>>>> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int flags, umode_t mode)
>>>>    {
>>>>    	struct filename *name = getname_kernel(filename);
>>>>    	struct file *file = ERR_CAST(name);
>>>> -
>>>> +
>>>>    	if (!IS_ERR(name)) {
>>>>    		file = file_open_name(name, flags, mode);
>>>>    		putname(name);
>>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>>> index 60fff133c0b1..dee35ab253ba 100644
>>>> --- a/include/linux/lsm_hook_defs.h
>>>> +++ b/include/linux/lsm_hook_defs.h
>>>> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
>>>>    	 struct fown_struct *fown, int sig)
>>>>    LSM_HOOK(int, 0, file_receive, struct file *file)
>>>>    LSM_HOOK(int, 0, file_open, struct file *file)
>>>> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>>>>    LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>>>>    	 unsigned long clone_flags)
>>>>    LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
>>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>>> index 7bd0c490703d..f80b23382dd9 100644
>>>> --- a/include/linux/security.h
>>>> +++ b/include/linux/security.h
>>>> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>>>>    				 struct fown_struct *fown, int sig);
>>>>    int security_file_receive(struct file *file);
>>>>    int security_file_open(struct file *file);
>>>> +int security_file_truncate(struct file *file);
>>>>    int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>>>>    void security_task_free(struct task_struct *task);
>>>>    int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>>>> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file *file)
>>>>    	return 0;
>>>>    }
>>>>
>>>> +static inline int security_file_truncate(struct file *file)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static inline int security_task_alloc(struct task_struct *task,
>>>>    				      unsigned long clone_flags)
>>>>    {
>>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>>> index e29cade7b662..98ecb7f221b8 100644
>>>> --- a/security/apparmor/lsm.c
>>>> +++ b/security/apparmor/lsm.c
>>>> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct path *path)
>>>>    	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
>>>>    }
>>>>
>>>> +static int apparmor_file_truncate(struct file *file)
>>>> +{
>>>> +	return apparmor_path_truncate(&file->f_path);
>>>> +}
>>>> +
>>>>    static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
>>>>    				 const char *old_name)
>>>>    {
>>>> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
>>>>    	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>>>>    	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>>>>    	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
>>>> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>>>>
>>>>    	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>>>>    	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 4b95de24bc8d..e491120c48ba 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path *path)
>>>>    	return call_int_hook(path_truncate, 0, path);
>>>>    }
>>>>
>>>> +int security_file_truncate(struct file *file)
>>>> +{
>>>> +	return call_int_hook(file_truncate, 0, file);
>>>> +}
>>>> +
>>>>    int security_path_chmod(const struct path *path, umode_t mode)
>>>>    {
>>>>    	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
>>>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>>>> index 71e82d855ebf..af04a7b7eb28 100644
>>>> --- a/security/tomoyo/tomoyo.c
>>>> +++ b/security/tomoyo/tomoyo.c
>>>> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path *path)
>>>>    	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>>>>    }
>>>>
>>>> +/**
>>>> + * tomoyo_file_truncate - Target for security_file_truncate().
>>>> + *
>>>> + * @file: Pointer to "struct file".
>>>> + *
>>>> + * Returns 0 on success, negative value otherwise.
>>>> + */
>>>> +static int tomoyo_file_truncate(struct file *file)
>>>> +{
>>>> +	return tomoyo_path_truncate(&file->f_path);
>>>> +}
>>>> +
>>>>    /**
>>>>     * tomoyo_path_unlink - Target for security_path_unlink().
>>>>     *
>>>> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
>>>>    	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>>>>    	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>>>>    	LSM_HOOK_INIT(file_open, tomoyo_file_open),
>>>> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>>>>    	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>>>>    	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>>>>    	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),
>>>> --
>>>> 2.37.3
>>>>
>>>
>>> --
>

Namjae Jeon Sept. 29, 2022, 2:55 a.m. UTC | #10

2022-09-29 5:04 GMT+09:00, Mickaël Salaün <[e-mail]>:
>
> On 26/09/2022 18:07, Günther Noack wrote:
>> On Fri, Sep 16, 2022 at 07:30:24PM +0200, Mickaël Salaün wrote:
>>> We may indeed need to change fs/open.c:vfs_truncate() because of these
>>> different call sites. I'm not sure how these subsystems work though.
>>
>> I thought about this some more, and I'm coming around to the
>> conclusion that we should not block the truncate patch set on changes
>> in ksmbd and cachefiles.
>>
>> The reasoning is:
>>
>> * Landlock does already work for ksmbd and cachefiles. vfs_truncate
>>    does call the security_path_truncate() hook in the background.
>>
>> * ksmbd and cachefiles using vfs_truncate() in kernel space is roughly
>>    equivalent to a user space program using truncate(2) in a place
>>    where ftruncate(2) is possible. It might not be the most elegant
>>    approach, but it's legitimate to do.
>>
>> * Like with any userspace program that is supposed to run under
>>    Landlock, ksmbd and cachefiles both may need to be adapted slightly
>>    to work well with Landlock enforcement. It is up to the person
>>    adding the Landlock enforcement to double check that the program
>>    works correctly under the enforced ruleset. This is true for both
>>    programs running in user space and kernel space.
>>
>> So yes, to run ksmbd and cachefiles under Landlock, we may need to
>> extract a fs/open.c:vfs_ftruncate() in addition to vfs_truncate(), but
>> I don't think it should be part of this patch set.
>>
>> So my proposal would be to:
>>
>> * not do the ksmbd and cachefiles changes now,
>>
>> * but leave them for later when someone actually tries to run ksmbd or
>>    cachefiles under Landlock.
>>
>> If these components never get executed in a Landlocked context, all
>> the better - we can spare ourselves a more complicated refactoring in
>> a core part of the kernel.
>
>  From my understanding, ksmbd should be treated as a process, but
> without file descriptors, which excludes it from calling ftruncate-like
> interfaces. Furthermore, I think ksmbd cannot be sandboxed because it
> calls prepare_kernel_cred(NULL) which then uses init_cred.
>
> As a side node, using current_user_ns() in this context looks like a
> bug… I think it should be &init_user_ns instead. Any though Namjae,
> Steve or Hyunchul?
Agreed, Could you please send the patch for this to the list ?

Thanks!
>
> About cachefiles, I think it should be OK to ignore it, but I'd really
> like to get some input from file system folks. Any though David or
> Christian?
>
>
>>
>> FWIW, I've played around with it yesterday and found that the change
>> to extract a new "vfs_ftruncate()" next to vfs_truncate() is
>> reasonably self-contained. But I'm not a file system expert either,
>> it's well possible that I'm overlooking something.
>>
>> Let me know what you think!
>>
>>> On 08/09/2022 22:28, Günther Noack wrote:
>>>> Adding Namjae Jeon and David Howells as authors of the respective
>>>> files in fs/ksmbd and fs/cachefiles -- do you happen to know whether
>>>> these vfs_truncate() calls are using 'struct file's that are opened by
>>>> normal userspace processes, where LSM policies may apply?
>>>>
>>>> P.S. In this patch I have looked for all places where the
>>>> security_path_truncate() hook was called, to see which of these should
>>>> rather use security_file_truncate() (and I made sure that it does the
>>>> same thing for all the LSMs that use it).
>>>>
>>>> I'm confident that this does the right thing when truncate() or
>>>> ftruncate() are called from userspace, but one of the places that
>>>> still calls the path-based hook is vfs_truncate(), and this is called
>>>> from more places in the kernel than just from userspace:
>>>>
>>>> init/initramfs.c
>>>> 387:				vfs_truncate(&wfile->f_path, body_len);
>>>>
>>>> security/keys/big_key.c
>>>> 172:		vfs_truncate(&payload->path, 0);
>>>>
>>>> fs/cachefiles/interface.c
>>>> 242:		ret = vfs_truncate(&file->f_path, dio_size);
>>>>
>>>> fs/cachefiles/namei.c
>>>> 497:			ret = vfs_truncate(&path, ni_size); >
>>>> fs/ksmbd/smb2pdu.c
>>>> 2350:	int rc = vfs_truncate(path, 0);
>>>>
>>>> fs/ksmbd/vfs.c
>>>> 874:	err = vfs_truncate(&filp->f_path, size);
>>>>
>>>> I suspect that these are benign but am not familiar with all of these
>>>> corners of the codebase. -- The question is: Some of these call
>>>> vfs_truncate() on the f_path of an existing struct file -- should
>>>> these rather be calling the security_file_truncate() than the
>>>> security_path_truncate() hook to authorize the truncation?
>>>>
>>>> Specifically, I think:
>>>>
>>>> * initramfs happens at system startup and LSMs should not interfere at
>>>>     this point yet
>>>> * security/keys does not use an opened struct file, so calling the
>>>>     path-based hook through vfs_truncate() is correct
>>>> * fs/cachefiles and fs/ksmbd use the file system from the kernel to
>>>>     expose it as another file system (in a cached form for cachefiles,
>>>>     and over the network for ksmbd). I suspect that these file systems
>>>>     are not handling 'struct file's which are opened in contexts where
>>>> a
>>>>     LSM applies? It that a reasonable assumption?
>>>
>>> I think you're right but I have some doubts about the cachefiles
>>> subsystem.
>>> I don't know how ksmb deals with these file descriptors but changing
>>> such
>>> call sites (where there is a struct file) could improve API consistency
>>> though.
>>> Any though?
>>
>> My conclusion is already summarized above, and I've tried to abstract
>> away from the concrete use cases. For completeness, I've also looked
>> into ksmbd and cachefiles specifically though so see whether
>> security_path_truncate and security_file_truncate would make a
>> difference.
>>
>> For ksmbd, I strongly suspect it does not make a difference (90%
>> confidence) -- the files are getting opened by the same request
>> handler context which is also truncating the files later on behalf of
>> a truncation operation in the SMB protocol. It's anyway unclear to me
>> whether the kernel tasks executing this can be put under Landlock
>> enforcement at all..?
>>
>> fs/cachefiles is a more layered system and uses some
>> cachefiles-independent caching structures with void* pointers, whose
>> values I found difficult to trace. I'm less certain about this one as
>> well, but as discussed above, it does not make a difference as long as
>> none of the cachefiles code executes in a Landlock context. I'm still
>> in favor of decoupling potential ksmbd and cachefiles changes from
>> this patch set.
>>
>> —Günther
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Günther
>>>>
>>>> On Thu, Sep 08, 2022 at 09:58:01PM +0200, Günther Noack wrote:
>>>>> Like path_truncate, the file_truncate hook also restricts file
>>>>> truncation, but is called in the cases where truncation is attempted
>>>>> on an already-opened file.
>>>>>
>>>>> This is required in a subsequent commit to handle ftruncate()
>>>>> operations differently to truncate() operations.
>>>>>
>>>>> Signed-off-by: Günther Noack <[e-mail]>
>>>>> ---
>>>>>    fs/namei.c                    |  6 +++---
>>>>>    fs/open.c                     |  4 ++--
>>>>>    include/linux/lsm_hook_defs.h |  1 +
>>>>>    include/linux/security.h      |  6 ++++++
>>>>>    security/apparmor/lsm.c       |  6 ++++++
>>>>>    security/security.c           |  5 +++++
>>>>>    security/tomoyo/tomoyo.c      | 13 +++++++++++++
>>>>>    7 files changed, 36 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/namei.c b/fs/namei.c
>>>>> index 53b4bc094db2..52105873d1f8 100644
>>>>> --- a/fs/namei.c
>>>>> +++ b/fs/namei.c
>>>>> @@ -53,8 +53,8 @@
>>>>>     * The new code replaces the old recursive symlink resolution with
>>>>>     * an iterative one (in case of non-nested symlink chains).  It
>>>>> does
>>>>>     * this with calls to <fs>_follow_link().
>>>>> - * As a side effect, dir_namei(), _namei() and follow_link() are now
>>>>> - * replaced with a single function lookup_dentry() that can handle
>>>>> all
>>>>> + * As a side effect, dir_namei(), _namei() and follow_link() are now
>>>>> + * replaced with a single function lookup_dentry() that can handle
>>>>> all
>>>>>     * the special cases of the former code.
>>>>>     *
>>>>>     * With the new dcache, the pathname is stored at each inode, at
>>>>> least as
>>>>> @@ -3211,7 +3211,7 @@ static int handle_truncate(struct user_namespace
>>>>> *mnt_userns, struct file *filp)
>>>>>    	if (error)
>>>>>    		return error;
>>>>>
>>>>> -	error = security_path_truncate(path);
>>>>> +	error = security_file_truncate(filp);
>>>>>    	if (!error) {
>>>>>    		error = do_truncate(mnt_userns, path->dentry, 0,
>>>>>    				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
>>>>> diff --git a/fs/open.c b/fs/open.c
>>>>> index 8a813fa5ca56..0831433e493a 100644
>>>>> --- a/fs/open.c
>>>>> +++ b/fs/open.c
>>>>> @@ -188,7 +188,7 @@ long do_sys_ftruncate(unsigned int fd, loff_t
>>>>> length, int small)
>>>>>    	if (IS_APPEND(file_inode(f.file)))
>>>>>    		goto out_putf;
>>>>>    	sb_start_write(inode->i_sb);
>>>>> -	error = security_path_truncate(&f.file->f_path);
>>>>> +	error = security_file_truncate(f.file);
>>>>>    	if (!error)
>>>>>    		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
>>>>>    				    ATTR_MTIME | ATTR_CTIME, f.file);
>>>>> @@ -1271,7 +1271,7 @@ struct file *filp_open(const char *filename, int
>>>>> flags, umode_t mode)
>>>>>    {
>>>>>    	struct filename *name = getname_kernel(filename);
>>>>>    	struct file *file = ERR_CAST(name);
>>>>> -
>>>>> +
>>>>>    	if (!IS_ERR(name)) {
>>>>>    		file = file_open_name(name, flags, mode);
>>>>>    		putname(name);
>>>>> diff --git a/include/linux/lsm_hook_defs.h
>>>>> b/include/linux/lsm_hook_defs.h
>>>>> index 60fff133c0b1..dee35ab253ba 100644
>>>>> --- a/include/linux/lsm_hook_defs.h
>>>>> +++ b/include/linux/lsm_hook_defs.h
>>>>> @@ -177,6 +177,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct
>>>>> task_struct *tsk,
>>>>>    	 struct fown_struct *fown, int sig)
>>>>>    LSM_HOOK(int, 0, file_receive, struct file *file)
>>>>>    LSM_HOOK(int, 0, file_open, struct file *file)
>>>>> +LSM_HOOK(int, 0, file_truncate, struct file *file)
>>>>>    LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
>>>>>    	 unsigned long clone_flags)
>>>>>    LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
>>>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>>>> index 7bd0c490703d..f80b23382dd9 100644
>>>>> --- a/include/linux/security.h
>>>>> +++ b/include/linux/security.h
>>>>> @@ -394,6 +394,7 @@ int security_file_send_sigiotask(struct task_struct
>>>>> *tsk,
>>>>>    				 struct fown_struct *fown, int sig);
>>>>>    int security_file_receive(struct file *file);
>>>>>    int security_file_open(struct file *file);
>>>>> +int security_file_truncate(struct file *file);
>>>>>    int security_task_alloc(struct task_struct *task, unsigned long
>>>>> clone_flags);
>>>>>    void security_task_free(struct task_struct *task);
>>>>>    int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>>>>> @@ -1011,6 +1012,11 @@ static inline int security_file_open(struct file
>>>>> *file)
>>>>>    	return 0;
>>>>>    }
>>>>>
>>>>> +static inline int security_file_truncate(struct file *file)
>>>>> +{
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>    static inline int security_task_alloc(struct task_struct *task,
>>>>>    				      unsigned long clone_flags)
>>>>>    {
>>>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>>>> index e29cade7b662..98ecb7f221b8 100644
>>>>> --- a/security/apparmor/lsm.c
>>>>> +++ b/security/apparmor/lsm.c
>>>>> @@ -329,6 +329,11 @@ static int apparmor_path_truncate(const struct
>>>>> path *path)
>>>>>    	return common_perm_cond(OP_TRUNC, path, MAY_WRITE |
>>>>> AA_MAY_SETATTR);
>>>>>    }
>>>>>
>>>>> +static int apparmor_file_truncate(struct file *file)
>>>>> +{
>>>>> +	return apparmor_path_truncate(&file->f_path);
>>>>> +}
>>>>> +
>>>>>    static int apparmor_path_symlink(const struct path *dir, struct
>>>>> dentry *dentry,
>>>>>    				 const char *old_name)
>>>>>    {
>>>>> @@ -1232,6 +1237,7 @@ static struct security_hook_list apparmor_hooks[]
>>>>> __lsm_ro_after_init = {
>>>>>    	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
>>>>>    	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
>>>>>    	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
>>>>> +	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
>>>>>
>>>>>    	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
>>>>>    	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
>>>>> diff --git a/security/security.c b/security/security.c
>>>>> index 4b95de24bc8d..e491120c48ba 100644
>>>>> --- a/security/security.c
>>>>> +++ b/security/security.c
>>>>> @@ -1210,6 +1210,11 @@ int security_path_truncate(const struct path
>>>>> *path)
>>>>>    	return call_int_hook(path_truncate, 0, path);
>>>>>    }
>>>>>
>>>>> +int security_file_truncate(struct file *file)
>>>>> +{
>>>>> +	return call_int_hook(file_truncate, 0, file);
>>>>> +}
>>>>> +
>>>>>    int security_path_chmod(const struct path *path, umode_t mode)
>>>>>    {
>>>>>    	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
>>>>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
>>>>> index 71e82d855ebf..af04a7b7eb28 100644
>>>>> --- a/security/tomoyo/tomoyo.c
>>>>> +++ b/security/tomoyo/tomoyo.c
>>>>> @@ -134,6 +134,18 @@ static int tomoyo_path_truncate(const struct path
>>>>> *path)
>>>>>    	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
>>>>>    }
>>>>>
>>>>> +/**
>>>>> + * tomoyo_file_truncate - Target for security_file_truncate().
>>>>> + *
>>>>> + * @file: Pointer to "struct file".
>>>>> + *
>>>>> + * Returns 0 on success, negative value otherwise.
>>>>> + */
>>>>> +static int tomoyo_file_truncate(struct file *file)
>>>>> +{
>>>>> +	return tomoyo_path_truncate(&file->f_path);
>>>>> +}
>>>>> +
>>>>>    /**
>>>>>     * tomoyo_path_unlink - Target for security_path_unlink().
>>>>>     *
>>>>> @@ -545,6 +557,7 @@ static struct security_hook_list tomoyo_hooks[]
>>>>> __lsm_ro_after_init = {
>>>>>    	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
>>>>>    	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
>>>>>    	LSM_HOOK_INIT(file_open, tomoyo_file_open),
>>>>> +	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
>>>>>    	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
>>>>>    	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
>>>>>    	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),
>>>>> --
>>>>> 2.37.3
>>>>>
>>>>
>>>> --
>>
>

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 53b4bc094db2..52105873d1f8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -53,8 +53,8 @@ 
  * The new code replaces the old recursive symlink resolution with
  * an iterative one (in case of non-nested symlink chains).  It does
  * this with calls to <fs>_follow_link().
- * As a side effect, dir_namei(), _namei() and follow_link() are now 
- * replaced with a single function lookup_dentry() that can handle all 
+ * As a side effect, dir_namei(), _namei() and follow_link() are now
+ * replaced with a single function lookup_dentry() that can handle all
  * the special cases of the former code.
  *
  * With the new dcache, the pathname is stored at each inode, at least as
@@ -3211,7 +3211,7 @@  static int handle_truncate(struct user_namespace *mnt_userns, struct file *filp)
 	if (error)
 		return error;
 
-	error = security_path_truncate(path);
+	error = security_file_truncate(filp);
 	if (!error) {
 		error = do_truncate(mnt_userns, path->dentry, 0,
 				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
diff --git a/fs/open.c b/fs/open.c
index 8a813fa5ca56..0831433e493a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -188,7 +188,7 @@  long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
 	if (IS_APPEND(file_inode(f.file)))
 		goto out_putf;
 	sb_start_write(inode->i_sb);
-	error = security_path_truncate(&f.file->f_path);
+	error = security_file_truncate(f.file);
 	if (!error)
 		error = do_truncate(file_mnt_user_ns(f.file), dentry, length,
 				    ATTR_MTIME | ATTR_CTIME, f.file);
@@ -1271,7 +1271,7 @@  struct file *filp_open(const char *filename, int flags, umode_t mode)
 {
 	struct filename *name = getname_kernel(filename);
 	struct file *file = ERR_CAST(name);
-	
+
 	if (!IS_ERR(name)) {
 		file = file_open_name(name, flags, mode);
 		putname(name);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 60fff133c0b1..dee35ab253ba 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -177,6 +177,7 @@  LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
 	 struct fown_struct *fown, int sig)
 LSM_HOOK(int, 0, file_receive, struct file *file)
 LSM_HOOK(int, 0, file_open, struct file *file)
+LSM_HOOK(int, 0, file_truncate, struct file *file)
 LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
 	 unsigned long clone_flags)
 LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
diff --git a/include/linux/security.h b/include/linux/security.h
index 7bd0c490703d..f80b23382dd9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -394,6 +394,7 @@  int security_file_send_sigiotask(struct task_struct *tsk,
 				 struct fown_struct *fown, int sig);
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file);
+int security_file_truncate(struct file *file);
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
 void security_task_free(struct task_struct *task);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
@@ -1011,6 +1012,11 @@  static inline int security_file_open(struct file *file)
 	return 0;
 }
 
+static inline int security_file_truncate(struct file *file)
+{
+	return 0;
+}
+
 static inline int security_task_alloc(struct task_struct *task,
 				      unsigned long clone_flags)
 {
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index e29cade7b662..98ecb7f221b8 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -329,6 +329,11 @@  static int apparmor_path_truncate(const struct path *path)
 	return common_perm_cond(OP_TRUNC, path, MAY_WRITE | AA_MAY_SETATTR);
 }
 
+static int apparmor_file_truncate(struct file *file)
+{
+	return apparmor_path_truncate(&file->f_path);
+}
+
 static int apparmor_path_symlink(const struct path *dir, struct dentry *dentry,
 				 const char *old_name)
 {
@@ -1232,6 +1237,7 @@  static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(mmap_file, apparmor_mmap_file),
 	LSM_HOOK_INIT(file_mprotect, apparmor_file_mprotect),
 	LSM_HOOK_INIT(file_lock, apparmor_file_lock),
+	LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
 
 	LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
 	LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
diff --git a/security/security.c b/security/security.c
index 4b95de24bc8d..e491120c48ba 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1210,6 +1210,11 @@  int security_path_truncate(const struct path *path)
 	return call_int_hook(path_truncate, 0, path);
 }
 
+int security_file_truncate(struct file *file)
+{
+	return call_int_hook(file_truncate, 0, file);
+}
+
 int security_path_chmod(const struct path *path, umode_t mode)
 {
 	if (unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 71e82d855ebf..af04a7b7eb28 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -134,6 +134,18 @@  static int tomoyo_path_truncate(const struct path *path)
 	return tomoyo_path_perm(TOMOYO_TYPE_TRUNCATE, path, NULL);
 }
 
+/**
+ * tomoyo_file_truncate - Target for security_file_truncate().
+ *
+ * @file: Pointer to "struct file".
+ *
+ * Returns 0 on success, negative value otherwise.
+ */
+static int tomoyo_file_truncate(struct file *file)
+{
+	return tomoyo_path_truncate(&file->f_path);
+}
+
 /**
  * tomoyo_path_unlink - Target for security_path_unlink().
  *
@@ -545,6 +557,7 @@  static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security),
 	LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl),
 	LSM_HOOK_INIT(file_open, tomoyo_file_open),
+	LSM_HOOK_INIT(file_truncate, tomoyo_file_truncate),
 	LSM_HOOK_INIT(path_truncate, tomoyo_path_truncate),
 	LSM_HOOK_INIT(path_unlink, tomoyo_path_unlink),
 	LSM_HOOK_INIT(path_mkdir, tomoyo_path_mkdir),