On Mon, 2017-07-10 at 17:03 +0300, Dmitry Kasatkin wrote:
> On Fri, Jun 9, 2017 at 9:02 PM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> > The few filesystems that currently define the read file operation
> > method, either call seq_read() or have a filesystem specific read
> > method. None of them, at least in the fs directory, take the
> > i_rwsem.
> >
> > Since some files on these filesystems were previously
> > measured/appraised, not measuring them would change the PCR hash
> > value(s), possibly breaking userspace. For filesystems that do
> > not define the integrity_read file operation method and have a
> > read method, use the read method to calculate the file hash.
> >
> > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> > ---
> > security/integrity/iint.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index c5489672b5aa..e3ef3fba16dc 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -189,20 +189,29 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> > struct iovec iov = { .iov_base = addr, .iov_len = count };
> > struct kiocb kiocb;
> > struct iov_iter iter;
> > - ssize_t ret;
> > + ssize_t ret = -EBADF;
> >
> > lockdep_assert_held(&inode->i_rwsem);
> >
> > if (!(file->f_mode & FMODE_READ))
> > return -EBADF;
> > - if (!file->f_op->integrity_read)
> > - return -EBADF;
> >
> > init_sync_kiocb(&kiocb, file);
> > kiocb.ki_pos = offset;
> > iov_iter_init(&iter, READ | ITER_KVEC, &iov, 1, count);
> >
> > - ret = file->f_op->integrity_read(&kiocb, &iter);
> > + if (file->f_op->integrity_read) {
> > + ret = file->f_op->integrity_read(&kiocb, &iter);
> > + } else if (file->f_op->read) {
> > + mm_segment_t old_fs;
> > + char __user *buf = (char __user *)addr;
> > +
> > + old_fs = get_fs();
> > + set_fs(get_ds());
> > + ret = file->f_op->read(file, buf, count, &offset);
> > + set_fs(old_fs);
> > + }
>
> Hi,
>
> Original code was using "__vfs_read()".
> Patch 1 replaced use of it by ->integrity_read.
> This patch re-added f_op->read() directly...
>
> While __vfs_read() did it differently
>
> if (file->f_op->read)
> return file->f_op->read(file, buf, count, pos);
> else if (file->f_op->read_iter)
> return new_sync_read(file, buf, count, pos);
>
>
> Is avoiding use of __vfs_read() is intentional?
Yes, some filesystems ->read_iter attempt to take the i_rwsem, which
IMA has already taken. This patch set introduces a new file operation
method ->integrity_read, which reads the file without re-taking the
lock. (This method should probably be renamed ->integrity_read_iter.)
The next version of this patch set will remove this patch, unless
someone provides a reason for needing it. At that point, a new method
equivalent to ->read would need to be defined.
Mimi
