diff mbox

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

Message ID 20160530141015.GC5758@veci.piliscsaba.szeredi.hu (mailing list archive)
State New, archived
Headers show

Commit Message

Miklos Szeredi May 30, 2016, 2:10 p.m. UTC 
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(-)

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

Comments

Al Viro May 30, 2016, 4:50 p.m. UTC | #1 
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);
--
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
Mimi Zohar May 31, 2016, 2:15 a.m. UTC | #2 
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
> 


--
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
Mimi Zohar May 31, 2016, 2:29 a.m. UTC | #3 
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

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

Patch

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