| Message ID | 1430456532-16068-1-git-send-email-chris.j.arges@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 01, 2015 at 12:02:12AM -0500, Chris J Arges wrote: > From: Paul Moore <pmoore@redhat.com> > > BugLink: http://bugs.launchpad.net/bugs/1450442 > > 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> > (cherry picked from commit fcf22d8267ad2601fe9b6c549d1be96401c23e0b) > Signed-off-by: Chris J Arges <chris.j.arges@canonical.com> > --- > kernel/auditsc.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 40 insertions(+), 9 deletions(-) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ff99c05..78f71cd 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -71,6 +71,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" > > @@ -1870,8 +1872,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry, > } > > 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 */ > @@ -1890,14 +1891,44 @@ out_alloc: > 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; > -- > 1.9.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Applied to Utopic master-next.
diff --git a/kernel/auditsc.c b/kernel/auditsc.c index ff99c05..78f71cd 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -71,6 +71,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" @@ -1870,8 +1872,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry, } 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 */ @@ -1890,14 +1891,44 @@ out_alloc: 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;