[RFC PATCH 0/5] Overhaul the audit filename handling

[RFC PATCH 0/5] Overhaul the audit filename handling

[Paul Moore]

There have been some patches added to v3.19-rcX to fix various
problems in the way audit handles filenames but they have been hacks
on top of hacks, not really something we want long term.  This
patchset reworks the way audit handles filenames, removing a lot of
nasty hacks added recently, and fixing a few bugs that still remain.

Most significant to folks outside of audit, patch 5/5 does away with
the nasty getname()/putname() kludge in favor of a less ugly reference
count approach.

Comments and feedback are welcome.  If nothing ugly pops up on review
I'll see if Linus is interested in taking this for the next v3.19-rcX
release, otherwise I'll toss it into linux-next for v3.20.

-Paul

---

Paul Moore (5):
      fs: rework getname_kernel to handle up to PATH_MAX sized filenames
      fs: create proper filename objects using getname_kernel()
      audit: enable filename recording via getname_kernel()
      audit: fix filename matching in __audit_inode() and __audit_inode_child()
      audit: replace getname()/putname() hacks with reference counters


 fs/exec.c             |   11 +++
 fs/namei.c            |   98 ++++++++++++++++++-----------
 fs/open.c             |   11 +++
 include/linux/audit.h |    3 -
 include/linux/fs.h    |    9 +--
 kernel/audit.h        |   17 +----
 kernel/auditsc.c      |  167 +++++++++----------------------------------------
 7 files changed, 115 insertions(+), 201 deletions(-)

[RFC PATCH 1/5] fs: rework getname_kernel to handle up to PATH_MAX sized filenames

[Paul Moore]

In preparation for expanded use in the kernel, make getname_kernel()
more useful by allowing it to handle any legal filename length.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 fs/namei.c |   34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 9eb787e..eeb3b83 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -211,32 +211,38 @@ getname(const char __user * filename)
 	return getname_flags(filename, 0, NULL);
 }
 
-/*
- * The "getname_kernel()" interface doesn't do pathnames longer
- * than EMBEDDED_NAME_MAX. Deal with it - you're a kernel user.
- */
 struct filename *
 getname_kernel(const char * filename)
 {
 	struct filename *result;
-	char *kname;
-	int len;
-
-	len = strlen(filename);
-	if (len >= EMBEDDED_NAME_MAX)
-		return ERR_PTR(-ENAMETOOLONG);
+	int len = strlen(filename) + 1;
 
 	result = __getname();
 	if (unlikely(!result))
 		return ERR_PTR(-ENOMEM);
 
-	kname = (char *)result + sizeof(*result);
-	result->name = kname;
+	if (len <= EMBEDDED_NAME_MAX) {
+		result->name = (char *)(result) + sizeof(*result);
+		result->separate = false;
+	} else if (len <= PATH_MAX) {
+		struct filename *tmp;
+
+		tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+		if (unlikely(!tmp)) {
+			__putname(result);
+			return ERR_PTR(-ENOMEM);
+		}
+		tmp->name = (char *)result;
+		tmp->separate = true;
+		result = tmp;
+	} else {
+		__putname(result);
+		return ERR_PTR(-ENAMETOOLONG);
+	}
+	strlcpy((char *)result->name, filename, len);
 	result->uptr = NULL;
 	result->aname = NULL;
-	result->separate = false;
 
-	strlcpy(kname, filename, EMBEDDED_NAME_MAX);
 	return result;
 }
 

Re: [RFC PATCH 1/5] fs: rework getname_kernel to handle up to PATH_MAX sized filenames

[Richard Guy Briggs]

On 15/01/08, Paul Moore wrote:
> In preparation for expanded use in the kernel, make getname_kernel()
> more useful by allowing it to handle any legal filename length.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  fs/namei.c |   34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 9eb787e..eeb3b83 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -211,32 +211,38 @@ getname(const char __user * filename)
>  	return getname_flags(filename, 0, NULL);
>  }
>  
> -/*
> - * The "getname_kernel()" interface doesn't do pathnames longer
> - * than EMBEDDED_NAME_MAX. Deal with it - you're a kernel user.
> - */
>  struct filename *
>  getname_kernel(const char * filename)
>  {
>  	struct filename *result;
> -	char *kname;
> -	int len;
> -
> -	len = strlen(filename);
> -	if (len >= EMBEDDED_NAME_MAX)
> -		return ERR_PTR(-ENAMETOOLONG);
> +	int len = strlen(filename) + 1;
>  
>  	result = __getname();
>  	if (unlikely(!result))
>  		return ERR_PTR(-ENOMEM);
>  
> -	kname = (char *)result + sizeof(*result);
> -	result->name = kname;
> +	if (len <= EMBEDDED_NAME_MAX) {
> +		result->name = (char *)(result) + sizeof(*result);
> +		result->separate = false;
> +	} else if (len <= PATH_MAX) {
> +		struct filename *tmp;
> +
> +		tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> +		if (unlikely(!tmp)) {
> +			__putname(result);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		tmp->name = (char *)result;
> +		tmp->separate = true;
> +		result = tmp;
> +	} else {
> +		__putname(result);
> +		return ERR_PTR(-ENAMETOOLONG);
> +	}
> +	strlcpy((char *)result->name, filename, len);
>  	result->uptr = NULL;
>  	result->aname = NULL;
> -	result->separate = false;
>  
> -	strlcpy(kname, filename, EMBEDDED_NAME_MAX);
>  	return result;
>  }
>  
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[RFC PATCH 2/5] fs: create proper filename objects using getname_kernel()

[Paul Moore]

There are several areas in the kernel that create temporary filename
objects using the following pattern:

	int func(const char *name)
	{
		struct filename *file = { .name = name };
		...
		return 0;
	}

... which for the most part works okay, but it causes havoc within the
audit subsystem as the filename object does not persist beyond the
lifetime of the function.  This patch converts all of these temporary
filename objects into proper filename objects using getname_kernel()
and putname() which ensure that the filename object persists until the
audit subsystem is finished with it.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 fs/exec.c  |   11 +++++++++--
 fs/namei.c |   34 ++++++++++++++++++++++++++--------
 fs/open.c  |   11 +++++++++--
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index a3d33fe..d067771 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -789,8 +789,15 @@ exit:
 
 struct file *open_exec(const char *name)
 {
-	struct filename tmp = { .name = name };
-	return do_open_exec(&tmp);
+	struct file *file;
+	struct filename *tmp;
+
+	tmp = getname_kernel(name);
+	if (unlikely(IS_ERR(tmp)))
+		return (void *)tmp;
+	file = do_open_exec(tmp);
+	putname(tmp);
+	return file;
 }
 EXPORT_SYMBOL(open_exec);
 
diff --git a/fs/namei.c b/fs/namei.c
index eeb3b83..c3d21b7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2001,9 +2001,15 @@ static int filename_lookup(int dfd, struct filename *name,
 static int do_path_lookup(int dfd, const char *name,
 				unsigned int flags, struct nameidata *nd)
 {
-	struct filename filename = { .name = name };
+	int retval;
+	struct filename *filename;
 
-	return filename_lookup(dfd, &filename, flags, nd);
+	filename = getname_kernel(name);
+	if (unlikely(IS_ERR(filename)))
+		return PTR_ERR(filename);
+	retval = filename_lookup(dfd, filename, flags, nd);
+	putname(filename);
+	return retval;
 }
 
 /* does lookup, returns the object with parent locked */
@@ -2368,8 +2374,15 @@ int
 kern_path_mountpoint(int dfd, const char *name, struct path *path,
 			unsigned int flags)
 {
-	struct filename s = {.name = name};
-	return filename_mountpoint(dfd, &s, path, flags);
+	int retval;
+	struct filename *s;
+
+	s = getname_kernel(name);
+	if (unlikely(IS_ERR(s)))
+		return PTR_ERR(s);
+	retval = filename_mountpoint(dfd, s, path, flags);
+	putname(s);
+	return retval;
 }
 EXPORT_SYMBOL(kern_path_mountpoint);
 
@@ -3259,7 +3272,7 @@ struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
 {
 	struct nameidata nd;
 	struct file *file;
-	struct filename filename = { .name = name };
+	struct filename *filename;
 	int flags = op->lookup_flags | LOOKUP_ROOT;
 
 	nd.root.mnt = mnt;
@@ -3268,11 +3281,16 @@ struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
 	if (d_is_symlink(dentry) && op->intent & LOOKUP_OPEN)
 		return ERR_PTR(-ELOOP);
 
-	file = path_openat(-1, &filename, &nd, op, flags | LOOKUP_RCU);
+	filename = getname_kernel(name);
+	if (unlikely(IS_ERR(filename)))
+		return (void *)filename;
+
+	file = path_openat(-1, filename, &nd, op, flags | LOOKUP_RCU);
 	if (unlikely(file == ERR_PTR(-ECHILD)))
-		file = path_openat(-1, &filename, &nd, op, flags);
+		file = path_openat(-1, filename, &nd, op, flags);
 	if (unlikely(file == ERR_PTR(-ESTALE)))
-		file = path_openat(-1, &filename, &nd, op, flags | LOOKUP_REVAL);
+		file = path_openat(-1, filename, &nd, op, flags | LOOKUP_REVAL);
+	putname(filename);
 	return file;
 }
 
diff --git a/fs/open.c b/fs/open.c
index d6fd3ac..666982b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -940,8 +940,15 @@ struct file *file_open_name(struct filename *name, int flags, umode_t mode)
  */
 struct file *filp_open(const char *filename, int flags, umode_t mode)
 {
-	struct filename name = {.name = filename};
-	return file_open_name(&name, flags, mode);
+	struct file *file;
+	struct filename *name;
+
+	name = getname_kernel(filename);
+	if (unlikely(IS_ERR(name)))
+		return (void *)name;
+	file = file_open_name(name, flags, mode);
+	putname(name);
+	return file;
 }
 EXPORT_SYMBOL(filp_open);
 

Re: [RFC PATCH 2/5] fs: create proper filename objects using getname_kernel()

[Richard Guy Briggs]

On 15/01/08, Paul Moore wrote:
> There are several areas in the kernel that create temporary filename
> objects using the following pattern:
> 
> 	int func(const char *name)
> 	{
> 		struct filename *file = { .name = name };
> 		...
> 		return 0;
> 	}
> 
> ... which for the most part works okay, but it causes havoc within the
> audit subsystem as the filename object does not persist beyond the
> lifetime of the function.  This patch converts all of these temporary
> filename objects into proper filename objects using getname_kernel()
> and putname() which ensure that the filename object persists until the
> audit subsystem is finished with it.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  fs/exec.c  |   11 +++++++++--
>  fs/namei.c |   34 ++++++++++++++++++++++++++--------
>  fs/open.c  |   11 +++++++++--
>  3 files changed, 44 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index a3d33fe..d067771 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -789,8 +789,15 @@ exit:
>  
>  struct file *open_exec(const char *name)
>  {
> -	struct filename tmp = { .name = name };
> -	return do_open_exec(&tmp);
> +	struct file *file;
> +	struct filename *tmp;
> +
> +	tmp = getname_kernel(name);
> +	if (unlikely(IS_ERR(tmp)))
> +		return (void *)tmp;
> +	file = do_open_exec(tmp);
> +	putname(tmp);
> +	return file;
>  }
>  EXPORT_SYMBOL(open_exec);
>  
> diff --git a/fs/namei.c b/fs/namei.c
> index eeb3b83..c3d21b7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2001,9 +2001,15 @@ static int filename_lookup(int dfd, struct filename *name,
>  static int do_path_lookup(int dfd, const char *name,
>  				unsigned int flags, struct nameidata *nd)
>  {
> -	struct filename filename = { .name = name };
> +	int retval;
> +	struct filename *filename;
>  
> -	return filename_lookup(dfd, &filename, flags, nd);
> +	filename = getname_kernel(name);
> +	if (unlikely(IS_ERR(filename)))
> +		return PTR_ERR(filename);
> +	retval = filename_lookup(dfd, filename, flags, nd);
> +	putname(filename);
> +	return retval;
>  }
>  
>  /* does lookup, returns the object with parent locked */
> @@ -2368,8 +2374,15 @@ int
>  kern_path_mountpoint(int dfd, const char *name, struct path *path,
>  			unsigned int flags)
>  {
> -	struct filename s = {.name = name};
> -	return filename_mountpoint(dfd, &s, path, flags);
> +	int retval;
> +	struct filename *s;
> +
> +	s = getname_kernel(name);
> +	if (unlikely(IS_ERR(s)))
> +		return PTR_ERR(s);
> +	retval = filename_mountpoint(dfd, s, path, flags);
> +	putname(s);
> +	return retval;
>  }
>  EXPORT_SYMBOL(kern_path_mountpoint);
>  
> @@ -3259,7 +3272,7 @@ struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
>  {
>  	struct nameidata nd;
>  	struct file *file;
> -	struct filename filename = { .name = name };
> +	struct filename *filename;
>  	int flags = op->lookup_flags | LOOKUP_ROOT;
>  
>  	nd.root.mnt = mnt;
> @@ -3268,11 +3281,16 @@ struct file *do_file_open_root(struct dentry *dentry, struct vfsmount *mnt,
>  	if (d_is_symlink(dentry) && op->intent & LOOKUP_OPEN)
>  		return ERR_PTR(-ELOOP);
>  
> -	file = path_openat(-1, &filename, &nd, op, flags | LOOKUP_RCU);
> +	filename = getname_kernel(name);
> +	if (unlikely(IS_ERR(filename)))
> +		return (void *)filename;
> +
> +	file = path_openat(-1, filename, &nd, op, flags | LOOKUP_RCU);
>  	if (unlikely(file == ERR_PTR(-ECHILD)))
> -		file = path_openat(-1, &filename, &nd, op, flags);
> +		file = path_openat(-1, filename, &nd, op, flags);
>  	if (unlikely(file == ERR_PTR(-ESTALE)))
> -		file = path_openat(-1, &filename, &nd, op, flags | LOOKUP_REVAL);
> +		file = path_openat(-1, filename, &nd, op, flags | LOOKUP_REVAL);
> +	putname(filename);
>  	return file;
>  }
>  
> diff --git a/fs/open.c b/fs/open.c
> index d6fd3ac..666982b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -940,8 +940,15 @@ struct file *file_open_name(struct filename *name, int flags, umode_t mode)
>   */
>  struct file *filp_open(const char *filename, int flags, umode_t mode)
>  {
> -	struct filename name = {.name = filename};
> -	return file_open_name(&name, flags, mode);
> +	struct file *file;
> +	struct filename *name;
> +
> +	name = getname_kernel(filename);
> +	if (unlikely(IS_ERR(name)))
> +		return (void *)name;
> +	file = file_open_name(name, flags, mode);
> +	putname(name);
> +	return file;
>  }
>  EXPORT_SYMBOL(filp_open);
>  
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[RFC PATCH 3/5] audit: enable filename recording via getname_kernel()

[Paul Moore]

Enable recording of filenames in getname_kernel() and remove the
kludgy workaround in __audit_inode() now that we have proper filename
logging for kernel users.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 fs/namei.c       |    1 +
 kernel/auditsc.c |   40 +++-------------------------------------
 2 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c3d21b7..1c0d4c7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -242,6 +242,7 @@ getname_kernel(const char * filename)
 	strlcpy((char *)result->name, filename, len);
 	result->uptr = NULL;
 	result->aname = NULL;
+	audit_getname(result);
 
 	return result;
 }
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 793e9e9..c967ffc 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1882,44 +1882,10 @@ out_alloc:
 	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
 	if (!n)
 		return;
-	/* unfortunately, while we may have a path name to record with the
-	 * inode, we can't always rely on the string lasting until the end of
-	 * the syscall so we need to create our own copy, it may fail due to
-	 * memory allocation issues, but we do our best */
-	if (name) {
-		/* we can't use getname_kernel() due to size limits */
-		size_t len = strlen(name->name) + 1;
-		struct filename *new = __getname();
-
-		if (unlikely(!new))
-			goto out;
+	if (name)
+		/* no need to set ->name_put as the original will cleanup */
+		n->name = name;
 
-		if (len <= (PATH_MAX - sizeof(*new))) {
-			new->name = (char *)(new) + sizeof(*new);
-			new->separate = false;
-		} else if (len <= PATH_MAX) {
-			/* this looks odd, but is due to final_putname() */
-			struct filename *new2;
-
-			new2 = kmalloc(sizeof(*new2), GFP_KERNEL);
-			if (unlikely(!new2)) {
-				__putname(new);
-				goto out;
-			}
-			new2->name = (char *)new;
-			new2->separate = true;
-			new = new2;
-		} else {
-			/* we should never get here, but let's be safe */
-			__putname(new);
-			goto out;
-		}
-		strlcpy((char *)new->name, name->name, len);
-		new->uptr = NULL;
-		new->aname = n;
-		n->name = new;
-		n->name_put = true;
-	}
 out:
 	if (parent) {
 		n->name_len = n->name ? parent_len(n->name->name) : AUDIT_NAME_FULL;

Re: [RFC PATCH 3/5] audit: enable filename recording via getname_kernel()

[Richard Guy Briggs]

On 15/01/08, Paul Moore wrote:
> Enable recording of filenames in getname_kernel() and remove the
> kludgy workaround in __audit_inode() now that we have proper filename
> logging for kernel users.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  fs/namei.c       |    1 +
>  kernel/auditsc.c |   40 +++-------------------------------------
>  2 files changed, 4 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index c3d21b7..1c0d4c7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -242,6 +242,7 @@ getname_kernel(const char * filename)
>  	strlcpy((char *)result->name, filename, len);
>  	result->uptr = NULL;
>  	result->aname = NULL;
> +	audit_getname(result);
>  
>  	return result;
>  }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 793e9e9..c967ffc 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1882,44 +1882,10 @@ out_alloc:
>  	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
>  	if (!n)
>  		return;
> -	/* unfortunately, while we may have a path name to record with the
> -	 * inode, we can't always rely on the string lasting until the end of
> -	 * the syscall so we need to create our own copy, it may fail due to
> -	 * memory allocation issues, but we do our best */
> -	if (name) {
> -		/* we can't use getname_kernel() due to size limits */
> -		size_t len = strlen(name->name) + 1;
> -		struct filename *new = __getname();
> -
> -		if (unlikely(!new))
> -			goto out;
> +	if (name)
> +		/* no need to set ->name_put as the original will cleanup */
> +		n->name = name;
>  
> -		if (len <= (PATH_MAX - sizeof(*new))) {
> -			new->name = (char *)(new) + sizeof(*new);
> -			new->separate = false;
> -		} else if (len <= PATH_MAX) {
> -			/* this looks odd, but is due to final_putname() */
> -			struct filename *new2;
> -
> -			new2 = kmalloc(sizeof(*new2), GFP_KERNEL);
> -			if (unlikely(!new2)) {
> -				__putname(new);
> -				goto out;
> -			}
> -			new2->name = (char *)new;
> -			new2->separate = true;
> -			new = new2;
> -		} else {
> -			/* we should never get here, but let's be safe */
> -			__putname(new);
> -			goto out;
> -		}
> -		strlcpy((char *)new->name, name->name, len);
> -		new->uptr = NULL;
> -		new->aname = n;
> -		n->name = new;
> -		n->name_put = true;
> -	}
>  out:
>  	if (parent) {
>  		n->name_len = n->name ? parent_len(n->name->name) : AUDIT_NAME_FULL;
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[RFC PATCH 4/5] audit: fix filename matching in __audit_inode() and __audit_inode_child()

[Paul Moore]

In all likelihood there were some subtle, and perhaps not so subtle,
bugs with filename matching in audit_inode() and audit_inode_child()
for some time, however, recent changes to the audit filename code have
definitely broken the filename matching code.  The breakage could
result in duplicate filenames in the audit log and other odd audit
record entries.  This patch fixes the filename matching code and
restores some sanity to the filename audit records.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 kernel/auditsc.c |   34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index c967ffc..98edf06 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1846,6 +1846,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
 	/* The struct filename _must_ have a populated ->name */
 	BUG_ON(!name->name);
 #endif
+
 	/*
 	 * If we have a pointer to an audit_names entry already, then we can
 	 * just use it directly if the type is correct.
@@ -1863,7 +1864,17 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
 	}
 
 	list_for_each_entry_reverse(n, &context->names_list, list) {
-		if (!n->name || strcmp(n->name->name, name->name))
+		if (inode->i_ino) {
+			/* valid inode number, use that for the comparison */
+			if (n->ino != inode->i_ino ||
+			    n->dev != inode->i_sb->s_dev)
+				continue;
+		} else if (n->name) {
+			/* inode number has not been set, check the name */
+			if (strcmp(n->name->name, name->name))
+				continue;
+		} else
+			/* no inode and no name (?!) ... this is odd ... */
 			continue;
 
 		/* match the correct record type */
@@ -1931,11 +1942,16 @@ void __audit_inode_child(const struct inode *parent,
 
 	/* look for a parent entry first */
 	list_for_each_entry(n, &context->names_list, list) {
-		if (!n->name || n->type != AUDIT_TYPE_PARENT)
+		if (!n->name ||
+		    (n->type != AUDIT_TYPE_PARENT &&
+		     n->type != AUDIT_TYPE_UNKNOWN))
 			continue;
 
-		if (n->ino == parent->i_ino &&
-		    !audit_compare_dname_path(dname, n->name->name, n->name_len)) {
+		if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev &&
+		    !audit_compare_dname_path(dname,
+					      n->name->name, n->name_len)) {
+			if (n->type == AUDIT_TYPE_UNKNOWN)
+				n->type = AUDIT_TYPE_PARENT;
 			found_parent = n;
 			break;
 		}
@@ -1944,11 +1960,8 @@ void __audit_inode_child(const struct inode *parent,
 	/* is there a matching child entry? */
 	list_for_each_entry(n, &context->names_list, list) {
 		/* can only match entries that have a name */
-		if (!n->name || n->type != type)
-			continue;
-
-		/* if we found a parent, make sure this one is a child of it */
-		if (found_parent && (n->name != found_parent->name))
+		if (!n->name ||
+		    (n->type != type && n->type != AUDIT_TYPE_UNKNOWN))
 			continue;
 
 		if (!strcmp(dname, n->name->name) ||
@@ -1956,6 +1969,8 @@ void __audit_inode_child(const struct inode *parent,
 						found_parent ?
 						found_parent->name_len :
 						AUDIT_NAME_FULL)) {
+			if (n->type == AUDIT_TYPE_UNKNOWN)
+				n->type = type;
 			found_child = n;
 			break;
 		}
@@ -1984,6 +1999,7 @@ void __audit_inode_child(const struct inode *parent,
 			found_child->name_put = false;
 		}
 	}
+
 	if (inode)
 		audit_copy_inode(found_child, dentry, inode);
 	else

Re: [RFC PATCH 4/5] audit: fix filename matching in __audit_inode() and __audit_inode_child()

[Richard Guy Briggs]

On 15/01/08, Paul Moore wrote:
> In all likelihood there were some subtle, and perhaps not so subtle,
> bugs with filename matching in audit_inode() and audit_inode_child()
> for some time, however, recent changes to the audit filename code have
> definitely broken the filename matching code.  The breakage could
> result in duplicate filenames in the audit log and other odd audit
> record entries.  This patch fixes the filename matching code and
> restores some sanity to the filename audit records.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  kernel/auditsc.c |   34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index c967ffc..98edf06 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1846,6 +1846,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
>  	/* The struct filename _must_ have a populated ->name */
>  	BUG_ON(!name->name);
>  #endif
> +
>  	/*
>  	 * If we have a pointer to an audit_names entry already, then we can
>  	 * just use it directly if the type is correct.
> @@ -1863,7 +1864,17 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
>  	}
>  
>  	list_for_each_entry_reverse(n, &context->names_list, list) {
> -		if (!n->name || strcmp(n->name->name, name->name))
> +		if (inode->i_ino) {
> +			/* valid inode number, use that for the comparison */
> +			if (n->ino != inode->i_ino ||
> +			    n->dev != inode->i_sb->s_dev)
> +				continue;
> +		} else if (n->name) {
> +			/* inode number has not been set, check the name */
> +			if (strcmp(n->name->name, name->name))
> +				continue;
> +		} else
> +			/* no inode and no name (?!) ... this is odd ... */
>  			continue;
>  
>  		/* match the correct record type */
> @@ -1931,11 +1942,16 @@ void __audit_inode_child(const struct inode *parent,
>  
>  	/* look for a parent entry first */
>  	list_for_each_entry(n, &context->names_list, list) {
> -		if (!n->name || n->type != AUDIT_TYPE_PARENT)
> +		if (!n->name ||
> +		    (n->type != AUDIT_TYPE_PARENT &&
> +		     n->type != AUDIT_TYPE_UNKNOWN))
>  			continue;
>  
> -		if (n->ino == parent->i_ino &&
> -		    !audit_compare_dname_path(dname, n->name->name, n->name_len)) {
> +		if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev &&
> +		    !audit_compare_dname_path(dname,
> +					      n->name->name, n->name_len)) {
> +			if (n->type == AUDIT_TYPE_UNKNOWN)
> +				n->type = AUDIT_TYPE_PARENT;
>  			found_parent = n;
>  			break;
>  		}
> @@ -1944,11 +1960,8 @@ void __audit_inode_child(const struct inode *parent,
>  	/* is there a matching child entry? */
>  	list_for_each_entry(n, &context->names_list, list) {
>  		/* can only match entries that have a name */
> -		if (!n->name || n->type != type)
> -			continue;
> -
> -		/* if we found a parent, make sure this one is a child of it */
> -		if (found_parent && (n->name != found_parent->name))
> +		if (!n->name ||
> +		    (n->type != type && n->type != AUDIT_TYPE_UNKNOWN))
>  			continue;
>  
>  		if (!strcmp(dname, n->name->name) ||
> @@ -1956,6 +1969,8 @@ void __audit_inode_child(const struct inode *parent,
>  						found_parent ?
>  						found_parent->name_len :
>  						AUDIT_NAME_FULL)) {
> +			if (n->type == AUDIT_TYPE_UNKNOWN)
> +				n->type = type;
>  			found_child = n;
>  			break;
>  		}
> @@ -1984,6 +1999,7 @@ void __audit_inode_child(const struct inode *parent,
>  			found_child->name_put = false;
>  		}
>  	}
> +
>  	if (inode)
>  		audit_copy_inode(found_child, dentry, inode);
>  	else
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[RFC PATCH 5/5] audit: replace getname()/putname() hacks with reference counters

[Paul Moore]

In order to ensure that filenames are not released before the audit
subsystem is done with the strings there are a number of hacks built
into the fs and audit subsystems around getname() and putname().  To
say these hacks are "ugly" would be kind.

This patch removes the filename hackery in favor of a more
conventional reference count based approach.  The diffstat below tells
most of the story; lots of audit/fs specific code is replaced with a
traditional reference count based approach that is easily understood,
even by those not familiar with the audit and/or fs subsystems.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 fs/namei.c            |   29 +++++++-------
 include/linux/audit.h |    3 -
 include/linux/fs.h    |    9 +---
 kernel/audit.h        |   17 +-------
 kernel/auditsc.c      |  101 ++++---------------------------------------------
 5 files changed, 26 insertions(+), 133 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 1c0d4c7..7cbe13a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -117,15 +117,6 @@
  * POSIX.1 2.4: an empty pathname is invalid (ENOENT).
  * PATH_MAX includes the nul terminator --RR.
  */
-void final_putname(struct filename *name)
-{
-	if (name->separate) {
-		__putname(name->name);
-		kfree(name);
-	} else {
-		__putname(name);
-	}
-}
 
 #define EMBEDDED_NAME_MAX	(PATH_MAX - sizeof(struct filename))
 
@@ -144,6 +135,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
 	result = __getname();
 	if (unlikely(!result))
 		return ERR_PTR(-ENOMEM);
+	result->refcnt = 1;
 
 	/*
 	 * First, try to embed the struct filename inside the names_cache
@@ -178,6 +170,7 @@ recopy:
 		}
 		result->name = kname;
 		result->separate = true;
+		result->refcnt = 1;
 		max = PATH_MAX;
 		goto recopy;
 	}
@@ -201,7 +194,7 @@ recopy:
 	return result;
 
 error:
-	final_putname(result);
+	putname(result);
 	return err;
 }
 
@@ -242,19 +235,25 @@ getname_kernel(const char * filename)
 	strlcpy((char *)result->name, filename, len);
 	result->uptr = NULL;
 	result->aname = NULL;
+	result->refcnt = 1;
 	audit_getname(result);
 
 	return result;
 }
 
-#ifdef CONFIG_AUDITSYSCALL
 void putname(struct filename *name)
 {
-	if (unlikely(!audit_dummy_context()))
-		return audit_putname(name);
-	final_putname(name);
+	BUG_ON(name->refcnt <= 0);
+
+	if (--name->refcnt > 0)
+		return;
+
+	if (name->separate) {
+		__putname(name->name);
+		kfree(name);
+	} else
+		__putname(name);
 }
-#endif
 
 static int check_acl(struct inode *inode, int mask)
 {
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9333192..086b2b1 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -128,7 +128,6 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
 extern void __audit_syscall_exit(int ret_success, long ret_value);
 extern struct filename *__audit_reusename(const __user char *uptr);
 extern void __audit_getname(struct filename *name);
-extern void audit_putname(struct filename *name);
 
 #define AUDIT_INODE_PARENT	1	/* dentry represents the parent */
 #define AUDIT_INODE_HIDDEN	2	/* audit record should be hidden */
@@ -347,8 +346,6 @@ static inline struct filename *audit_reusename(const __user char *name)
 }
 static inline void audit_getname(struct filename *name)
 { }
-static inline void audit_putname(struct filename *name)
-{ }
 static inline void __audit_inode(struct filename *name,
 					const struct dentry *dentry,
 					unsigned int flags)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e11d60c..df7a047 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2017,6 +2017,7 @@ struct filename {
 	const char		*name;	/* pointer to actual string */
 	const __user char	*uptr;	/* original userland pointer */
 	struct audit_names	*aname;
+	int			refcnt;
 	bool			separate; /* should "name" be freed? */
 };
 
@@ -2036,6 +2037,7 @@ extern int filp_close(struct file *, fl_owner_t id);
 
 extern struct filename *getname(const char __user *);
 extern struct filename *getname_kernel(const char *);
+extern void putname(struct filename *name);
 
 enum {
 	FILE_CREATED = 1,
@@ -2056,15 +2058,8 @@ extern void __init vfs_caches_init(unsigned long);
 
 extern struct kmem_cache *names_cachep;
 
-extern void final_putname(struct filename *name);
-
 #define __getname()		kmem_cache_alloc(names_cachep, GFP_KERNEL)
 #define __putname(name)		kmem_cache_free(names_cachep, (void *)(name))
-#ifndef CONFIG_AUDITSYSCALL
-#define putname(name)		final_putname(name)
-#else
-extern void putname(struct filename *name);
-#endif
 
 #ifdef CONFIG_BLOCK
 extern int register_blkdev(unsigned int, const char *);
diff --git a/kernel/audit.h b/kernel/audit.h
index 3cdffad..1caa0d3 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -24,12 +24,6 @@
 #include <linux/skbuff.h>
 #include <uapi/linux/mqueue.h>
 
-/* 0 = no checking
-   1 = put_count checking
-   2 = verbose put_count checking
-*/
-#define AUDIT_DEBUG 0
-
 /* AUDIT_NAMES is the number of slots we reserve in the audit_context
  * for saving names from getname().  If we get more names we will allocate
  * a name dynamically and also add those to the list anchored by names_list. */
@@ -74,9 +68,8 @@ struct audit_cap_data {
 	};
 };
 
-/* When fs/namei.c:getname() is called, we store the pointer in name and
- * we don't let putname() free it (instead we free all of the saved
- * pointers at syscall exit time).
+/* When fs/namei.c:getname() is called, we store the pointer in name and bump
+ * the refcnt in the associated filename struct.
  *
  * Further, in fs/namei.c:path_lookup() we store the inode and device.
  */
@@ -86,7 +79,6 @@ struct audit_names {
 	struct filename		*name;
 	int			name_len;	/* number of chars to log */
 	bool			hidden;		/* don't log this record */
-	bool			name_put;	/* call __putname()? */
 
 	unsigned long		ino;
 	dev_t			dev;
@@ -208,11 +200,6 @@ struct audit_context {
 	};
 	int fds[2];
 	struct audit_proctitle proctitle;
-
-#if AUDIT_DEBUG
-	int		    put_count;
-	int		    ino_count;
-#endif
 };
 
 extern u32 audit_ever_enabled;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 98edf06..f743cda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -866,33 +866,10 @@ static inline void audit_free_names(struct audit_context *context)
 {
 	struct audit_names *n, *next;
 
-#if AUDIT_DEBUG == 2
-	if (context->put_count + context->ino_count != context->name_count) {
-		int i = 0;
-
-		pr_err("%s:%d(:%d): major=%d in_syscall=%d"
-		       " name_count=%d put_count=%d ino_count=%d"
-		       " [NOT freeing]\n", __FILE__, __LINE__,
-		       context->serial, context->major, context->in_syscall,
-		       context->name_count, context->put_count,
-		       context->ino_count);
-		list_for_each_entry(n, &context->names_list, list) {
-			pr_err("names[%d] = %p = %s\n", i++, n->name,
-			       n->name->name ?: "(null)");
-		}
-		dump_stack();
-		return;
-	}
-#endif
-#if AUDIT_DEBUG
-	context->put_count  = 0;
-	context->ino_count  = 0;
-#endif
-
 	list_for_each_entry_safe(n, next, &context->names_list, list) {
 		list_del(&n->list);
-		if (n->name && n->name_put)
-			final_putname(n->name);
+		if (n->name)
+			putname(n->name);
 		if (n->should_free)
 			kfree(n);
 	}
@@ -1711,9 +1688,6 @@ static struct audit_names *audit_alloc_name(struct audit_context *context,
 	list_add_tail(&aname->list, &context->names_list);
 
 	context->name_count++;
-#if AUDIT_DEBUG
-	context->ino_count++;
-#endif
 	return aname;
 }
 
@@ -1752,19 +1726,8 @@ void __audit_getname(struct filename *name)
 	struct audit_context *context = current->audit_context;
 	struct audit_names *n;
 
-	if (!context->in_syscall) {
-#if AUDIT_DEBUG == 2
-		pr_err("%s:%d(:%d): ignoring getname(%p)\n",
-		       __FILE__, __LINE__, context->serial, name);
-		dump_stack();
-#endif
+	if (!context->in_syscall)
 		return;
-	}
-
-#if AUDIT_DEBUG
-	/* The filename _must_ have a populated ->name */
-	BUG_ON(!name->name);
-#endif
 
 	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
 	if (!n)
@@ -1772,56 +1735,13 @@ void __audit_getname(struct filename *name)
 
 	n->name = name;
 	n->name_len = AUDIT_NAME_FULL;
-	n->name_put = true;
 	name->aname = n;
+	name->refcnt++;
 
 	if (!context->pwd.dentry)
 		get_fs_pwd(current->fs, &context->pwd);
 }
 
-/* audit_putname - intercept a putname request
- * @name: name to intercept and delay for putname
- *
- * If we have stored the name from getname in the audit context,
- * then we delay the putname until syscall exit.
- * Called from include/linux/fs.h:putname().
- */
-void audit_putname(struct filename *name)
-{
-	struct audit_context *context = current->audit_context;
-
-	BUG_ON(!context);
-	if (!name->aname || !context->in_syscall) {
-#if AUDIT_DEBUG == 2
-		pr_err("%s:%d(:%d): final_putname(%p)\n",
-		       __FILE__, __LINE__, context->serial, name);
-		if (context->name_count) {
-			struct audit_names *n;
-			int i = 0;
-
-			list_for_each_entry(n, &context->names_list, list)
-				pr_err("name[%d] = %p = %s\n", i++, n->name,
-				       n->name->name ?: "(null)");
-			}
-#endif
-		final_putname(name);
-	}
-#if AUDIT_DEBUG
-	else {
-		++context->put_count;
-		if (context->put_count > context->name_count) {
-			pr_err("%s:%d(:%d): major=%d in_syscall=%d putname(%p)"
-			       " name_count=%d put_count=%d\n",
-			       __FILE__, __LINE__,
-			       context->serial, context->major,
-			       context->in_syscall, name->name,
-			       context->name_count, context->put_count);
-			dump_stack();
-		}
-	}
-#endif
-}
-
 /**
  * __audit_inode - store the inode and device from a lookup
  * @name: name being audited
@@ -1842,11 +1762,6 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
 	if (!name)
 		goto out_alloc;
 
-#if AUDIT_DEBUG
-	/* The struct filename _must_ have a populated ->name */
-	BUG_ON(!name->name);
-#endif
-
 	/*
 	 * If we have a pointer to an audit_names entry already, then we can
 	 * just use it directly if the type is correct.
@@ -1893,9 +1808,10 @@ out_alloc:
 	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
 	if (!n)
 		return;
-	if (name)
-		/* no need to set ->name_put as the original will cleanup */
+	if (name) {
 		n->name = name;
+		name->refcnt++;
+	}
 
 out:
 	if (parent) {
@@ -1995,8 +1911,7 @@ void __audit_inode_child(const struct inode *parent,
 		if (found_parent) {
 			found_child->name = found_parent->name;
 			found_child->name_len = AUDIT_NAME_FULL;
-			/* don't call __putname() */
-			found_child->name_put = false;
+			found_child->name->refcnt++;
 		}
 	}
 

Re: [RFC PATCH 5/5] audit: replace getname()/putname() hacks with reference counters

[Richard Guy Briggs]

On 15/01/08, Paul Moore wrote:
> In order to ensure that filenames are not released before the audit
> subsystem is done with the strings there are a number of hacks built
> into the fs and audit subsystems around getname() and putname().  To
> say these hacks are "ugly" would be kind.
> 
> This patch removes the filename hackery in favor of a more
> conventional reference count based approach.  The diffstat below tells
> most of the story; lots of audit/fs specific code is replaced with a
> traditional reference count based approach that is easily understood,
> even by those not familiar with the audit and/or fs subsystems.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

The only nit I've got is "refcnt" enlarges "struct filename" where I
would have used a bitfield with "separate".

Otherwise, this looks like an improvement.  Thanks.

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  fs/namei.c            |   29 +++++++-------
>  include/linux/audit.h |    3 -
>  include/linux/fs.h    |    9 +---
>  kernel/audit.h        |   17 +-------
>  kernel/auditsc.c      |  101 ++++---------------------------------------------
>  5 files changed, 26 insertions(+), 133 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 1c0d4c7..7cbe13a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -117,15 +117,6 @@
>   * POSIX.1 2.4: an empty pathname is invalid (ENOENT).
>   * PATH_MAX includes the nul terminator --RR.
>   */
> -void final_putname(struct filename *name)
> -{
> -	if (name->separate) {
> -		__putname(name->name);
> -		kfree(name);
> -	} else {
> -		__putname(name);
> -	}
> -}
>  
>  #define EMBEDDED_NAME_MAX	(PATH_MAX - sizeof(struct filename))
>  
> @@ -144,6 +135,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
>  	result = __getname();
>  	if (unlikely(!result))
>  		return ERR_PTR(-ENOMEM);
> +	result->refcnt = 1;
>  
>  	/*
>  	 * First, try to embed the struct filename inside the names_cache
> @@ -178,6 +170,7 @@ recopy:
>  		}
>  		result->name = kname;
>  		result->separate = true;
> +		result->refcnt = 1;
>  		max = PATH_MAX;
>  		goto recopy;
>  	}
> @@ -201,7 +194,7 @@ recopy:
>  	return result;
>  
>  error:
> -	final_putname(result);
> +	putname(result);
>  	return err;
>  }
>  
> @@ -242,19 +235,25 @@ getname_kernel(const char * filename)
>  	strlcpy((char *)result->name, filename, len);
>  	result->uptr = NULL;
>  	result->aname = NULL;
> +	result->refcnt = 1;
>  	audit_getname(result);
>  
>  	return result;
>  }
>  
> -#ifdef CONFIG_AUDITSYSCALL
>  void putname(struct filename *name)
>  {
> -	if (unlikely(!audit_dummy_context()))
> -		return audit_putname(name);
> -	final_putname(name);
> +	BUG_ON(name->refcnt <= 0);
> +
> +	if (--name->refcnt > 0)
> +		return;
> +
> +	if (name->separate) {
> +		__putname(name->name);
> +		kfree(name);
> +	} else
> +		__putname(name);
>  }
> -#endif
>  
>  static int check_acl(struct inode *inode, int mask)
>  {
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9333192..086b2b1 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -128,7 +128,6 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>  extern void __audit_syscall_exit(int ret_success, long ret_value);
>  extern struct filename *__audit_reusename(const __user char *uptr);
>  extern void __audit_getname(struct filename *name);
> -extern void audit_putname(struct filename *name);
>  
>  #define AUDIT_INODE_PARENT	1	/* dentry represents the parent */
>  #define AUDIT_INODE_HIDDEN	2	/* audit record should be hidden */
> @@ -347,8 +346,6 @@ static inline struct filename *audit_reusename(const __user char *name)
>  }
>  static inline void audit_getname(struct filename *name)
>  { }
> -static inline void audit_putname(struct filename *name)
> -{ }
>  static inline void __audit_inode(struct filename *name,
>  					const struct dentry *dentry,
>  					unsigned int flags)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e11d60c..df7a047 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2017,6 +2017,7 @@ struct filename {
>  	const char		*name;	/* pointer to actual string */
>  	const __user char	*uptr;	/* original userland pointer */
>  	struct audit_names	*aname;
> +	int			refcnt;
>  	bool			separate; /* should "name" be freed? */
>  };
>  
> @@ -2036,6 +2037,7 @@ extern int filp_close(struct file *, fl_owner_t id);
>  
>  extern struct filename *getname(const char __user *);
>  extern struct filename *getname_kernel(const char *);
> +extern void putname(struct filename *name);
>  
>  enum {
>  	FILE_CREATED = 1,
> @@ -2056,15 +2058,8 @@ extern void __init vfs_caches_init(unsigned long);
>  
>  extern struct kmem_cache *names_cachep;
>  
> -extern void final_putname(struct filename *name);
> -
>  #define __getname()		kmem_cache_alloc(names_cachep, GFP_KERNEL)
>  #define __putname(name)		kmem_cache_free(names_cachep, (void *)(name))
> -#ifndef CONFIG_AUDITSYSCALL
> -#define putname(name)		final_putname(name)
> -#else
> -extern void putname(struct filename *name);
> -#endif
>  
>  #ifdef CONFIG_BLOCK
>  extern int register_blkdev(unsigned int, const char *);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 3cdffad..1caa0d3 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -24,12 +24,6 @@
>  #include <linux/skbuff.h>
>  #include <uapi/linux/mqueue.h>
>  
> -/* 0 = no checking
> -   1 = put_count checking
> -   2 = verbose put_count checking
> -*/
> -#define AUDIT_DEBUG 0
> -
>  /* AUDIT_NAMES is the number of slots we reserve in the audit_context
>   * for saving names from getname().  If we get more names we will allocate
>   * a name dynamically and also add those to the list anchored by names_list. */
> @@ -74,9 +68,8 @@ struct audit_cap_data {
>  	};
>  };
>  
> -/* When fs/namei.c:getname() is called, we store the pointer in name and
> - * we don't let putname() free it (instead we free all of the saved
> - * pointers at syscall exit time).
> +/* When fs/namei.c:getname() is called, we store the pointer in name and bump
> + * the refcnt in the associated filename struct.
>   *
>   * Further, in fs/namei.c:path_lookup() we store the inode and device.
>   */
> @@ -86,7 +79,6 @@ struct audit_names {
>  	struct filename		*name;
>  	int			name_len;	/* number of chars to log */
>  	bool			hidden;		/* don't log this record */
> -	bool			name_put;	/* call __putname()? */
>  
>  	unsigned long		ino;
>  	dev_t			dev;
> @@ -208,11 +200,6 @@ struct audit_context {
>  	};
>  	int fds[2];
>  	struct audit_proctitle proctitle;
> -
> -#if AUDIT_DEBUG
> -	int		    put_count;
> -	int		    ino_count;
> -#endif
>  };
>  
>  extern u32 audit_ever_enabled;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 98edf06..f743cda 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -866,33 +866,10 @@ static inline void audit_free_names(struct audit_context *context)
>  {
>  	struct audit_names *n, *next;
>  
> -#if AUDIT_DEBUG == 2
> -	if (context->put_count + context->ino_count != context->name_count) {
> -		int i = 0;
> -
> -		pr_err("%s:%d(:%d): major=%d in_syscall=%d"
> -		       " name_count=%d put_count=%d ino_count=%d"
> -		       " [NOT freeing]\n", __FILE__, __LINE__,
> -		       context->serial, context->major, context->in_syscall,
> -		       context->name_count, context->put_count,
> -		       context->ino_count);
> -		list_for_each_entry(n, &context->names_list, list) {
> -			pr_err("names[%d] = %p = %s\n", i++, n->name,
> -			       n->name->name ?: "(null)");
> -		}
> -		dump_stack();
> -		return;
> -	}
> -#endif
> -#if AUDIT_DEBUG
> -	context->put_count  = 0;
> -	context->ino_count  = 0;
> -#endif
> -
>  	list_for_each_entry_safe(n, next, &context->names_list, list) {
>  		list_del(&n->list);
> -		if (n->name && n->name_put)
> -			final_putname(n->name);
> +		if (n->name)
> +			putname(n->name);
>  		if (n->should_free)
>  			kfree(n);
>  	}
> @@ -1711,9 +1688,6 @@ static struct audit_names *audit_alloc_name(struct audit_context *context,
>  	list_add_tail(&aname->list, &context->names_list);
>  
>  	context->name_count++;
> -#if AUDIT_DEBUG
> -	context->ino_count++;
> -#endif
>  	return aname;
>  }
>  
> @@ -1752,19 +1726,8 @@ void __audit_getname(struct filename *name)
>  	struct audit_context *context = current->audit_context;
>  	struct audit_names *n;
>  
> -	if (!context->in_syscall) {
> -#if AUDIT_DEBUG == 2
> -		pr_err("%s:%d(:%d): ignoring getname(%p)\n",
> -		       __FILE__, __LINE__, context->serial, name);
> -		dump_stack();
> -#endif
> +	if (!context->in_syscall)
>  		return;
> -	}
> -
> -#if AUDIT_DEBUG
> -	/* The filename _must_ have a populated ->name */
> -	BUG_ON(!name->name);
> -#endif
>  
>  	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
>  	if (!n)
> @@ -1772,56 +1735,13 @@ void __audit_getname(struct filename *name)
>  
>  	n->name = name;
>  	n->name_len = AUDIT_NAME_FULL;
> -	n->name_put = true;
>  	name->aname = n;
> +	name->refcnt++;
>  
>  	if (!context->pwd.dentry)
>  		get_fs_pwd(current->fs, &context->pwd);
>  }
>  
> -/* audit_putname - intercept a putname request
> - * @name: name to intercept and delay for putname
> - *
> - * If we have stored the name from getname in the audit context,
> - * then we delay the putname until syscall exit.
> - * Called from include/linux/fs.h:putname().
> - */
> -void audit_putname(struct filename *name)
> -{
> -	struct audit_context *context = current->audit_context;
> -
> -	BUG_ON(!context);
> -	if (!name->aname || !context->in_syscall) {
> -#if AUDIT_DEBUG == 2
> -		pr_err("%s:%d(:%d): final_putname(%p)\n",
> -		       __FILE__, __LINE__, context->serial, name);
> -		if (context->name_count) {
> -			struct audit_names *n;
> -			int i = 0;
> -
> -			list_for_each_entry(n, &context->names_list, list)
> -				pr_err("name[%d] = %p = %s\n", i++, n->name,
> -				       n->name->name ?: "(null)");
> -			}
> -#endif
> -		final_putname(name);
> -	}
> -#if AUDIT_DEBUG
> -	else {
> -		++context->put_count;
> -		if (context->put_count > context->name_count) {
> -			pr_err("%s:%d(:%d): major=%d in_syscall=%d putname(%p)"
> -			       " name_count=%d put_count=%d\n",
> -			       __FILE__, __LINE__,
> -			       context->serial, context->major,
> -			       context->in_syscall, name->name,
> -			       context->name_count, context->put_count);
> -			dump_stack();
> -		}
> -	}
> -#endif
> -}
> -
>  /**
>   * __audit_inode - store the inode and device from a lookup
>   * @name: name being audited
> @@ -1842,11 +1762,6 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
>  	if (!name)
>  		goto out_alloc;
>  
> -#if AUDIT_DEBUG
> -	/* The struct filename _must_ have a populated ->name */
> -	BUG_ON(!name->name);
> -#endif
> -
>  	/*
>  	 * If we have a pointer to an audit_names entry already, then we can
>  	 * just use it directly if the type is correct.
> @@ -1893,9 +1808,10 @@ out_alloc:
>  	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
>  	if (!n)
>  		return;
> -	if (name)
> -		/* no need to set ->name_put as the original will cleanup */
> +	if (name) {
>  		n->name = name;
> +		name->refcnt++;
> +	}
>  
>  out:
>  	if (parent) {
> @@ -1995,8 +1911,7 @@ void __audit_inode_child(const struct inode *parent,
>  		if (found_parent) {
>  			found_child->name = found_parent->name;
>  			found_child->name_len = AUDIT_NAME_FULL;
> -			/* don't call __putname() */
> -			found_child->name_put = false;
> +			found_child->name->refcnt++;
>  		}
>  	}
>  
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [RFC PATCH 5/5] audit: replace getname()/putname() hacks with reference counters

[Paul Moore]

On Wednesday, January 14, 2015 04:37:17 PM Richard Guy Briggs wrote:
> On 15/01/08, Paul Moore wrote:
> > In order to ensure that filenames are not released before the audit
> > subsystem is done with the strings there are a number of hacks built
> > into the fs and audit subsystems around getname() and putname().  To
> > say these hacks are "ugly" would be kind.
> > 
> > This patch removes the filename hackery in favor of a more
> > conventional reference count based approach.  The diffstat below tells
> > most of the story; lots of audit/fs specific code is replaced with a
> > traditional reference count based approach that is easily understood,
> > even by those not familiar with the audit and/or fs subsystems.
> > 
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> 
> The only nit I've got is "refcnt" enlarges "struct filename" where I
> would have used a bitfield with "separate".
> 
> Otherwise, this looks like an improvement.  Thanks.

I agree that it is unfortunate that struct filename increases, but it seemed 
liked a valid tradeoff considering that we got to remove the 
getname()/putname() hacks in favor of a more traditional approach.

As far the int versus bitfield, I suppose I favor the int in this particular 
case, but if the fs folks want a bitfield I can do that.

> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

Thanks for taking the time to review the patchset.

-- 
paul moore
security @ redhat

Re: [RFC PATCH 0/5] Overhaul the audit filename handling

[Paul Moore]

On Thursday, January 08, 2015 11:50:23 AM Paul Moore wrote:
> There have been some patches added to v3.19-rcX to fix various
> problems in the way audit handles filenames but they have been hacks
> on top of hacks, not really something we want long term.  This
> patchset reworks the way audit handles filenames, removing a lot of
> nasty hacks added recently, and fixing a few bugs that still remain.
> 
> Most significant to folks outside of audit, patch 5/5 does away with
> the nasty getname()/putname() kludge in favor of a less ugly reference
> count approach.
> 
> Comments and feedback are welcome.  If nothing ugly pops up on review
> I'll see if Linus is interested in taking this for the next v3.19-rcX
> release, otherwise I'll toss it into linux-next for v3.20.
> 
> -Paul
> 
> ---
> 
> Paul Moore (5):
>       fs: rework getname_kernel to handle up to PATH_MAX sized filenames
>       fs: create proper filename objects using getname_kernel()
>       audit: enable filename recording via getname_kernel()
>       audit: fix filename matching in __audit_inode() and
>              __audit_inode_child()
>       audit: replace getname()/putname() hacks with reference counters
> 
> 
>  fs/exec.c             |   11 +++
>  fs/namei.c            |   98 ++++++++++++++++++-----------
>  fs/open.c             |   11 +++
>  include/linux/audit.h |    3 -
>  include/linux/fs.h    |    9 +--
>  kernel/audit.h        |   17 +----
>  kernel/auditsc.c      |  167 ++++++++-------------------------------------
>  7 files changed, 115 insertions(+), 201 deletions(-)

Al/fs dev: any chance you guys can take a quick look at the fs relevant 
portions of this patchset and give it a ACK/NACK?  I'll carry the patchset in 
the audit tree, but I'd like to see a thumbs up from someone in fs land before 
I merge these patches.

Thanks,
-Paul

-- 
paul moore
security @ redhat