From: Jan Kara <jack@suse.cz> To: Mimi Zohar <zohar@linux.vnet.ibm.com> Cc: Jan Kara <jack@suse.cz>, Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>, James Morris <jmorris@namei.org>, linux-fsdevel@vger.kernel.org, linux-ima-devel@lists.sourceforge.net, linux-security-module@vger.kernel.org, Matthew Garrett <matthew.garrett@nebula.com>, Jan Kara <jack@suse.com>, Theodore Ts'o <tytso@mit.edu>, Andreas Dilger <adilger.kernel@dilger.ca>, Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <yuchao0@huawei.com>, Steven Whitehouse <swhiteho@redhat.com>, Bob Peterson <rpeterso@redhat.com>, David Woodhouse <dwmw2@infradead.org>, Dave Kleikamp <shaggy@kernel.org>, Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>, Mark Fasheh <mfasheh@versity.com>, Joel Becker <jlbec@evilplan.org>, Richard Weinberger <richard@nod.at>, "Darrick J. Wong" <darrick.wong@oracle.com>, Hugh Dickins <hughd@google.com>, Chris Mason <clm@fb.com> Subject: Re: [PATCH v4 2/5] ima: use fs method to read integrity data [updated] Date: Mon, 7 Aug 2017 12:04:45 +0200 [thread overview] Message-ID: <20170807100445.GB6470@quack2.suse.cz> (raw) In-Reply-To: <1501880831.27872.153.camel@linux.vnet.ibm.com> On Fri 04-08-17 17:07:11, Mimi Zohar wrote: > On Thu, 2017-08-03 at 12:56 +0200, Jan Kara wrote: > > On Wed 02-08-17 13:11:52, Mimi Zohar wrote: > > > On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote: > > > > On Tue 01-08-17 16:24:30, Mimi Zohar wrote: > > > > > From: Christoph Hellwig <hch@lst.de> > > > > > > > > > > Add a new ->integrity_read file operation to read data for integrity > > > > > hash collection. This is defined to be equivalent to ->read_iter, > > > > > except that it will be called with the i_rwsem held exclusively. > > > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > Cc: Matthew Garrett <matthew.garrett@nebula.com> > > > > > Cc: Jan Kara <jack@suse.com> > > > > > Cc: "Theodore Ts'o" <tytso@mit.edu> > > > > > Cc: Andreas Dilger <adilger.kernel@dilger.ca> > > > > > Cc: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > Cc: Chao Yu <yuchao0@huawei.com> > > > > > Cc: Steven Whitehouse <swhiteho@redhat.com> > > > > > Cc: Bob Peterson <rpeterso@redhat.com> > > > > > Cc: David Woodhouse <dwmw2@infradead.org> > > > > > Cc: Dave Kleikamp <shaggy@kernel.org> > > > > > Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp> > > > > > Cc: Mark Fasheh <mfasheh@versity.com> > > > > > Cc: Joel Becker <jlbec@evilplan.org> > > > > > Cc: Richard Weinberger <richard@nod.at> > > > > > Cc: "Darrick J. Wong" <darrick.wong@oracle.com> > > > > > Cc: Hugh Dickins <hughd@google.com> > > > > > Cc: Chris Mason <clm@fb.com> > > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > > > > > > ... > > > > > > > > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb, > > > > > + struct iov_iter *to) > > > > > +{ > > > > > + struct inode *inode = file_inode(iocb->ki_filp); > > > > > + int o_direct = iocb->ki_flags & IOCB_DIRECT; > > > > > + > > > > > + lockdep_assert_held(&inode->i_rwsem); > > > > > + if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > > > > + return -EIO; > > > > > + > > > > > + if (!iov_iter_count(to)) > > > > > + return 0; /* skip atime */ > > > > > + > > > > > +#ifdef CONFIG_FS_DAX > > > > > + if (IS_DAX(inode)) > > > > > + return dax_iomap_rw(iocb, to, &ext4_iomap_ops); > > > > > +#endif > > > > > + if (o_direct) > > > > > + return -EINVAL; > > > > > + return generic_file_read_iter(iocb, to); > > > > > +} > > > > > > > > I have noticed this o_direct check - why is it only in ext4 and shouldn't > > > > rather higher layers make sure IOCB_DIRECT iocbs cannot reach > > > > .integrity_read() methods? > > > > > > This failure happens when opening a file with O_DIRECT on a block > > > device that does not support dax (eg. loop). xfs makes it to here too, > > > but the call to generic_file_read_iter() fails properly with -EINVAL.� > > > (Only tested on those filesystems included that support dax (eg. ext2, > > > ext4, and xfs).) > > > > Well, yes, ext2 and ext4 will silently fall back to buffered read (as do > > pretty much all filesystems except for XFS). However I fail to see why IMA > > should care (which is probably due to my lack of knowledge about IMA). > > Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX? > > Thank you for the explanation! �(I was confused about the relationship > between O_DIRECT and DAX.) �You're correct. �IMA does not support > O_DIRECT in the buffered case for two reasons, locking and updating > the file hash, which are described in commit�f9b2a735bddd "ima: audit > log files opened with O_DIRECT flag". �After reverting this commit, > the O_DIRECT check is needed before calling generic_file_read_iter(). Thanks for the pointer. This cleaned up the issue for me. > �Most likely the same would need to be done for other filesystems that > support O_DIRECT. �Probably a generic_integrity_file_read_iter() > should be defined. Yeah, then please define some common helper that takes care of refusing direct IO - IMO you should check this even before calling into ->integrity_read helper. > For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading > the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the > lock has been taken. �Assuming the file system is mounted with > i_version, the file hash is updated properly. Yes, for DAX direct IO is basically no different but frankly I would just refuse O_DIRECT on DAX inodes as well just for the consistency sake. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
WARNING: multiple messages have this Message-ID (diff)
From: jack@suse.cz (Jan Kara) To: linux-security-module@vger.kernel.org Subject: [PATCH v4 2/5] ima: use fs method to read integrity data [updated] Date: Mon, 7 Aug 2017 12:04:45 +0200 [thread overview] Message-ID: <20170807100445.GB6470@quack2.suse.cz> (raw) In-Reply-To: <1501880831.27872.153.camel@linux.vnet.ibm.com> On Fri 04-08-17 17:07:11, Mimi Zohar wrote: > On Thu, 2017-08-03 at 12:56 +0200, Jan Kara wrote: > > On Wed 02-08-17 13:11:52, Mimi Zohar wrote: > > > On Wed, 2017-08-02 at 10:01 +0200, Jan Kara wrote: > > > > On Tue 01-08-17 16:24:30, Mimi Zohar wrote: > > > > > From: Christoph Hellwig <hch@lst.de> > > > > > > > > > > Add a new ->integrity_read file operation to read data for integrity > > > > > hash collection. This is defined to be equivalent to ->read_iter, > > > > > except that it will be called with the i_rwsem held exclusively. > > > > > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > Cc: Matthew Garrett <matthew.garrett@nebula.com> > > > > > Cc: Jan Kara <jack@suse.com> > > > > > Cc: "Theodore Ts'o" <tytso@mit.edu> > > > > > Cc: Andreas Dilger <adilger.kernel@dilger.ca> > > > > > Cc: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > Cc: Chao Yu <yuchao0@huawei.com> > > > > > Cc: Steven Whitehouse <swhiteho@redhat.com> > > > > > Cc: Bob Peterson <rpeterso@redhat.com> > > > > > Cc: David Woodhouse <dwmw2@infradead.org> > > > > > Cc: Dave Kleikamp <shaggy@kernel.org> > > > > > Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp> > > > > > Cc: Mark Fasheh <mfasheh@versity.com> > > > > > Cc: Joel Becker <jlbec@evilplan.org> > > > > > Cc: Richard Weinberger <richard@nod.at> > > > > > Cc: "Darrick J. Wong" <darrick.wong@oracle.com> > > > > > Cc: Hugh Dickins <hughd@google.com> > > > > > Cc: Chris Mason <clm@fb.com> > > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > > > > > > ... > > > > > > > > > +static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb, > > > > > + struct iov_iter *to) > > > > > +{ > > > > > + struct inode *inode = file_inode(iocb->ki_filp); > > > > > + int o_direct = iocb->ki_flags & IOCB_DIRECT; > > > > > + > > > > > + lockdep_assert_held(&inode->i_rwsem); > > > > > + if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) > > > > > + return -EIO; > > > > > + > > > > > + if (!iov_iter_count(to)) > > > > > + return 0; /* skip atime */ > > > > > + > > > > > +#ifdef CONFIG_FS_DAX > > > > > + if (IS_DAX(inode)) > > > > > + return dax_iomap_rw(iocb, to, &ext4_iomap_ops); > > > > > +#endif > > > > > + if (o_direct) > > > > > + return -EINVAL; > > > > > + return generic_file_read_iter(iocb, to); > > > > > +} > > > > > > > > I have noticed this o_direct check - why is it only in ext4 and shouldn't > > > > rather higher layers make sure IOCB_DIRECT iocbs cannot reach > > > > .integrity_read() methods? > > > > > > This failure happens when opening a file with O_DIRECT on a block > > > device that does not support dax (eg. loop). xfs makes it to here too, > > > but the call to generic_file_read_iter() fails properly with -EINVAL.? > > > (Only tested on those filesystems included that support dax (eg. ext2, > > > ext4, and xfs).) > > > > Well, yes, ext2 and ext4 will silently fall back to buffered read (as do > > pretty much all filesystems except for XFS). However I fail to see why IMA > > should care (which is probably due to my lack of knowledge about IMA). > > Is O_DIRECT somehow excepted from IMA? But then why it is not for DAX? > > Thank you for the explanation! ?(I was confused about the relationship > between O_DIRECT and DAX.) ?You're correct. ?IMA does not support > O_DIRECT in the buffered case for two reasons, locking and updating > the file hash, which are described in commit?f9b2a735bddd "ima: audit > log files opened with O_DIRECT flag". ?After reverting this commit, > the O_DIRECT check is needed before calling generic_file_read_iter(). Thanks for the pointer. This cleaned up the issue for me. > ?Most likely the same would need to be done for other filesystems that > support O_DIRECT. ?Probably a generic_integrity_file_read_iter() > should be defined. Yeah, then please define some common helper that takes care of refusing direct IO - IMO you should check this even before calling into ->integrity_read helper. > For DAX, unlike do_blockdev_direct_IO() which takes the lock, reading > the file with O_DIRECT is fine, as dax_iomap_rw() only checks that the > lock has been taken. ?Assuming the file system is mounted with > i_version, the file hash is updated properly. Yes, for DAX direct IO is basically no different but frankly I would just refuse O_DIRECT on DAX inodes as well just for the consistency sake. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR -- 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
next prev parent reply other threads:[~2017-08-07 10:04 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-07-26 13:22 [PATCH v4 0/5] define new fs integrity_read method Mimi Zohar 2017-07-26 13:22 ` Mimi Zohar 2017-07-26 13:22 ` [PATCH v4 1/5] ima: always measure and audit files in policy Mimi Zohar 2017-07-26 13:22 ` Mimi Zohar 2017-08-22 9:24 ` [Linux-ima-devel] " Dmitry Kasatkin 2017-08-22 9:24 ` Dmitry Kasatkin 2017-07-26 13:22 ` [PATCH v4 2/5] ima: use fs method to read integrity data Mimi Zohar 2017-07-26 13:22 ` Mimi Zohar 2017-07-31 7:01 ` Jan Kara 2017-07-31 7:01 ` Jan Kara 2017-07-31 19:08 ` Mimi Zohar 2017-07-31 19:08 ` Mimi Zohar 2017-08-01 10:42 ` Jan Kara 2017-08-01 10:42 ` Jan Kara 2017-08-01 15:38 ` Mimi Zohar 2017-08-01 15:38 ` Mimi Zohar 2017-08-01 20:24 ` [PATCH v4 2/5] ima: use fs method to read integrity data [updated] Mimi Zohar 2017-08-01 20:24 ` Mimi Zohar 2017-08-02 8:01 ` Jan Kara 2017-08-02 8:01 ` Jan Kara 2017-08-02 17:11 ` Mimi Zohar 2017-08-02 17:11 ` Mimi Zohar 2017-08-03 10:56 ` Jan Kara 2017-08-03 10:56 ` Jan Kara 2017-08-04 21:07 ` Mimi Zohar 2017-08-04 21:07 ` Mimi Zohar 2017-08-07 10:04 ` Jan Kara [this message] 2017-08-07 10:04 ` Jan Kara 2017-08-07 20:12 ` Mimi Zohar 2017-08-07 20:12 ` Mimi Zohar 2017-08-08 11:17 ` Jan Kara 2017-08-08 11:17 ` Jan Kara 2017-08-22 9:59 ` [PATCH v4 2/5] ima: use fs method to read integrity data Dmitry Kasatkin 2017-08-22 9:59 ` Dmitry Kasatkin 2017-07-26 13:22 ` [PATCH v4 3/5] ima: define "dont_failsafe" policy action rule Mimi Zohar 2017-07-26 13:22 ` Mimi Zohar 2017-08-22 9:34 ` [Linux-ima-devel] " Dmitry Kasatkin 2017-08-22 9:34 ` Dmitry Kasatkin 2017-08-22 9:39 ` Dmitry Kasatkin 2017-08-22 9:39 ` Dmitry Kasatkin 2017-07-26 13:22 ` [PATCH v4 4/5] ima: define "fs_unsafe" builtin policy Mimi Zohar 2017-07-26 13:22 ` Mimi Zohar 2017-08-22 9:36 ` [Linux-ima-devel] " Dmitry Kasatkin 2017-08-22 9:36 ` Dmitry Kasatkin 2017-07-26 13:22 ` [PATCH v4 5/5] ima: remove permit_directio policy option Mimi Zohar 2017-07-26 13:22 ` Mimi Zohar 2017-08-22 9:27 ` [Linux-ima-devel] " Dmitry Kasatkin 2017-08-22 9:27 ` Dmitry Kasatkin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170807100445.GB6470@quack2.suse.cz \ --to=jack@suse.cz \ --cc=adilger.kernel@dilger.ca \ --cc=clm@fb.com \ --cc=darrick.wong@oracle.com \ --cc=dwmw2@infradead.org \ --cc=hch@lst.de \ --cc=hughd@google.com \ --cc=jack@suse.com \ --cc=jaegeuk@kernel.org \ --cc=jlbec@evilplan.org \ --cc=jmorris@namei.org \ --cc=konishi.ryusuke@lab.ntt.co.jp \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-ima-devel@lists.sourceforge.net \ --cc=linux-security-module@vger.kernel.org \ --cc=matthew.garrett@nebula.com \ --cc=mfasheh@versity.com \ --cc=richard@nod.at \ --cc=rpeterso@redhat.com \ --cc=shaggy@kernel.org \ --cc=swhiteho@redhat.com \ --cc=tytso@mit.edu \ --cc=viro@zeniv.linux.org.uk \ --cc=yuchao0@huawei.com \ --cc=zohar@linux.vnet.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.