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

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

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

[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  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 
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 
Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and 
f_inode to the underlay")
Signed-off-by: Miklos Szeredi 
Cc:  # 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, _value);
+   xattr_len = ima_read_xattr(file_dentry(file), _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.

[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  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 
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 
Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and 
f_inode to the underlay")
Signed-off-by: Miklos Szeredi 
Cc:  # 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, _value);
+   xattr_len = ima_read_xattr(file_dentry(file), _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.

[Krisztian Litkey]

On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar  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(>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 = 
>>
>>   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(>lock)) {
>> unlock = true;
>> mutex_lock(>lock);
>> }
>>
>> retval = send_packet(ictx);
>> if (retval)
>> goto out;
>>
>> ictx->rc_type = *rc_type;
>> ictx->pad_mouse = false;
>>
>> out:
>> if (unlock)
>> mutex_unlock(>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 majord...@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.

[Krisztian Litkey]

On Fri, May 20, 2016 at 8:00 PM, Mimi Zohar  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(>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 = 
>>
>>   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(>lock)) {
>> unlock = true;
>> mutex_lock(>lock);
>> }
>>
>> retval = send_packet(ictx);
>> if (retval)
>> goto out;
>>
>> ictx->rc_type = *rc_type;
>> ictx->pad_mouse = false;
>>
>> out:
>> if (unlock)
>> mutex_unlock(>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 majord...@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 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(>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 = 
> 
>   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(>lock)) {
> unlock = true;
> mutex_lock(>lock);
> }
> 
> retval = send_packet(ictx);
> if (retval)
> goto out;
> 
> ictx->rc_type = *rc_type;
> ictx->pad_mouse = false;
> 
> out:
> if (unlock)
> mutex_unlock(>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.

[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(>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 = 
> 
>   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(>lock)) {
> unlock = true;
> mutex_lock(>lock);
> }
> 
> retval = send_packet(ictx);
> if (retval)
> goto out;
> 
> ictx->rc_type = *rc_type;
> ictx->pad_mouse = false;
> 
> out:
> if (unlock)
> mutex_unlock(>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.

[Al Viro]

On Fri, May 20, 2016 at 10:21:27AM -0400, Mimi Zohar wrote:
> > +   if (mutex_is_locked(>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 = 

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(>lock)) {
unlock = true;
mutex_lock(>lock);
}

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

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

out:
if (unlock)
mutex_unlock(>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.

[Al Viro]

On Fri, May 20, 2016 at 10:21:27AM -0400, Mimi Zohar wrote:
> > +   if (mutex_is_locked(>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 = 

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(>lock)) {
unlock = true;
mutex_lock(>lock);
}

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

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

out:
if (unlock)
mutex_unlock(>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.