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

["Günther Noack" <gnoack3000@gmail.com>]

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 <gnoack3000@gmail.com> 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@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(-)
>
> 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

--