[PATCH 0/2] audit: Fix two bugs with deleting audit watches

[PATCH 0/2] audit: Fix two bugs with deleting audit watches

[Jan Kara]

Hello,

Tony pointed me to a long-standing use after free issue in audit watch
subsystem. The first patch should fix this. When analyzing and fixing
that issue, I've noticed that audit watch does not received FS_UNMOUNT
event although from the code it seems it expects to receive it - patch
two fixes that. The patches survive some basic testing but especially
for the second patch I'd like someone with more experience with audit
subsystem to have a look whether the patch makes sense.

								Honza

[PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()

[Jan Kara]

audit_remove_watch_rule() drops watch's reference to parent but then
continues to work with it. That is not safe as parent can get freed once
we drop our reference. The following is a trivial reproducer:

mount -o loop image /mnt
touch /mnt/file
auditctl -w /mnt/file -p wax
umount /mnt
auditctl -D
<crash in fsnotify_destroy_mark()>

Grab our own reference in audit_remove_watch_rule() earlier to make sure
mark does not get freed under us.

CC: stable@vger.kernel.org
Reported-by: Tony Jones <tonyj@suse.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/audit_watch.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 62d686d96581..ed748ee40029 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
 	list_del(&krule->rlist);
 
 	if (list_empty(&watch->rules)) {
+		/*
+		 * audit_remove_watch() drops our reference to 'parent' which
+		 * can get freed. Grab our own reference to be safe.
+		 */
+		audit_get_parent(parent);
 		audit_remove_watch(watch);
-
-		if (list_empty(&parent->watches)) {
-			audit_get_parent(parent);
+		if (list_empty(&parent->watches))
 			fsnotify_destroy_mark(&parent->mark, audit_watch_group);
-			audit_put_parent(parent);
-		}
+		audit_put_parent(parent);
 	}
 }
 
-- 
2.12.3

[PATCH 2/2] audit: Receive unmount event

[Jan Kara]

Although audit_watch_handle_event() can handle FS_UNMOUNT event, it is
not part of AUDIT_FS_WATCH mask and thus such event never gets to
audit_watch_handle_event(). Thus fsnotify marks are deleted by fsnotify
subsystem on unmount without audit being notified about that which leads
to a strange state of existing audit rules with dead fsnotify marks.

Add FS_UNMOUNT to the mask of events to be received so that audit can
clean up its state accordingly.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/audit_watch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index ed748ee40029..9eb8b3511636 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -66,7 +66,7 @@ static struct fsnotify_group *audit_watch_group;
 
 /* fsnotify events we care about. */
 #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
-			FS_MOVE_SELF | FS_EVENT_ON_CHILD)
+			FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
 
 static void audit_free_parent(struct audit_parent *parent)
 {
-- 
2.12.3

Re: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()

[Tony Jones]

On 08/15/2017 04:00 AM, Jan Kara wrote:
> audit_remove_watch_rule() drops watch's reference to parent but then
> continues to work with it. That is not safe as parent can get freed once
> we drop our reference. The following is a trivial reproducer:
> 
> mount -o loop image /mnt
> touch /mnt/file
> auditctl -w /mnt/file -p wax
> umount /mnt
> auditctl -D
> <crash in fsnotify_destroy_mark()>
> 
> Grab our own reference in audit_remove_watch_rule() earlier to make sure
> mark does not get freed under us.
> 
> CC: stable@vger.kernel.org
> Reported-by: Tony Jones <tonyj@suse.de>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---

Tested-by: Tony Jones <tonyj@suse.de>

Fix tested and verified against v3.0 and mainline

Re: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()

[Paul Moore]

On Tue, Aug 15, 2017 at 7:00 AM, Jan Kara <jack@suse.cz> wrote:
> audit_remove_watch_rule() drops watch's reference to parent but then
> continues to work with it. That is not safe as parent can get freed once
> we drop our reference. The following is a trivial reproducer:
>
> mount -o loop image /mnt
> touch /mnt/file
> auditctl -w /mnt/file -p wax
> umount /mnt
> auditctl -D
> <crash in fsnotify_destroy_mark()>
>
> Grab our own reference in audit_remove_watch_rule() earlier to make sure
> mark does not get freed under us.
>
> CC: stable@vger.kernel.org
> Reported-by: Tony Jones <tonyj@suse.de>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  kernel/audit_watch.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

This looks good to me, thanks for the fix.  I'm going to merge this
into the audit/stable-4.13 branch and assuming it passes the audit
tests I'll send it up to Linus later this week.

> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 62d686d96581..ed748ee40029 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
>         list_del(&krule->rlist);
>
>         if (list_empty(&watch->rules)) {
> +               /*
> +                * audit_remove_watch() drops our reference to 'parent' which
> +                * can get freed. Grab our own reference to be safe.
> +                */
> +               audit_get_parent(parent);
>                 audit_remove_watch(watch);
> -
> -               if (list_empty(&parent->watches)) {
> -                       audit_get_parent(parent);
> +               if (list_empty(&parent->watches))
>                         fsnotify_destroy_mark(&parent->mark, audit_watch_group);
> -                       audit_put_parent(parent);
> -               }
> +               audit_put_parent(parent);
>         }
>  }
>
> --
> 2.12.3

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] audit: Receive unmount event

[Paul Moore]

On Tue, Aug 15, 2017 at 7:00 AM, Jan Kara <jack@suse.cz> wrote:
> Although audit_watch_handle_event() can handle FS_UNMOUNT event, it is
> not part of AUDIT_FS_WATCH mask and thus such event never gets to
> audit_watch_handle_event(). Thus fsnotify marks are deleted by fsnotify
> subsystem on unmount without audit being notified about that which leads
> to a strange state of existing audit rules with dead fsnotify marks.
>
> Add FS_UNMOUNT to the mask of events to be received so that audit can
> clean up its state accordingly.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  kernel/audit_watch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

It's funny how the rest of the audit code handles the FS_UNMOUNT
event, but it isn't in the mask.  It looks like it was lost in the
inotify to fanotify conversion.  Since I'm likely sending your other
patch up to Linus later this week, and I think this is a reasonable
bug-fix, I'm going to include this in the audit/stable-4.13 branch.

> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index ed748ee40029..9eb8b3511636 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -66,7 +66,7 @@ static struct fsnotify_group *audit_watch_group;
>
>  /* fsnotify events we care about. */
>  #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
> -                       FS_MOVE_SELF | FS_EVENT_ON_CHILD)
> +                       FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
>
>  static void audit_free_parent(struct audit_parent *parent)
>  {
> --
> 2.12.3

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()

[Richard Guy Briggs]

On 2017-08-15 13:00, Jan Kara wrote:
> audit_remove_watch_rule() drops watch's reference to parent but then
> continues to work with it. That is not safe as parent can get freed once
> we drop our reference. The following is a trivial reproducer:
> 
> mount -o loop image /mnt
> touch /mnt/file
> auditctl -w /mnt/file -p wax
> umount /mnt
> auditctl -D
> <crash in fsnotify_destroy_mark()>
> 
> Grab our own reference in audit_remove_watch_rule() earlier to make sure
> mark does not get freed under us.
> 
> CC: stable@vger.kernel.org
> Reported-by: Tony Jones <tonyj@suse.de>
> Signed-off-by: Jan Kara <jack@suse.cz>

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

> ---
>  kernel/audit_watch.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 62d686d96581..ed748ee40029 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
>  	list_del(&krule->rlist);
>  
>  	if (list_empty(&watch->rules)) {
> +		/*
> +		 * audit_remove_watch() drops our reference to 'parent' which
> +		 * can get freed. Grab our own reference to be safe.
> +		 */
> +		audit_get_parent(parent);
>  		audit_remove_watch(watch);
> -
> -		if (list_empty(&parent->watches)) {
> -			audit_get_parent(parent);
> +		if (list_empty(&parent->watches))
>  			fsnotify_destroy_mark(&parent->mark, audit_watch_group);
> -			audit_put_parent(parent);
> -		}
> +		audit_put_parent(parent);
>  	}
>  }
>  
> -- 
> 2.12.3
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

Re: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()

[Richard Guy Briggs]

On 2017-08-15 13:00, Jan Kara wrote:
> audit_remove_watch_rule() drops watch's reference to parent but then
> continues to work with it. That is not safe as parent can get freed once
> we drop our reference. The following is a trivial reproducer:
> 
> mount -o loop image /mnt
> touch /mnt/file
> auditctl -w /mnt/file -p wax
> umount /mnt
> auditctl -D
> <crash in fsnotify_destroy_mark()>
> 
> Grab our own reference in audit_remove_watch_rule() earlier to make sure
> mark does not get freed under us.
> 
> CC: stable@vger.kernel.org
> Reported-by: Tony Jones <tonyj@suse.de>
> Signed-off-by: Jan Kara <jack@suse.cz>

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

> ---
>  kernel/audit_watch.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 62d686d96581..ed748ee40029 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
>  	list_del(&krule->rlist);
>  
>  	if (list_empty(&watch->rules)) {
> +		/*
> +		 * audit_remove_watch() drops our reference to 'parent' which
> +		 * can get freed. Grab our own reference to be safe.
> +		 */
> +		audit_get_parent(parent);
>  		audit_remove_watch(watch);
> -
> -		if (list_empty(&parent->watches)) {
> -			audit_get_parent(parent);
> +		if (list_empty(&parent->watches))
>  			fsnotify_destroy_mark(&parent->mark, audit_watch_group);
> -			audit_put_parent(parent);
> -		}
> +		audit_put_parent(parent);
>  	}
>  }
>  
> -- 
> 2.12.3
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

Re: [PATCH 2/2] audit: Receive unmount event

[Richard Guy Briggs]

On 2017-08-15 15:55, Paul Moore wrote:
> On Tue, Aug 15, 2017 at 7:00 AM, Jan Kara <jack@suse.cz> wrote:
> > Although audit_watch_handle_event() can handle FS_UNMOUNT event, it is
> > not part of AUDIT_FS_WATCH mask and thus such event never gets to
> > audit_watch_handle_event(). Thus fsnotify marks are deleted by fsnotify
> > subsystem on unmount without audit being notified about that which leads
> > to a strange state of existing audit rules with dead fsnotify marks.
> >
> > Add FS_UNMOUNT to the mask of events to be received so that audit can
> > clean up its state accordingly.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>

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

> > ---
> >  kernel/audit_watch.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> It's funny how the rest of the audit code handles the FS_UNMOUNT
> event, but it isn't in the mask.  It looks like it was lost in the
> inotify to fanotify conversion.  Since I'm likely sending your other
> patch up to Linus later this week, and I think this is a reasonable
> bug-fix, I'm going to include this in the audit/stable-4.13 branch.
> 
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index ed748ee40029..9eb8b3511636 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -66,7 +66,7 @@ static struct fsnotify_group *audit_watch_group;
> >
> >  /* fsnotify events we care about. */
> >  #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
> > -                       FS_MOVE_SELF | FS_EVENT_ON_CHILD)
> > +                       FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
> >
> >  static void audit_free_parent(struct audit_parent *parent)
> >  {
> > --
> > 2.12.3
> 
> -- 
> paul moore
> www.paul-moore.com
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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