| Message ID | 1497031364-19949-4-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
A strong and a weak NAK on this. For one thing you should not call ->read for fs code at all - use read_iter where it fits (it does here) or the kernel_read() helper otherwise. But once again I don't think this is correct - it's a potentially unsafe default, so please wire up the file systems actually tested and known to work manually. E.g. this does the wrong thing for at least NFS and OCFS2.
On Tue, 2017-06-13 at 08:46 +0200, Christoph Hellwig wrote: > A strong and a weak NAK on this. For one thing you should not > call ->read for fs code at all - use read_iter where it fits > (it does here) or the kernel_read() helper otherwise. Calling ->read directly is intentional. Commit C0430e49b6e7c "ima: introduce ima_kernel_read()" replaced the call to kernel_read with ima_kernel_read(), the non-security checking version of kernel_read(). Subsequently, commit e3c4abbfa97e "integrity: define a new function integrity_read_file()" renamed ima_read_file() to integrity_read_file(). > But once again I don't think this is correct - it's a potentially > unsafe default, so please wire up the file systems actually tested > and known to work manually. > > E.g. this does the wrong thing for at least NFS and OCFS2. Both NFS and OCFS define their own specific read_iter(), nfs_file_read() and ocfs2_file_read_iter() respectively. As these file systems have not yet been converted to use ->read_integrity, the xfstests fail. Mimi
On Tue, Jun 13, 2017 at 10:17:45AM -0400, Mimi Zohar wrote: > Calling ->read directly is intentional. Commit C0430e49b6e7c "ima: > introduce ima_kernel_read()" replaced the call to kernel_read with > ima_kernel_read(), the non-security checking version of kernel_read(). > Subsequently, commit e3c4abbfa97e "integrity: define a new function > integrity_read_file()" renamed ima_read_file() to > integrity_read_file(). Again, the point is you should not call ->read for in-kernel reads. > Both NFS and OCFS define their own specific read_iter(), > nfs_file_read() and ocfs2_file_read_iter() respectively. As these > file systems have not yet been converted to use ->read_integrity, the > xfstests fail. So they will need to be converted. The xfstests will not just fail, it will deadlock the calling process with this code.
On Tue, 2017-06-13 at 16:22 +0200, Christoph Hellwig wrote: > On Tue, Jun 13, 2017 at 10:17:45AM -0400, Mimi Zohar wrote: > > Calling ->read directly is intentional. Commit C0430e49b6e7c "ima: > > introduce ima_kernel_read()" replaced the call to kernel_read with > > ima_kernel_read(), the non-security checking version of kernel_read(). > > Subsequently, commit e3c4abbfa97e "integrity: define a new function > > integrity_read_file()" renamed ima_read_file() to > > integrity_read_file(). > > Again, the point is you should not call ->read for in-kernel reads. Ok, there was a reason for restoring this behavior. Perhaps, something that was previously being measured wasn't being measured without it. Looking ... > > Both NFS and OCFS define their own specific read_iter(), > > nfs_file_read() and ocfs2_file_read_iter() respectively. As these > > file systems have not yet been converted to use ->read_integrity, the > > xfstests fail. > > So they will need to be converted. The xfstests will not just fail, > it will deadlock the calling process with this code. Right, process_measurement() is fail safe, meaning that any file, which matches a rule in the IMA policy, that isn't appraised properly fails. from ima_main: process_measurement( out: inode_unlock(inode); if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) return -EACCES; return 0; With your patch, we now need to take into consideration that the file system doesn't support IMA-appraisal. We can't just change the existing behavior, so we will probably need the ability to override the fail safe behavior for file systems that do not support IMA. The bigger problem is that files that were previously measured, might now not be measured, without any indication in the audit logs or the IMA measurement list. Mimi
On Tue, Jun 13, 2017 at 11:07:29AM -0400, Mimi Zohar wrote: > The bigger problem is that files that were previously measured, might > now not be measured, without any indication in the audit logs or the > IMA measurement list. And that's exactly what I've been preaching for a long time - you need to decide on what your requirements for IMA are and check for them when enabling it, not just have things sort of work or not at runtime.
On Fri, Jun 9, 2017 at 9:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> > --- > 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? Dmitry > + > BUG_ON(ret == -EIOCBQUEUED); > return ret; > } > -- > 2.7.4 > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Linux-ima-devel mailing list > Linux-ima-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
On Mon, 2017-07-10 at 17:03 +0300, Dmitry Kasatkin wrote: > On Fri, Jun 9, 2017 at 9:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> > > --- > > 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
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); + } + BUG_ON(ret == -EIOCBQUEUED); return ret; }
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@linux.vnet.ibm.com> --- security/integrity/iint.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)