Re: [RFC 2/2] ima: Fix detection of read/write violations on stacked filesystems

[Stefan Berger]

On 4/12/24 14:08, Amir Goldstein wrote:
> On Fri, Apr 12, 2024 at 5:01 PM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
> > On a stacked filesystem, when one process opens the file holding a file's
> > data (e.g., on upper or lower layer on overlayfs) then issue a violation
> > when another process opens the file for reading on the top layer (overlay
> > layer on overlayfs). This then provides similar behavior to the existing
> > case where a violation is generated when one process opens a file for
> > writing and another one opens the same file for reading. On stacked
> > filesystem also search all the lower layers for relevant files opened for
> > writing and issue the violation if one is found.
> >
> > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > ---
> >   security/integrity/ima/ima_main.c | 27 ++++++++++++++++++++++-----
> >   1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index f04f43af651c..590dd9d5d99a 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -121,8 +121,11 @@ static void ima_rdwr_violation_check(struct file *file,
> >                                       const char **pathname,
> >                                       char *filename)
> >   {
> > +       struct inode *real_inode = d_real_inode(file_dentry(file));
> >          struct inode *inode = file_inode(file);
> > +       struct dentry *fd_dentry, *d;
> >          fmode_t mode = file->f_mode;
> > +       struct inode *fd_inode;
> >          bool send_tomtou = false, send_writers = false;
> >
> >          if (mode & FMODE_WRITE) {
> > @@ -134,11 +137,25 @@ static void ima_rdwr_violation_check(struct file *file,
> >                                                  &iint->atomic_flags))
> >                                  send_tomtou = true;
> >                  }
> > -       } else {
> > -               if (must_measure)
> > -                       set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
> > -               if (inode_is_open_for_write(inode) && must_measure)
> > -                       send_writers = true;
> > +       } else if (must_measure) {
> > +               set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
> > +
> > +               if (inode == real_inode) {
> > +                       if (inode_is_open_for_write(inode))
> > +                               send_writers = true;
> > +               } else {
> > +                       d = d_real(file_dentry(file), D_REAL_FILEDATA);
> > +                       do {
> > +                               fd_dentry = d;
> > +                               fd_inode = d_inode(fd_dentry);
> > +                               if (inode_is_open_for_write(fd_inode)) {
> > +                                       send_writers = true;
> > +                                       break;
> > +                               }
> > +                               /* next layer of stacked fs */
> > +                               d = d_real(fd_dentry, D_REAL_FILEDATA);
> > +                       } while (d != fd_dentry);
> > +               }
>
> The idea of digging though ovl layers feels wrong to me.

I have a couple of test cases that expect violations to be logged. One 
test case has 2 overlay filesystems stacked on top of each other (lower 
= A, upper = B) and it passes those test cases when for example

- opening the file on lower on 'A' for writing
- opening the file on overlay layer on 'B' for reading

OR

- opening the file on overlay layer on 'A' (= lower layer of 'B') for 
writing
- opening the file on overlay layer on 'B' for reading



After causing a copy-up only the following test case causes a violation 
to be logged:

- opening the file on upper on 'B' for writing
- opening the file on overlay layer on 'B' for reading

No violation will the be logged for example for:

- opening the file on overlay layer on 'A' (= lower of 'B') for writing
- opening the file on overlay layer on 'B' for reading



> As Miklos is the designer of overlayfs and its vfs architecture,

I was hoping that this would be sufficiently generic to work with 
potential future stacked filesystems as well that would need to also 
provide support for D_REAL_FILEDATA.

> I am deferring the call about adding this interface to Miklos.
>
> Thanks,
> Amir.