ovl: setxattr: avoid deadlock when writing IMA xattrs

PATCH In review Mimi Zohar 5 participants 21 messages May 15, 2016 – May 31, 2016 1 message not in the archive

Subsystems (4)

Security (LSM) Integrity measurement architecture (IMA) Extended Verification Module (EVM) Overlay filesystem

lore

Share & tools

lore

Mimi Zohar

author

2016-05-15 19:23 UTC #0 lore raw

Hi Krisztian,

Quoted 5 lines

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

Quoted 60 lines

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 <[email protected]>

---
 fs/overlayfs/inode.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

fs/overlayfs/inode.c +18 -6

index b29036a..9257e8d 100644
@@ -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;

}

Signature · 1 line

2.5.5

Krisztian Litkey

2016-05-15 20:07 UTC #1 lore raw

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

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 <[email protected]>

---
 fs/overlayfs/inode.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

fs/overlayfs/inode.c +18 -6

index b29036a..15f4af3 100644
@@ -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;
 }

Signature · 1 line

2.5.5

A message that is not in the archive (sent off-list or not yet received): [email protected]

Krisztian Litkey

2016-05-16 15:13 UTC #3 lore raw

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

Quoted 17 lines · Mimi Zohar

Krisztian Litkey wrote on 05/15/2016 04:07:35 PM:
Quoted 5 lines
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.
Quoted 5 lines
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.

Quoted 4 lines

Quoted 1 line
Signed-off-by Krisztian Litkey <[email protected]>
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.

Quoted 3 lines

Mimi

Cheers,
Krisztian

Quoted 57 lines

Quoted 56 lines


---
 fs/overlayfs/inode.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

fs/overlayfs/inode.c +18 -6

index b29036a..15f4af3 100644
@@ -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

Krisztian Litkey

2016-05-16 20:22 UTC #4 lore raw

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

Quoted 38 lines · Krisztian Litkey

Quoted 17 lines · Mimi Zohar
Krisztian Litkey wrote on 05/15/2016 04:07:35 PM:
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
Quoted 1 line
Signed-off-by Krisztian Litkey <[email protected]>
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.

Quoted 5 lines

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 ?

Quoted 4 lines

Quoted 4 lines
Mimi

Cheers,
Krisztian

Quoted 61 lines

Cheers,
Krisztian

Quoted 57 lines

Quoted 56 lines


---
 fs/overlayfs/inode.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

fs/overlayfs/inode.c +18 -6

index b29036a..15f4af3 100644
@@ -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

Mimi Zohar

author

2016-05-18 22:45 UTC #5 lore raw

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

Quoted 32 lines · Krisztian Litkey

Quoted 31 lines · Krisztian Litkey
Quoted 17 lines · Mimi Zohar
Krisztian Litkey wrote on 05/15/2016 04:07:35 PM:
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
Quoted 1 line
Signed-off-by Krisztian Litkey <[email protected]>
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...

Quoted 6 lines

Quoted 6 lines
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.

Quoted 19 lines

Quoted 5 lines
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.

Quoted 4 lines

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

Krisztian Litkey

2016-05-20 06:28 UTC #6 lore raw

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

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 <[email protected]>

---
 fs/overlayfs/inode.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

fs/overlayfs/inode.c +14 ~~-11~~

index b29036a..25bfeeb 100644
@@ -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;
 }

Signature · 1 line

2.5.5

Mimi Zohar

author

2016-05-20 14:22 UTC #7 lore raw

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

Hi Krisztian,

cc'ing LSM and AL for comments

Quoted 49 lines · 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 <[email protected]>
---
 fs/overlayfs/inode.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
fs/overlayfs/inode.c +6 -8
index b29036a..25bfeeb 100644
@@ -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);

fs/overlayfs/inode.c int ovl_setxattr(struct dentry *dentry, const char *name, · line 245

Quoted 10 lines

fs/overlayfs/inode.c +6 -3

-	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

Quoted 5 lines

fs/overlayfs/inode.c +2 -0

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

Al Viro

2016-05-20 16:29 UTC #8 lore raw

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

Quoted 9 lines · Mimi Zohar

fs/overlayfs/inode.c int ovl_setxattr(struct dentry *dentry, const char *name, · line 245
Quoted 2 lines
fs/overlayfs/inode.c +2 -0
+	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.

Mimi Zohar

author

2016-05-20 17:01 UTC #9 lore raw

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

Quoted 41 lines · Al Viro

Quoted 9 lines · Mimi Zohar
fs/overlayfs/inode.c int ovl_setxattr(struct dentry *dentry, const char *name, · line 245
Quoted 2 lines
fs/overlayfs/inode.c +2 -0
+	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;

Quoted 8 lines

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.

Quoted 2 lines

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

Krisztian Litkey

2016-05-20 20:53 UTC #10 lore raw

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

Quoted 47 lines · Mimi Zohar

Quoted 46 lines · Al Viro
Quoted 9 lines · Mimi Zohar
Quoted 2 lines
+ 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...

Quoted 5 lines

Quoted 3 lines
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.

Quoted 11 lines

Quoted 2 lines
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...

Quoted 5 lines

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

Quoted 4 lines

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

Miklos Szeredi

maintainer

2016-05-30 14:10 UTC #11 lore raw

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

Quoted 10 lines · Krisztian Litkey

Quoted 8 lines · Mimi Zohar
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 <[email protected]>
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 <[email protected]>

Fixes 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay")

Signed-off-by Miklos Szeredi <[email protected]>

Cc <[email protected]> # v4.2

---
 security/integrity/ima/ima_appraise.c |    4 ++--
 security/integrity/ima/ima_main.c     |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

security/integrity/ima/ima_main.c +1 -1

@@ -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);

security/integrity/ima/ima_appraise.c +2 -2

@@ -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 */

Al Viro

2016-05-30 16:50 UTC #12 lore raw

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

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

Mimi Zohar

author

2016-05-31 02:15 UTC #13 lore raw

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

Quoted 5 lines · 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.

Isn't i_mutex required?

Quoted 24 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mimi Zohar

author

2016-05-31 02:15 UTC #14 lore raw

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

In reply to Al Viro

Quoted 5 lines · 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.

Isn't i_mutex required?

Quoted 24 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mimi Zohar

author

2016-05-31 02:15 UTC #15 lore raw

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

In reply to Al Viro

Quoted 5 lines · 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.

Isn't i_mutex required?

Quoted 24 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mimi Zohar

author

2016-05-31 02:16 UTC #16 lore raw

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

In reply to Al Viro

Quoted 5 lines · 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.

Isn't i_mutex required?

Quoted 24 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mimi Zohar

author

2016-05-31 02:29 UTC #17 lore raw

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

In reply to Miklos Szeredi

Quoted 12 lines · Miklos Szeredi

Quoted 10 lines · Krisztian Litkey
Quoted 8 lines · Mimi Zohar
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

Quoted 8 lines

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

Mimi Zohar

author

2016-05-31 02:29 UTC #18 lore raw

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

In reply to Miklos Szeredi

Quoted 12 lines · Miklos Szeredi

Quoted 10 lines · Krisztian Litkey
Quoted 8 lines · Mimi Zohar
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

Quoted 8 lines

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

Mimi Zohar

author

2016-05-31 02:29 UTC #19 lore raw

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

In reply to Miklos Szeredi

Quoted 12 lines · Miklos Szeredi

Quoted 10 lines · Krisztian Litkey
Quoted 8 lines · Mimi Zohar
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

Quoted 8 lines

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

Mimi Zohar

author

2016-05-31 02:29 UTC #20 lore raw

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

In reply to Miklos Szeredi

Quoted 12 lines · Miklos Szeredi

Quoted 10 lines · Krisztian Litkey
Quoted 8 lines · Mimi Zohar
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

Quoted 8 lines

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

Andy Whitcroft

2016-05-20 15:18 UTC #21 lore raw

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

In reply to Krisztian Litkey

Quoted 53 lines · 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 <[email protected]>
---
 fs/overlayfs/inode.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
fs/overlayfs/inode.c +6 ~~-11~~
index b29036a..25bfeeb 100644
@@ -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);

fs/overlayfs/inode.c int ovl_setxattr(struct dentry *dentry, const char *name, · line 249

Quoted 10 lines

fs/overlayfs/inode.c +8 -0

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