From mboxrd@z Thu Jan 1 00:00:00 1970 From: zohar@linux.vnet.ibm.com (Mimi Zohar) Date: Mon, 10 Jul 2017 11:12:41 -0400 Subject: [Linux-ima-devel] [PATCH 3/4] ima: use existing read file operation method to calculate file hash In-Reply-To: References: <1497031364-19949-1-git-send-email-zohar@linux.vnet.ibm.com> <1497031364-19949-4-git-send-email-zohar@linux.vnet.ibm.com> Message-ID: <1499699561.6034.129.camel@linux.vnet.ibm.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Mon, 2017-07-10 at 17:03 +0300, Dmitry Kasatkin wrote: > On Fri, Jun 9, 2017 at 9:02 PM, Mimi Zohar 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 > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:35640 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754285AbdGJPPT (ORCPT ); Mon, 10 Jul 2017 11:15:19 -0400 Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v6AFEgFW048555 for ; Mon, 10 Jul 2017 11:15:19 -0400 Received: from e23smtp06.au.ibm.com (e23smtp06.au.ibm.com [202.81.31.148]) by mx0a-001b2d01.pphosted.com with ESMTP id 2bma0ne0yj-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 10 Jul 2017 11:15:18 -0400 Received: from localhost by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 11 Jul 2017 01:15:16 +1000 Subject: Re: [Linux-ima-devel] [PATCH 3/4] ima: use existing read file operation method to calculate file hash From: Mimi Zohar To: Dmitry Kasatkin Cc: Christoph Hellwig , Al Viro , linux-fsdevel@vger.kernel.org, linux-ima-devel , linux-security-module Date: Mon, 10 Jul 2017 11:12:41 -0400 In-Reply-To: References: <1497031364-19949-1-git-send-email-zohar@linux.vnet.ibm.com> <1497031364-19949-4-git-send-email-zohar@linux.vnet.ibm.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Message-Id: <1499699561.6034.129.camel@linux.vnet.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, 2017-07-10 at 17:03 +0300, Dmitry Kasatkin wrote: > On Fri, Jun 9, 2017 at 9:02 PM, Mimi Zohar 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 > > --- > > 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