[PATCH v6 1/5] security: create file_truncate hook from path_truncate hook
Günther Noack
gnoack3000 at gmail.com
Mon Sep 26 16:07:48 UTC 2022
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 <gnoack3000 at gmail.com>
> > > ---
> > > 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
> > >
> >
> > --
--
More information about the Linux-security-module-archive
mailing list