On Wed, Apr 22, 2020 at 9:23 AM Richard Guy Briggs <[email protected]> wrote: > On 2020-04-22 14:28, Yiwen Gu wrote: > > Currently, audit_reusename check file path only by comparing > > userspace pointer "uptr", without checking the situation where > > the file name is different with the same uptr. > > Has this been found to actually be a problem? If so, is there a > reproducer to demonstrate this? > > The reason it was originally set up like this was to make the check as > efficient as possible, comparing only a pointer, rather than needing to > compare the entire string.
Let's also not forget that we don't actually use the uptr's dereferenced value for PATH reporting, for obvious reasons (it's owned and subject to manipulation by userspace). We use the uptr pointer value for tracking only, which is okay. audit_reusename() is a shortcut to get a previously populated filename struct associated with the filename buffer passed into the kernel by userspace. It doesn't matter that the buffer contents may have changed, we safely copy the buffer contents into the kernel (via the filename struct) before use, what does matter is that we can associate the filename struct with the uptr ... which we do in audit_reusename, audit_getname(), etc. > > Add kname into audit_reusename function to check file names > > from the audit_names list. > > > > Signed-off-by: Yiwen Gu <[email protected]> > > --- > > fs/namei.c | 9 +++++---- > > include/linux/audit.h | 11 +++++++---- > > kernel/auditsc.c | 7 ++++--- > > 3 files changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index db6565c99825..d5fb4bd12aec 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -128,13 +128,10 @@ struct filename * > > getname_flags(const char __user *filename, int flags, int *empty) > > { > > struct filename *result; > > + struct filename *result_audit; > > char *kname; > > int len; > > > > - result = audit_reusename(filename); > > - if (result) > > - return result; > > - > > result = __getname(); > > if (unlikely(!result)) > > return ERR_PTR(-ENOMEM); > > @@ -197,6 +194,10 @@ getname_flags(const char __user *filename, int flags, > > int *empty) > > } > > } > > > > + result_audit = audit_reusename(filename, kname); > > + if (result_audit) > > + return result_audit; > > + > > result->uptr = filename; > > result->aname = NULL; > > audit_getname(result); > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index f9ceae57ca8d..71fb783f14c4 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -266,7 +266,8 @@ extern void __audit_free(struct task_struct *task); > > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned > > long a1, > > unsigned long a2, unsigned long a3); > > extern void __audit_syscall_exit(int ret_success, long ret_value); > > -extern struct filename *__audit_reusename(const __user char *uptr); > > +extern struct filename *__audit_reusename(const __user char *uptr, > > + const char *kname); > > extern void __audit_getname(struct filename *name); > > > > extern void __audit_inode(struct filename *name, const struct dentry > > *dentry, > > @@ -316,10 +317,11 @@ static inline void audit_syscall_exit(void *pt_regs) > > __audit_syscall_exit(success, return_code); > > } > > } > > -static inline struct filename *audit_reusename(const __user char *name) > > +static inline struct filename *audit_reusename(const __user char *name, > > + const char *kname) > > { > > if (unlikely(!audit_dummy_context())) > > - return __audit_reusename(name); > > + return __audit_reusename(name, kname); > > return NULL; > > } > > static inline void audit_getname(struct filename *name) > > @@ -539,7 +541,8 @@ static inline struct audit_context *audit_context(void) > > { > > return NULL; > > } > > -static inline struct filename *audit_reusename(const __user char *name) > > +static inline struct filename *audit_reusename(const __user char *name, > > + const char *kname) > > { > > return NULL; > > } > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 4effe01ebbe2..62ffc02abb98 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -1843,13 +1843,14 @@ static struct audit_names *audit_alloc_name(struct > > audit_context *context, > > /** > > * __audit_reusename - fill out filename with info from existing entry > > * @uptr: userland ptr to pathname > > + * @kname: kernel pathname string > > * > > * Search the audit_names list for the current audit context. If there is > > an > > - * existing entry with a matching "uptr" then return the filename > > + * existing entry with matching "uptr" and "kname" then return the filename > > * associated with that audit_name. If not, return NULL. > > */ > > struct filename * > > -__audit_reusename(const __user char *uptr) > > +__audit_reusename(const __user char *uptr, const char *kname) > > { > > struct audit_context *context = audit_context(); > > struct audit_names *n; > > @@ -1857,7 +1858,7 @@ __audit_reusename(const __user char *uptr) > > list_for_each_entry(n, &context->names_list, list) { > > if (!n->name) > > continue; > > - if (n->name->uptr == uptr) { > > + if (n->name->uptr == uptr && !strcmp(kname, n->name->name)) { > > n->name->refcnt++; > > return n->name; > > } > > -- > > 2.17.1 > > > > > > > > -- > > Linux-audit mailing list > > [email protected] > > https://www.redhat.com/mailman/listinfo/linux-audit > > - RGB > > -- > Richard Guy Briggs <[email protected]> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 > -- paul moore www.paul-moore.com -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
