audit: create private file name copies when auditing inodes

Unfortunately, while commit 4a928436 ("audit: correctly record file names with different path name types") fixed a problem where we were not recording filenames, it created a new problem by attempting to use these file names after they had been freed. This patch resolves the issue by creating a copy of the filename which the audit subsystem frees after it is done with the string. At some point it would be nice to resolve this issue with refcounts, or something similar, instead of having to allocate/copy strings, but that is almost surely beyond the scope of a -rcX patch so we'll defer that for later. On the plus side, only audit users should be impacted by the string copying. Reported-by: Toralf Foerster <toralf.foerster@gmx.de> Signed-off-by: Paul Moore <pmoore@redhat.com>

audit: create private file name copies when auditing inodes
Unfortunately, while commit 4a928436 ("audit: correctly record file names with different path name types") fixed a problem where we were not recording filenames, it created a new problem by attempting to use these file names after they had been freed. This patch resolves the issue by creating a copy of the filename which the audit subsystem frees after it is done with the string. At some point it would be nice to resolve this issue with refcounts, or something similar, instead of having to allocate/copy strings, but that is almost surely beyond the scope of a -rcX patch so we'll defer that for later. On the plus side, only audit users should be impacted by the string copying. Reported-by: Toralf Foerster <toralf.foerster@gmx.de> Signed-off-by: Paul Moore <pmoore@redhat.com>
Paul Moore
1 parent 041d7b98ff
Showing 1 changed file with 40 additions and 9 deletions Side-by-side Diff
kernel/auditsc.c
@@ -72,6 +72,8 @@
 #include <linux/fs_struct.h>
 #include <linux/compat.h>
 #include <linux/ctype.h>
+#include <linux/string.h>
+#include <uapi/linux/limits.h>
  
 #include "audit.h"
  
@@ -1861,8 +1863,7 @@
 	}
  
 	list_for_each_entry_reverse(n, &context->names_list, list) {
-		/* does the name pointer match? */
-		if (!n->name || n->name->name != name->name)
+		if (!n->name || strcmp(n->name->name, name->name))
 			continue;
  
 		/* match the correct record type */
  
@@ -1881,14 +1882,44 @@
 	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
 	if (!n)
 		return;
-	if (name)
-		/* since name is not NULL we know there is already a matching
-		 * name record, see audit_getname(), so there must be a type
-		 * mismatch; reuse the string path since the original name
-		 * record will keep the string valid until we free it in
-		 * audit_free_names() */
-		n->name = name;
+	/* 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 (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;
...	...	@@ -72,6 +72,8 @@
72	72	#include <linux/fs_struct.h>
73	73	#include <linux/compat.h>
74	74	#include <linux/ctype.h>
	75	+#include <linux/string.h>
	76	+#include <uapi/linux/limits.h>
75	77
76	78	#include "audit.h"
77	79
...	...	@@ -1861,8 +1863,7 @@
1861	1863	}
1862	1864
1863	1865	list_for_each_entry_reverse(n, &context->names_list, list) {
1864		- /* does the name pointer match? */
1865		- if (!n->name \|\| n->name->name != name->name)
	1866	+ if (!n->name \|\| strcmp(n->name->name, name->name))
1866	1867	continue;
1867	1868
1868	1869	/* match the correct record type */
1869	1870
...	...	@@ -1881,14 +1882,44 @@
1881	1882	n = audit_alloc_name(context, AUDIT_TYPE_UNKNOWN);
1882	1883	if (!n)
1883	1884	return;
1884		- if (name)
1885		- /* since name is not NULL we know there is already a matching
1886		- * name record, see audit_getname(), so there must be a type
1887		- * mismatch; reuse the string path since the original name
1888		- * record will keep the string valid until we free it in
1889		- * audit_free_names() */
1890		- n->name = name;
	1885	+ /* unfortunately, while we may have a path name to record with the
	1886	+ * inode, we can't always rely on the string lasting until the end of
	1887	+ * the syscall so we need to create our own copy, it may fail due to
	1888	+ * memory allocation issues, but we do our best */
	1889	+ if (name) {
	1890	+ /* we can't use getname_kernel() due to size limits */
	1891	+ size_t len = strlen(name->name) + 1;
	1892	+ struct filename *new = __getname();
1891	1893
	1894	+ if (unlikely(!new))
	1895	+ goto out;
	1896	+
	1897	+ if (len <= (PATH_MAX - sizeof(*new))) {
	1898	+ new->name = (char )(new) + sizeof(new);
	1899	+ new->separate = false;
	1900	+ } else if (len <= PATH_MAX) {
	1901	+ /* this looks odd, but is due to final_putname() */
	1902	+ struct filename *new2;
	1903	+
	1904	+ new2 = kmalloc(sizeof(*new2), GFP_KERNEL);
	1905	+ if (unlikely(!new2)) {
	1906	+ __putname(new);
	1907	+ goto out;
	1908	+ }
	1909	+ new2->name = (char *)new;
	1910	+ new2->separate = true;
	1911	+ new = new2;
	1912	+ } else {
	1913	+ /* we should never get here, but let's be safe */
	1914	+ __putname(new);
	1915	+ goto out;
	1916	+ }
	1917	+ strlcpy((char *)new->name, name->name, len);
	1918	+ new->uptr = NULL;
	1919	+ new->aname = n;
	1920	+ n->name = new;
	1921	+ n->name_put = true;
	1922	+ }
1892	1923	out:
1893	1924	if (parent) {
1894	1925	n->name_len = n->name ? parent_len(n->name->name) : AUDIT_NAME_FULL;