| Message ID | 1498069110-10009-11-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
NAK - we'll need an explicit method for the integrity code. And just curious - what filesystem that you care about actually implements ->read instead of ->read_iter? We shouldn't be doing that for real file systems anymore.
[Cc'ing linux-ima-users] On Wed, 2017-06-28 at 16:41 +0200, Christoph Hellwig wrote: > NAK - we'll need an explicit method for the integrity code. > > And just curious - what filesystem that you care about actually > implements ->read instead of ->read_iter? We shouldn't be doing that > for real file systems anymore. Right, pseudo filesystems are using ->read. The existing builtin measurement policies exclude a number of pseudo filesystems, but not efivarfs. Unfortunately, we do not know what type of custom policies are currently being used. The contents of the IMA measurement list are verified against a reference manifest, provided at registration, or against a white list. Not measuring files that were previously measured could break userspace applications. Let's wait to hear back from the larger IMA community as to whether there is a need to measure files on pseudo filesystems, before implementing an explicit method. Mimi
On Wed, Jul 05, 2017 at 10:50:09AM -0400, Mimi Zohar wrote: > [Cc'ing linux-ima-users] > > On Wed, 2017-06-28 at 16:41 +0200, Christoph Hellwig wrote: > > NAK - we'll need an explicit method for the integrity code. > > > > And just curious - what filesystem that you care about actually > > implements ->read instead of ->read_iter? We shouldn't be doing that > > for real file systems anymore. > > Right, pseudo filesystems are using ->read. The existing builtin > measurement policies exclude a number of pseudo filesystems, but not > efivarfs. Unfortunately, we do not know what type of custom policies > are currently being used. efi variables contain information that may influence userspace behaviour and can also be modified out of band, so I think there's a reasonable argument that they should be measured.
On Wed, Jul 05, 2017 at 06:02:15PM +0100, Matthew Garrett wrote: > On Wed, Jul 05, 2017 at 10:50:09AM -0400, Mimi Zohar wrote: > > [Cc'ing linux-ima-users] > > > > On Wed, 2017-06-28 at 16:41 +0200, Christoph Hellwig wrote: > > > NAK - we'll need an explicit method for the integrity code. > > > > > > And just curious - what filesystem that you care about actually > > > implements ->read instead of ->read_iter? We shouldn't be doing that > > > for real file systems anymore. > > > > Right, pseudo filesystems are using ->read. The existing builtin > > measurement policies exclude a number of pseudo filesystems, but not > > efivarfs. Unfortunately, we do not know what type of custom policies > > are currently being used. > > efi variables contain information that may influence userspace behaviour > and can also be modified out of band, so I think there's a reasonable > argument that they should be measured. Then efivars should grow a ->integrity_read method.
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index df04f35a1d40..75c3cef5fd01 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 kvec 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_kvec(&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 builtin "ima_tcb" policy measures all files read by root. This policy includes, for example, files on efivars. Since some files on these filesystems were previously measured (eg. OsIndicationsSupported), not measuring them would change the PCR hash value(s), potentially breaking userspace. 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. For filesystems that do not define the ->integrity_read file operation method and have a ->read method, this patch calls 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(-)