Re: [PATCH v4 2/5] ima: use fs method to read integrity data [updated]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2017-08-07 at 12:04 +0200, Jan Kara wrote:
> 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@xxxxxx>
> > > > > > 
> > > > > > 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@xxxxxx>
> > > > > > Cc: Matthew Garrett <matthew.garrett@xxxxxxxxxx>
> > > > > > Cc: Jan Kara <jack@xxxxxxxx>
> > > > > > Cc: "Theodore Ts'o" <tytso@xxxxxxx>
> > > > > > Cc: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> > > > > > Cc: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > > > > > Cc: Chao Yu <yuchao0@xxxxxxxxxx>
> > > > > > Cc: Steven Whitehouse <swhiteho@xxxxxxxxxx>
> > > > > > Cc: Bob Peterson <rpeterso@xxxxxxxxxx>
> > > > > > Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> > > > > > Cc: Dave Kleikamp <shaggy@xxxxxxxxxx>
> > > > > > Cc: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxxxxxx>
> > > > > > Cc: Mark Fasheh <mfasheh@xxxxxxxxxxx>
> > > > > > Cc: Joel Becker <jlbec@xxxxxxxxxxxx>
> > > > > > Cc: Richard Weinberger <richard@xxxxxx>
> > > > > > Cc: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
> > > > > > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > > > > > Cc: Chris Mason <clm@xxxxxx>
> > > > > > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> > > > > 
> > > > > ...
> > > > > 
> > > > > > +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.

Great!

> >  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.

Agreed.

> > 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.

Ok.  So I shouldn't revert the original commit, which fails the
O_DIRECT open for either the buffered read or DAX.  I'll just move the
code to a bit later, so that the failure is added to the measurement
list.

The original commit returned -EACCES.  On xfs, the open for direct IO
buffer read fails with -EINVAL.  Do you have a preference IMA should
return?

thanks!

Mimi




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux