On 2018-07-11 14:39, Ronny Chevalier wrote: > audit_add_watch stores locally krule->watch without taking a reference > on watch. Then, it calls audit_add_to_parent, and uses the watch stored > locally. > > Unfortunately, it is possible that audit_add_to_parent updates > krule->watch. > When it happens, it also drops a reference of watch which > could free the watch. > > How to reproduce (with KASAN enabled): > > auditctl -w /etc/passwd -F success=0 -k test_passwd > auditctl -w /etc/passwd -F success=1 -k test_passwd2 > > The second call to auditctl triggers the use-after-free, because > audit_to_parent updates krule->watch to use a previous existing watch > and drops the reference to the newly created watch. > > To fix the issue, we grab a reference of watch and we release it at the > end of the function. > > Signed-off-by: Ronny Chevalier <[email protected]> > ---
Ronnie, This looks good to me, but let's see how Jan Kara's patches interplay with it. Reviewed-by: Richard Guy Briggs <[email protected]> > v3: > - Move audit_get_watch before audit_get_nd since it is using it > and call audit_put_watch if it fails. > v2: > - Move audit_get_watch before audit_find_parent. In the case of > audit_get_nd failing. > --- > kernel/audit_watch.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c > index 6f249bdf2d84..787c7afdf829 100644 > --- a/kernel/audit_watch.c > +++ b/kernel/audit_watch.c > @@ -420,6 +420,13 @@ int audit_add_watch(struct audit_krule *krule, struct > list_head **list) > struct path parent_path; > int h, ret = 0; > > + /* > + * When we will be calling audit_add_to_parent, krule->watch might have > + * been updated and watch might have been freed. > + * So we need to keep a reference of watch. > + */ > + audit_get_watch(watch); > + > mutex_unlock(&audit_filter_mutex); > > /* Avoid calling path_lookup under audit_filter_mutex. */ > @@ -428,8 +435,10 @@ int audit_add_watch(struct audit_krule *krule, struct > list_head **list) > /* caller expects mutex locked */ > mutex_lock(&audit_filter_mutex); > > - if (ret) > + if (ret) { > + audit_put_watch(watch); > return ret; > + } > > /* either find an old parent or attach a new one */ > parent = audit_find_parent(d_backing_inode(parent_path.dentry)); > @@ -447,6 +456,7 @@ int audit_add_watch(struct audit_krule *krule, struct > list_head **list) > *list = &audit_inode_hash[h]; > error: > path_put(&parent_path); > + audit_put_watch(watch); > return ret; > } > > -- > 2.18.0 > - 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 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
