On Thu, 2018-10-04 at 21:57 -0500, Goldwyn Rodrigues wrote:
> On 11:52 04/10, Mimi Zohar wrote:
> > On Thu, 2018-10-04 at 00:35 +0200, Miklos Szeredi wrote:
> >
> > > Right, if it's done from last fput() then it's at least not a security hole.
> > >
> > > This hack may work for some filesystems, but as you noticed, it won't
> > > work for overlayfs. And if probably won't work for a number of other
> > > filesystems as well: the fs can assume that f_mode & FMODE_READ will
> > > remain off if it was off at open time.
> > >
> > > The proper way to handle it generally is to open a separate instance
> > > of the same file from IMA with O_RDONLY and use that to calculate the
> > > hash. There's really no point in reusing the same file and hacking
> > > the f_mode bits.
> >
> > Is there an appropriate low level kernel call for creating a new file
> > descriptor that would be appropriate? dentry_open(), like the call in
> > file_clone_open(), has a lot of overhead, including the LSM calls.
> > Calculating the file hash always needs to work.
> >
>
> Mimi,
>
> I have formulated a patch which is working for me on overlayfs. I am
> using dentry_open(), which I agree may have overhead. While this
> opens up the possibility of using it for files opened with O_DIRECT
> which may end up getting the file into pagecache.
>
> Subject: [PATCH] Open new file instance O_RDONLY to calculate hash
>
> Using the open struct file to calculate the hash does not work
> with overlayfs, because the real struct file is hidden behind the
> overlays struct file. So, any file->f_mode manipulations are not
> reflected on the real struct file. So, open the file again, read and
> calculate the hash.
>
> As a byproduct, we can remove all instance of f_mode manipulations
> and can work with O_DIRECT as well.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
By "overhead", I didn't mean it from a performance perspective, but
was concerned about the dentry_open() failing. If "dentry_open" fails
for any reason, the file hash will not be re-calculated, causing
future file opens to fail. Missing is the test for dentry_open()
failure.
By removing the "read" check, re-opening the file is now for all
files, not just those opened without "read". From a testing
perspective, it will catch problems faster, but ...
I've had a patch queued that removes the O_DIRECT test, but haven't
had a chance to test it on ALL filesystems. I would probably re-open
the file with the original flags, plus read, as much as possible.
Mimi
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 7e7e7e7c250a..87e5fedc2162 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -210,7 +210,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
> {
> loff_t i_size, offset;
> char *rbuf[2] = { NULL, };
> - int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
> + int rc, rbuf_len, active = 0, ahash_rc = 0;
> struct ahash_request *req;
> struct scatterlist sg[1];
> struct crypto_wait wait;
> @@ -257,11 +257,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
> &rbuf_size[1], 0);
> }
>
> - if (!(file->f_mode & FMODE_READ)) {
> - file->f_mode |= FMODE_READ;
> - read = 1;
> - }
> -
> for (offset = 0; offset < i_size; offset += rbuf_len) {
> if (!rbuf[1] && offset) {
> /* Not using two buffers, and it is not the first
> @@ -300,8 +295,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
> /* wait for the last update request to complete */
> rc = ahash_wait(ahash_rc, &wait);
> out3:
> - if (read)
> - file->f_mode &= ~FMODE_READ;
> ima_free_pages(rbuf[0], rbuf_size[0]);
> ima_free_pages(rbuf[1], rbuf_size[1]);
> out2:
> @@ -336,7 +329,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
> {
> loff_t i_size, offset = 0;
> char *rbuf;
> - int rc, read = 0;
> + int rc;
> SHASH_DESC_ON_STACK(shash, tfm);
>
> shash->tfm = tfm;
> @@ -357,11 +350,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
> if (!rbuf)
> return -ENOMEM;
>
> - if (!(file->f_mode & FMODE_READ)) {
> - file->f_mode |= FMODE_READ;
> - read = 1;
> - }
> -
> while (offset < i_size) {
> int rbuf_len;
>
> @@ -378,8 +366,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
> if (rc)
> break;
> }
> - if (read)
> - file->f_mode &= ~FMODE_READ;
> kfree(rbuf);
> out:
> if (!rc)
> @@ -420,26 +406,21 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> {
> loff_t i_size;
> int rc;
> + struct file *f = dentry_open(&file->f_path, O_LARGEFILE | O_RDONLY,
> + current_cred());
>
> - /*
> - * For consistency, fail file's opened with the O_DIRECT flag on
> - * filesystems mounted with/without DAX option.
> - */
> - if (file->f_flags & O_DIRECT) {
> - hash->length = hash_digest_size[ima_hash_algo];
> - hash->algo = ima_hash_algo;
> - return -EINVAL;
> - }
> -
> - i_size = i_size_read(file_inode(file));
> + i_size = i_size_read(file_inode(f));
>
> if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
> - rc = ima_calc_file_ahash(file, hash);
> + rc = ima_calc_file_ahash(f, hash);
> if (!rc)
> - return 0;
> + goto out;
> }
>
> - return ima_calc_file_shash(file, hash);
> + rc = ima_calc_file_shash(f, hash);
> +out:
> + fput(f);
> + return rc;
> }
>
> /*
>
