On Mon, May 16, 2016 at 5:20 PM, Mimi Zohar <zohar@xxxxxxxxxx> wrote:
> Krisztian Litkey wrote on 05/15/2016 04:07:35 PM:
>
>> If we're writing the extended attribute used by IMA, don't
>> try to lock sb_writers (mnt_want_write) or i_mutex. We're
>> being called from ima_file_free and the necessary locks
>> are already being held. Trying to lock them again will
>> deadlock.
>
> Thinking about this some more, security.ima can be written here in
> ima_file_free, but it also can be written by userspace. The former updates
> the file hash, while the latter is used to write the file signature.
>
>> In practice we test if the real inode has the S_IMA flag
>> set and if the xattr being set is the IMA attribute. If
>> both are true, we call __vfs_setxattr_noperm instead of
>> the usual vfs_setxattr we call for all other cases.
>>
>> Signed-off-by: Krisztian Litkey <kli@xxxxxx>
>
> Writing the file signature from userspace should call the normal
> vfs_setxattr().
Yes. If there was a reliable way to detect that we're being called
as an end result of ima_fix_attr we should check for that and branch
accordingly. Based on your original suggestion I was under the
impression that testing S_IMA would have been the right check.
Now based on your recent comment, and checking the code, I see
that it will be set for all inodes that are associated with an iint cache
entry...
Unless we add a dedicated *vfs_setxattr* flag just for this and pass it
from ima_fix_xattr (so that it'd propagate back to ovl_setxattr where
we test and branch accordingly), I see no easy way to check for the
necessary precondition. And adding a dedicated flag would probably
be considered a horrible kludge... so I think I need to dig deeper and
see if I can come up with another way of fixing this.
I have taken a small peek at other architecturally not quite unlike FS
implementations. Encryptfs seems to be one of the closest ones. I
will test if their approach works together with IMA and if it does I'll try
to roll a similar implementation for overlayfs. Does that sound
reasonable to you ?
However, if it turns out that a similar deadlock is triggerable using
IMA with encryptfs then I think I'll be out of ideas about how to attack
this problem.
>
> Mimi
>
Cheers,
Krisztian
>
>
>> ---
>> fs/overlayfs/inode.c | 24 ++++++++++++++++++------
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index b29036a..15f4af3 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -222,12 +222,18 @@ static bool ovl_is_private_xattr(const char *name)
>> int ovl_setxattr(struct dentry *dentry, const char *name,
>> const void *value, size_t size, int flags)
>> {
>> - int err;
>> + int err, ima;
>> struct dentry *upperdentry;
>> + struct inode *inode;
>>
>> - err = ovl_want_write(dentry);
>> - if (err)
>> - goto out;
>> + inode = ovl_dentry_real(dentry)->d_inode;
>> + ima = IS_IMA(inode) && !strcmp(name, XATTR_NAME_IMA);
>> +
>> + if (!ima) {
>> + err = ovl_want_write(dentry);
>> + if (err)
>> + goto out;
>> + }
>>
>> err = -EPERM;
>> if (ovl_is_private_xattr(name))
>> @@ -238,10 +244,16 @@ int ovl_setxattr(struct dentry *dentry, const
>> char *name,
>> goto out_drop_write;
>>
>> upperdentry = ovl_dentry_upper(dentry);
>> - err = vfs_setxattr(upperdentry, name, value, size, flags);
>> +
>> + if (!ima)
>> + err = vfs_setxattr(upperdentry, name, value, size, flags);
>> + else
>> + err = __vfs_setxattr_noperm(upperdentry, name, value, size,
>> + flags);
>>
>> out_drop_write:
>> - ovl_drop_write(dentry);
>> + if (!ima)
>> + ovl_drop_write(dentry);
>> out:
>> return err;
>> }
>> --
>> 2.5.5
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
