Re: [PATCH 1/1] ovl: setxattr: avoid deadlock when writing IMA xattrs

Re: [PATCH 1/1] ovl: setxattr: avoid deadlock when writing IMA xattrs

[Mimi Zohar]

Hi Krisztian,  

> If we're writing an 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.

But it probably isn't the only function calling ovl_setxattr().   So in
addition to testing S_IMA, only if the security.ima xattr is being set,
would this be safe.

Mimi

> In practice we test if the real inode has the S_IMA flag
> set and if it does we call __vfs_setxattr_noperm instead
> of the usual vfs_setxattr we call for all other cases.
> 
> Signed-off-by: Krisztian Litkey <kli@iki.fi>
> ---
>  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..9257e8d 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);
> +
> +   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
> 

[PATCH v2 1/1] ovl: setxattr: avoid deadlock when setting IMA xattr.

[Krisztian Litkey]

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.

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@iki.fi>
---
 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

Re: [PATCH v2 1/1] ovl: setxattr: avoid deadlock when setting IMA xattr.

[Krisztian Litkey]

On Mon, May 16, 2016 at 5:20 PM, Mimi Zohar <zohar@us.ibm.com> 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@iki.fi>
>
> 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
>>
>

Re: [PATCH v2 1/1] ovl: setxattr: avoid deadlock when setting IMA xattr.

[Krisztian Litkey]

On Mon, May 16, 2016 at 6:13 PM, Krisztian Litkey <kli@iki.fi> wrote:
> On Mon, May 16, 2016 at 5:20 PM, Mimi Zohar <zohar@us.ibm.com> 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@iki.fi>
>>
>> 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 ?

Hmm... after having taken a closer look at ecryptfs, I think its setxattr
is not comparable to overlayfs. The former is much simpler because
it simply passes the call on to the underlying filesystem implementation
calling vfs_setxattr, much like overlayfs was trying to do. But ecryptfs
does not need to do anything else besides setting the attribute so it can
call vfs_setxattr without taking any locks or mutexes itself.

However, there is a crucial difference in overlayfs: it might need to
copy the dentry (and all directories leading up to it) on demand in
setxattr. This happens if setxattr is being called for a file system entry
which has never been modified in/through the overlay. I think this copy
operation is what really should be protected by mnt_{want,drop}_write.

So the right thing to do would be to protect the copy and do the rest by
unconditionally calling __vfs_setxattr_noperm. But wouldn't that just bring
us back to the original problem: if i_mutex is already held, can't really
mnt_want_write without triggering the lockdep problem, right ?

>
>>
>> Mimi
>>

  Cheers,
    Krisztian

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

Re: [PATCH v2 1/1] ovl: setxattr: avoid deadlock when setting IMA xattr.

[Mimi Zohar]

On Mon, 2016-05-16 at 23:22 +0300, Krisztian Litkey wrote:
> On Mon, May 16, 2016 at 6:13 PM, Krisztian Litkey <kli@iki.fi> wrote:
> > On Mon, May 16, 2016 at 5:20 PM, Mimi Zohar <zohar@us.ibm.com> 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@iki.fi>
> >>
> >> 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.

Another option, as the i_mutex lock is only taken for security.ima when
called from ima_file_free (__fput), would be to check if i_mutex is
already locked (mutex_is_locked).  If all three - IS_IMA, security.ima
and i_mutex taken -  call __vfs_setxattr_noperm().  Not a great
solution, but it is a solution.

> > 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 ?
> 
> Hmm... after having taken a closer look at ecryptfs, I think its setxattr
> is not comparable to overlayfs. The former is much simpler because
> it simply passes the call on to the underlying filesystem implementation
> calling vfs_setxattr, much like overlayfs was trying to do. But ecryptfs
> does not need to do anything else besides setting the attribute so it can
> call vfs_setxattr without taking any locks or mutexes itself.
> 
> However, there is a crucial difference in overlayfs: it might need to
> copy the dentry (and all directories leading up to it) on demand in
> setxattr. This happens if setxattr is being called for a file system entry
> which has never been modified in/through the overlay. I think this copy
> operation is what really should be protected by mnt_{want,drop}_write.
> 
> So the right thing to do would be to protect the copy and do the rest by
> unconditionally calling __vfs_setxattr_noperm. But wouldn't that just bring
> us back to the original problem: if i_mutex is already held, can't really
> mnt_want_write without triggering the lockdep problem, right ?

Yes, I think so.

Mimi

[PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Krisztian Litkey]

IMA tracks the integrity of files by hashing the file content
and storing the hash in an IMA-specific extended attribute
(security.ima). When a file opened for writing is closed by
the last writer, IMA recalculates the hash and updates the
extended attribute. Updating the attribute happens by locking
the inode mutex and calling  __vfs_setxattr_noperm.

For a file on an overlayfs mount, this causes ovl_setxattr
being eventually called (from __vfs_setxattr_noperm via
inode->i_op->setxattr) with the inode already locked. In this
case we cannot do the xattr setting by calling vfs_setxattr
since that will try to lock the inode mutex again. To avoid
a deadlock, we check if the mutex is already locked and call
__vfs_setxattr_noperm or vfs_setxattr accordingly.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
---
 fs/overlayfs/inode.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b29036a..25bfeeb 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -223,25 +223,28 @@ int ovl_setxattr(struct dentry *dentry, const char *name,
 		 const void *value, size_t size, int flags)
 {
 	int err;
-	struct dentry *upperdentry;
+	struct dentry *upper;
+
+	if (ovl_is_private_xattr(name))
+		return -EPERM;
 
 	err = ovl_want_write(dentry);
 	if (err)
 		goto out;
 
-	err = -EPERM;
-	if (ovl_is_private_xattr(name))
-		goto out_drop_write;
-
 	err = ovl_copy_up(dentry);
-	if (err)
-		goto out_drop_write;
+	if (!err)
+		upper = ovl_dentry_upper(dentry);
 
-	upperdentry = ovl_dentry_upper(dentry);
-	err = vfs_setxattr(upperdentry, name, value, size, flags);
-
-out_drop_write:
 	ovl_drop_write(dentry);
+
+	if (err)
+		goto out;
+
+	if (mutex_is_locked(&upper->d_inode->i_mutex))
+		err = __vfs_setxattr_noperm(upper, name, value, size, flags);
+	else
+		err = vfs_setxattr(upper, name, value, size, flags);
 out:
 	return err;
 }
-- 
2.5.5

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

Hi Krisztian,

cc'ing  LSM and AL for comments

On Fri, 2016-05-20 at 02:28 -0400, Krisztian Litkey wrote:
> IMA tracks the integrity of files by hashing the file content
> and storing the hash in an IMA-specific extended attribute
> (security.ima). When a file opened for writing is closed by
> the last writer, IMA recalculates the hash and updates the
> extended attribute. Updating the attribute happens by locking
> the inode mutex and calling  __vfs_setxattr_noperm.
> 
> For a file on an overlayfs mount, this causes ovl_setxattr
> being eventually called (from __vfs_setxattr_noperm via
> inode->i_op->setxattr) with the inode already locked. In this
> case we cannot do the xattr setting by calling vfs_setxattr
> since that will try to lock the inode mutex again. To avoid
> a deadlock, we check if the mutex is already locked and call
> __vfs_setxattr_noperm or vfs_setxattr accordingly.
> 
> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
> ---
>  fs/overlayfs/inode.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b29036a..25bfeeb 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -223,25 +223,28 @@ int ovl_setxattr(struct dentry *dentry, const char *name,
>  		 const void *value, size_t size, int flags)
>  {
>  	int err;
> -	struct dentry *upperdentry;
> +	struct dentry *upper;
> +
> +	if (ovl_is_private_xattr(name))
> +		return -EPERM;
> 
>  	err = ovl_want_write(dentry);
>  	if (err)
>  		goto out;
> 
> -	err = -EPERM;
> -	if (ovl_is_private_xattr(name))
> -		goto out_drop_write;
> -
>  	err = ovl_copy_up(dentry);
> -	if (err)
> -		goto out_drop_write;
> +	if (!err)
> +		upper = ovl_dentry_upper(dentry);
> 
> -	upperdentry = ovl_dentry_upper(dentry);
> -	err = vfs_setxattr(upperdentry, name, value, size, flags);
> -
> -out_drop_write:
>  	ovl_drop_write(dentry);
> +
> +	if (err)
> +		goto out;
> +
> +	if (mutex_is_locked(&upper->d_inode->i_mutex))
> +		err = __vfs_setxattr_noperm(upper, name, value, size, flags);

As far as I'm aware, the only time that i_mutex is taken, is during
__fput() when IMA writes security.ima.   Previous versions of this patch
checked whether the xattr being written was security.ima.  It would
probably be a good idea not to make that assumption here.   The question
is what should happen if the i_mutex is locked, but the xattr isn't
security.ima.  At minimum it should be audited.  Al, any comments?

Mimi

> +	else
> +		err = vfs_setxattr(upper, name, value, size, flags);
>  out:
>  	return err;
>  }

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Al Viro]

On Fri, May 20, 2016 at 10:21:27AM -0400, Mimi Zohar wrote:
> > +	if (mutex_is_locked(&upper->d_inode->i_mutex))
> > +		err = __vfs_setxattr_noperm(upper, name, value, size, flags);
> 
> As far as I'm aware, the only time that i_mutex is taken, is during
> __fput() when IMA writes security.ima.   Previous versions of this patch
> checked whether the xattr being written was security.ima.  It would
> probably be a good idea not to make that assumption here.   The question
> is what should happen if the i_mutex is locked, but the xattr isn't
> security.ima.  At minimum it should be audited.  Al, any comments?

ITYM "printable", and that's somewhat harder.  OK, let me try:

Anybody using ..._is_lock() kind of primitives that way ought to be
(re)educated before they are allowed near any kind of multithreaded
code _anywhere_.  mutex could've been held by a different thread of
execution and dropped just as mutex_is_locked() returns.  Or at any
subsequent point.  This is 100% bogus; one should *never* write that kind
of code.  As in "here's your well-earned F-, better luck next semester".

I haven't seen the full patch (you've quoted only a part of that gem), but
about the only way for it to be correct is to have it continue with
+ else
+       err = <identical call>

	Practically all calls of mutex_is_locked() are of form
WARN_ON(!mutex_is_locked(...)) or equivalent thereof.  And the rest contains
similar... wonders - for example, take a look at drivers/media/rc/imon.c;
imon_ir_change_protocol() has this
        if (!mutex_is_locked(&ictx->lock)) {
                unlock = true;
                mutex_lock(&ictx->lock);
        }

        retval = send_packet(ictx);
        if (retval)
                goto out;

        ictx->rc_type = *rc_type;
        ictx->pad_mouse = false;

out:
        if (unlock)
                mutex_unlock(&ictx->lock);
Finding why it's exploitably racy is left as a trivial exercise for readers...

Folks, if you see something of that sort in the code, it's a huge red flag.
There are legitimate uses of mutex_is_locked other than asserts, but those
are extremely rare.

I would need to see more context to be able to comment on the problem in
question, but this patch is almost certainly broken.

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Fri, 2016-05-20 at 17:29 +0100, Al Viro wrote:
> On Fri, May 20, 2016 at 10:21:27AM -0400, Mimi Zohar wrote:
> > > +	if (mutex_is_locked(&upper->d_inode->i_mutex))
> > > +		err = __vfs_setxattr_noperm(upper, name, value, size, flags);
> > 
> > As far as I'm aware, the only time that i_mutex is taken, is during
> > __fput() when IMA writes security.ima.   Previous versions of this patch
> > checked whether the xattr being written was security.ima.  It would
> > probably be a good idea not to make that assumption here.   The question
> > is what should happen if the i_mutex is locked, but the xattr isn't
> > security.ima.  At minimum it should be audited.  Al, any comments?
> 
> ITYM "printable", and that's somewhat harder.  OK, let me try:
> 
> Anybody using ..._is_lock() kind of primitives that way ought to be
> (re)educated before they are allowed near any kind of multithreaded
> code _anywhere_.  mutex could've been held by a different thread of
> execution and dropped just as mutex_is_locked() returns.  Or at any
> subsequent point.  This is 100% bogus; one should *never* write that kind
> of code.  As in "here's your well-earned F-, better luck next semester".
> 
> I haven't seen the full patch (you've quoted only a part of that gem), but
> about the only way for it to be correct is to have it continue with
> + else
> +       err = <identical call>
> 
> 	Practically all calls of mutex_is_locked() are of form
> WARN_ON(!mutex_is_locked(...)) or equivalent thereof.  And the rest contains
> similar... wonders - for example, take a look at drivers/media/rc/imon.c;
> imon_ir_change_protocol() has this
>         if (!mutex_is_locked(&ictx->lock)) {
>                 unlock = true;
>                 mutex_lock(&ictx->lock);
>         }
> 
>         retval = send_packet(ictx);
>         if (retval)
>                 goto out;
> 
>         ictx->rc_type = *rc_type;
>         ictx->pad_mouse = false;
> 
> out:
>         if (unlock)
>                 mutex_unlock(&ictx->lock);
> Finding why it's exploitably racy is left as a trivial exercise for readers...
> 
> Folks, if you see something of that sort in the code, it's a huge red flag.
> There are legitimate uses of mutex_is_locked other than asserts, but those
> are extremely rare.

My fault for even suggesting it.

> I would need to see more context to be able to comment on the problem in
> question, but this patch is almost certainly broken.

We deferred __fput() back in 2012 in order for IMA to safely take the
i_mutex and write security.ima.   Writing the security.ima xattr now
triggers overlayfs to write the xattr, but overlayfs doesn't
differentiate between callers - as a result of userspace or as described
here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
except the one triggered by __fput().   Refer to the original lockdep
report -    
http://thread.gmane.org/gmane.linux.file-systems.union/640

Al, any help in resolving this lockdep would be much appreciated.

Mimi

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Krisztian Litkey]

On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Fri, 2016-05-20 at 17:29 +0100, Al Viro wrote:
>> On Fri, May 20, 2016 at 10:21:27AM -0400, Mimi Zohar wrote:
>> > > + if (mutex_is_locked(&upper->d_inode->i_mutex))
>> > > +         err = __vfs_setxattr_noperm(upper, name, value, size, flags);
>> >
>> > As far as I'm aware, the only time that i_mutex is taken, is during
>> > __fput() when IMA writes security.ima.   Previous versions of this patch
>> > checked whether the xattr being written was security.ima.  It would
>> > probably be a good idea not to make that assumption here.   The question
>> > is what should happen if the i_mutex is locked, but the xattr isn't
>> > security.ima.  At minimum it should be audited.  Al, any comments?
>>
>> ITYM "printable", and that's somewhat harder.  OK, let me try:
>>
>> Anybody using ..._is_lock() kind of primitives that way ought to be
>> (re)educated before they are allowed near any kind of multithreaded
>> code _anywhere_.  mutex could've been held by a different thread of
>> execution and dropped just as mutex_is_locked() returns.  Or at any
>> subsequent point.  This is 100% bogus; one should *never* write that kind
>> of code.  As in "here's your well-earned F-, better luck next semester".
>>
>> I haven't seen the full patch (you've quoted only a part of that gem), but
>> about the only way for it to be correct is to have it continue with
>> + else
>> +       err = <identical call>
>>
>>       Practically all calls of mutex_is_locked() are of form
>> WARN_ON(!mutex_is_locked(...)) or equivalent thereof.  And the rest contains
>> similar... wonders - for example, take a look at drivers/media/rc/imon.c;
>> imon_ir_change_protocol() has this
>>         if (!mutex_is_locked(&ictx->lock)) {
>>                 unlock = true;
>>                 mutex_lock(&ictx->lock);
>>         }
>>
>>         retval = send_packet(ictx);
>>         if (retval)
>>                 goto out;
>>
>>         ictx->rc_type = *rc_type;
>>         ictx->pad_mouse = false;
>>
>> out:
>>         if (unlock)
>>                 mutex_unlock(&ictx->lock);
>> Finding why it's exploitably racy is left as a trivial exercise for readers...
>>
>> Folks, if you see something of that sort in the code, it's a huge red flag.
>> There are legitimate uses of mutex_is_locked other than asserts, but those
>> are extremely rare.
>
> My fault for even suggesting it.

Mimi, it is not your fault. I should have never missed something so
ridiculously trivial as this. I was simply being an idiot and rolled a
patch with my brain completely shut off and without stopping for
a single second to think about what I was doing. I agre with Al that
in that state of mind one should not be anywhere near a keyboard,
let alone trying to write kernel code. Got what I deserved.

>> I would need to see more context to be able to comment on the problem in
>> question, but this patch is almost certainly broken.
>
> We deferred __fput() back in 2012 in order for IMA to safely take the
> i_mutex and write security.ima.   Writing the security.ima xattr now
> triggers overlayfs to write the xattr, but overlayfs doesn't
> differentiate between callers - as a result of userspace or as described
> here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
> except the one triggered by __fput().   Refer to the original lockdep
> report -
> http://thread.gmane.org/gmane.linux.file-systems.union/640

How about the other (maybe kludgy) option of adding a dedicated inode
flag for this, setting it in the call to __vfs_setxattr_noperm in ima_appraise
and forcibly clearing it in vfs_setxattr. ovl_setxattr could then test it and
branch accordingly...

>
> Al, any help in resolving this lockdep would be much appreciated.
>
> Mimi
>

Cheers,
  Krisztian the Idiot, hiding in a quiet corner, bowing his head in shame


> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Miklos Szeredi]

On Fri, May 20, 2016 at 11:53:18PM +0300, Krisztian Litkey wrote:
> On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > We deferred __fput() back in 2012 in order for IMA to safely take the
> > i_mutex and write security.ima.   Writing the security.ima xattr now
> > triggers overlayfs to write the xattr, but overlayfs doesn't
> > differentiate between callers - as a result of userspace or as described
> > here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
> > except the one triggered by __fput().   Refer to the original lockdep
> > report -
> > http://thread.gmane.org/gmane.linux.file-systems.union/640

Looks like more fallout from 4bacc9c9234c ("overlayfs: Make f_path always point
to the overlay and f_inode to the underlay").

Not sure what we want here.  Doing everything on the underlying dentry/inode
would be trivial (see attached patch).

Question is, can we get setxattr() on file opened for O_RDONLY?  If so, then
that could fail on overlayfs (lower layer is read-only).

Thanks,
Miklos

---
From: Miklos Szeredi <mszeredi@redhat.com>
Subject: ima: use file_path()

Ima tries to call ->setxattr() on overlayfs dentry after having locked
underlying inode, which results in a deadlock.

Reported-by: Krisztian Litkey <kli@iki.fi>
Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: <stable@vger.kernel.org> # v4.2
---
 security/integrity/ima/ima_appraise.c |    4 ++--
 security/integrity/ima/ima_main.c     |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -222,7 +222,7 @@ static int process_measurement(struct fi
 	if ((action & IMA_APPRAISE_SUBMASK) ||
 		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
 		/* read 'security.ima' */
-		xattr_len = ima_read_xattr(file->f_path.dentry, &xattr_value);
+		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
 
 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
 
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -190,7 +190,7 @@ int ima_appraise_measurement(enum ima_ho
 {
 	static const char op[] = "appraise_data";
 	char *cause = "unknown";
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
 	int rc = xattr_len, hash_start = 0;
@@ -295,7 +295,7 @@ int ima_appraise_measurement(enum ima_ho
  */
 void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *dentry = file_dentry(file);
 	int rc = 0;
 
 	/* do not collect and update hash for digital signatures */

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Al Viro]

	Only tangentially related, but... that bug had been discussed,
without any results: the fallback in ima_d_path() to ->d_name.name is
completely broken.  There is no warranty whatsoever that dentry won't be
renamed, with its earlier (too long to be embedded into dentry itself)
->d_name.name getting freed.  Right under your code.

	Could we please get rid of that thing?  "A message in a near-oom
situation might be less informative than we'd like" is better than
"this code might end up dereferencing freed memory".

	Another similar bug is in ima_collect_measurement() -
        const char *filename = file->f_path.dentry->d_name.name;
	...
                integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
                                    filename, "collect_data", audit_cause,
with no warranty whatsoever that you are not passing a pointer to freed
memory.  The same goes for ima_eventname_init_common() -
        if (event_data->file) {
                cur_filename = event_data->file->f_path.dentry->d_name.name;
                cur_filename_len = strlen(cur_filename);
        } else  
...
        return ima_write_template_field_data(cur_filename, cur_filename_len,
                                             DATA_FMT_STRING, field_data);

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 17:50 +0100, Al Viro wrote:
> 	Only tangentially related, but... that bug had been discussed,
> without any results: the fallback in ima_d_path() to ->d_name.name is
> completely broken.  There is no warranty whatsoever that dentry won't be
> renamed, with its earlier (too long to be embedded into dentry itself)
> ->d_name.name getting freed.  Right under your code.

Isn't i_mutex required?

> 
> 	Could we please get rid of that thing?  "A message in a near-oom
> situation might be less informative than we'd like" is better than
> "this code might end up dereferencing freed memory".
> 
> 	Another similar bug is in ima_collect_measurement() -
>         const char *filename = file->f_path.dentry->d_name.name;
> 	...
>                 integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
>                                     filename, "collect_data", audit_cause,
> with no warranty whatsoever that you are not passing a pointer to freed
> memory.  The same goes for ima_eventname_init_common() -
>         if (event_data->file) {
>                 cur_filename = event_data->file->f_path.dentry->d_name.name;
>                 cur_filename_len = strlen(cur_filename);
>         } else  
> ...
>         return ima_write_template_field_data(cur_filename, cur_filename_len,
>                                              DATA_FMT_STRING, field_data);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 17:50 +0100, Al Viro wrote:
> 	Only tangentially related, but... that bug had been discussed,
> without any results: the fallback in ima_d_path() to ->d_name.name is
> completely broken.  There is no warranty whatsoever that dentry won't be
> renamed, with its earlier (too long to be embedded into dentry itself)
> ->d_name.name getting freed.  Right under your code.

Isn't i_mutex required?

> 
> 	Could we please get rid of that thing?  "A message in a near-oom
> situation might be less informative than we'd like" is better than
> "this code might end up dereferencing freed memory".
> 
> 	Another similar bug is in ima_collect_measurement() -
>         const char *filename = file->f_path.dentry->d_name.name;
> 	...
>                 integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
>                                     filename, "collect_data", audit_cause,
> with no warranty whatsoever that you are not passing a pointer to freed
> memory.  The same goes for ima_eventname_init_common() -
>         if (event_data->file) {
>                 cur_filename = event_data->file->f_path.dentry->d_name.name;
>                 cur_filename_len = strlen(cur_filename);
>         } else  
> ...
>         return ima_write_template_field_data(cur_filename, cur_filename_len,
>                                              DATA_FMT_STRING, field_data);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 17:50 +0100, Al Viro wrote:
> 	Only tangentially related, but... that bug had been discussed,
> without any results: the fallback in ima_d_path() to ->d_name.name is
> completely broken.  There is no warranty whatsoever that dentry won't be
> renamed, with its earlier (too long to be embedded into dentry itself)
> ->d_name.name getting freed.  Right under your code.

Isn't i_mutex required?

> 
> 	Could we please get rid of that thing?  "A message in a near-oom
> situation might be less informative than we'd like" is better than
> "this code might end up dereferencing freed memory".
> 
> 	Another similar bug is in ima_collect_measurement() -
>         const char *filename = file->f_path.dentry->d_name.name;
> 	...
>                 integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
>                                     filename, "collect_data", audit_cause,
> with no warranty whatsoever that you are not passing a pointer to freed
> memory.  The same goes for ima_eventname_init_common() -
>         if (event_data->file) {
>                 cur_filename = event_data->file->f_path.dentry->d_name.name;
>                 cur_filename_len = strlen(cur_filename);
>         } else  
> ...
>         return ima_write_template_field_data(cur_filename, cur_filename_len,
>                                              DATA_FMT_STRING, field_data);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 17:50 +0100, Al Viro wrote:
> 	Only tangentially related, but... that bug had been discussed,
> without any results: the fallback in ima_d_path() to ->d_name.name is
> completely broken.  There is no warranty whatsoever that dentry won't be
> renamed, with its earlier (too long to be embedded into dentry itself)
> ->d_name.name getting freed.  Right under your code.

Isn't i_mutex required?

> 
> 	Could we please get rid of that thing?  "A message in a near-oom
> situation might be less informative than we'd like" is better than
> "this code might end up dereferencing freed memory".
> 
> 	Another similar bug is in ima_collect_measurement() -
>         const char *filename = file->f_path.dentry->d_name.name;
> 	...
>                 integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
>                                     filename, "collect_data", audit_cause,
> with no warranty whatsoever that you are not passing a pointer to freed
> memory.  The same goes for ima_eventname_init_common() -
>         if (event_data->file) {
>                 cur_filename = event_data->file->f_path.dentry->d_name.name;
>                 cur_filename_len = strlen(cur_filename);
>         } else  
> ...
>         return ima_write_template_field_data(cur_filename, cur_filename_len,
>                                              DATA_FMT_STRING, field_data);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 16:10 +0200, Miklos Szeredi wrote:
> On Fri, May 20, 2016 at 11:53:18PM +0300, Krisztian Litkey wrote:
> > On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > We deferred __fput() back in 2012 in order for IMA to safely take the
> > > i_mutex and write security.ima.   Writing the security.ima xattr now
> > > triggers overlayfs to write the xattr, but overlayfs doesn't
> > > differentiate between callers - as a result of userspace or as described
> > > here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
> > > except the one triggered by __fput().   Refer to the original lockdep
> > > report -
> > > http://thread.gmane.org/gmane.linux.file-systems.union/640
> 
> Looks like more fallout from 4bacc9c9234c ("overlayfs: Make f_path always point
> to the overlay and f_inode to the underlay").
> 
> Not sure what we want here.  Doing everything on the underlying dentry/inode
> would be trivial (see attached patch).
> 
> Question is, can we get setxattr() on file opened for O_RDONLY?  If so, then
> that could fail on overlayfs (lower layer is read-only).

Normally only after a file has been modified is the xattr written.
However in "fix" mode, the xattr would be written for files opened
read-only (eg. bprm, mmap execute).

Mimi

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 16:10 +0200, Miklos Szeredi wrote:
> On Fri, May 20, 2016 at 11:53:18PM +0300, Krisztian Litkey wrote:
> > On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > We deferred __fput() back in 2012 in order for IMA to safely take the
> > > i_mutex and write security.ima.   Writing the security.ima xattr now
> > > triggers overlayfs to write the xattr, but overlayfs doesn't
> > > differentiate between callers - as a result of userspace or as described
> > > here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
> > > except the one triggered by __fput().   Refer to the original lockdep
> > > report -
> > > http://thread.gmane.org/gmane.linux.file-systems.union/640
> 
> Looks like more fallout from 4bacc9c9234c ("overlayfs: Make f_path always point
> to the overlay and f_inode to the underlay").
> 
> Not sure what we want here.  Doing everything on the underlying dentry/inode
> would be trivial (see attached patch).
> 
> Question is, can we get setxattr() on file opened for O_RDONLY?  If so, then
> that could fail on overlayfs (lower layer is read-only).

Normally only after a file has been modified is the xattr written.
However in "fix" mode, the xattr would be written for files opened
read-only (eg. bprm, mmap execute).

Mimi

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 16:10 +0200, Miklos Szeredi wrote:
> On Fri, May 20, 2016 at 11:53:18PM +0300, Krisztian Litkey wrote:
> > On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > We deferred __fput() back in 2012 in order for IMA to safely take the
> > > i_mutex and write security.ima.   Writing the security.ima xattr now
> > > triggers overlayfs to write the xattr, but overlayfs doesn't
> > > differentiate between callers - as a result of userspace or as described
> > > here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
> > > except the one triggered by __fput().   Refer to the original lockdep
> > > report -
> > > http://thread.gmane.org/gmane.linux.file-systems.union/640
> 
> Looks like more fallout from 4bacc9c9234c ("overlayfs: Make f_path always point
> to the overlay and f_inode to the underlay").
> 
> Not sure what we want here.  Doing everything on the underlying dentry/inode
> would be trivial (see attached patch).
> 
> Question is, can we get setxattr() on file opened for O_RDONLY?  If so, then
> that could fail on overlayfs (lower layer is read-only).

Normally only after a file has been modified is the xattr written.
However in "fix" mode, the xattr would be written for files opened
read-only (eg. bprm, mmap execute).

Mimi

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Mimi Zohar]

On Mon, 2016-05-30 at 16:10 +0200, Miklos Szeredi wrote:
> On Fri, May 20, 2016 at 11:53:18PM +0300, Krisztian Litkey wrote:
> > On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > We deferred __fput() back in 2012 in order for IMA to safely take the
> > > i_mutex and write security.ima.   Writing the security.ima xattr now
> > > triggers overlayfs to write the xattr, but overlayfs doesn't
> > > differentiate between callers - as a result of userspace or as described
> > > here in __fput().   All calls to ovl_setxattr() should call vfs_sexattr,
> > > except the one triggered by __fput().   Refer to the original lockdep
> > > report -
> > > http://thread.gmane.org/gmane.linux.file-systems.union/640
> 
> Looks like more fallout from 4bacc9c9234c ("overlayfs: Make f_path always point
> to the overlay and f_inode to the underlay").
> 
> Not sure what we want here.  Doing everything on the underlying dentry/inode
> would be trivial (see attached patch).
> 
> Question is, can we get setxattr() on file opened for O_RDONLY?  If so, then
> that could fail on overlayfs (lower layer is read-only).

Normally only after a file has been modified is the xattr written.
However in "fix" mode, the xattr would be written for files opened
read-only (eg. bprm, mmap execute).

Mimi

Re: [PATCH v3 1/1] ovl: setxattr: don't deadlock when called from ima_fix_xattr.

[Andy Whitcroft]

On Fri, May 20, 2016 at 02:28:38AM -0400, Krisztian Litkey wrote:
> IMA tracks the integrity of files by hashing the file content
> and storing the hash in an IMA-specific extended attribute
> (security.ima). When a file opened for writing is closed by
> the last writer, IMA recalculates the hash and updates the
> extended attribute. Updating the attribute happens by locking
> the inode mutex and calling  __vfs_setxattr_noperm.
> 
> For a file on an overlayfs mount, this causes ovl_setxattr
> being eventually called (from __vfs_setxattr_noperm via
> inode->i_op->setxattr) with the inode already locked. In this
> case we cannot do the xattr setting by calling vfs_setxattr
> since that will try to lock the inode mutex again. To avoid
> a deadlock, we check if the mutex is already locked and call
> __vfs_setxattr_noperm or vfs_setxattr accordingly.
> 
> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
> ---
>  fs/overlayfs/inode.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b29036a..25bfeeb 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -223,25 +223,28 @@ int ovl_setxattr(struct dentry *dentry, const char *name,
>  		 const void *value, size_t size, int flags)
>  {
>  	int err;
> -	struct dentry *upperdentry;
> +	struct dentry *upper;
> +
> +	if (ovl_is_private_xattr(name))
> +		return -EPERM;
>  
>  	err = ovl_want_write(dentry);
>  	if (err)
>  		goto out;
>  
> -	err = -EPERM;
> -	if (ovl_is_private_xattr(name))
> -		goto out_drop_write;
> -
>  	err = ovl_copy_up(dentry);
> -	if (err)
> -		goto out_drop_write;
> +	if (!err)
> +		upper = ovl_dentry_upper(dentry);
>  
> -	upperdentry = ovl_dentry_upper(dentry);
> -	err = vfs_setxattr(upperdentry, name, value, size, flags);
> -
> -out_drop_write:
>  	ovl_drop_write(dentry);
> +
> +	if (err)
> +		goto out;
> +
> +	if (mutex_is_locked(&upper->d_inode->i_mutex))
> +		err = __vfs_setxattr_noperm(upper, name, value, size, flags);
> +	else
> +		err = vfs_setxattr(upper, name, value, size, flags);
>  out:
>  	return err;

This is making the assumption that it is us who have locked it already.
What if it is not, what if it is another thread? It being locked does
not tell us it is us locking it and that it will remain locked does it?

-apw