Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1200584 > unrolled thread

[PATCH V5] audit: save signal match info in case entry passed in is the one deleted

Back to article view | Back to linux.kernel

Contents

  [PATCH V5] audit: save signal match info in case entry passed in is the one deleted Richard Guy Briggs <rgb@redhat.com> - 2015-08-05 11:30 +0200
    Re: [PATCH V5] audit: save signal match info in case entry passed in is the one deleted Paul Moore <pmoore@redhat.com> - 2015-08-05 17:20 +0200
      Re: [PATCH V5] audit: save signal match info in case entry passed in  is the one deleted Richard Guy Briggs <rgb@redhat.com> - 2015-08-05 21:30 +0200

#1200584 — [PATCH V5] audit: save signal match info in case entry passed in is the one deleted

Move the access to the entry for audit_match_signal() to the beginning of the
function in case the entry found is the same one passed in.  This will enable
it to be used by audit_remove_mark_rule().

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

Revision history:
v4 -> v5:
    Move mutex_unlock after out label.
    Move list_del group after test for signal to remove temp variable.

---
This patch was split out from the audit by executable path patch set due to the
potential to use it elsewhere.

In particular, some questions came up while assessing the potential for code
reuse:

	Why does audit_remove_parent_watches() not call audit_del_rule() for
	each entry found?
                Is audit_signals not properly decremented?
                Is audit_n_rules not properly decremented?

        Why does kill_rules() not call audit_del_rule() for each entry found?
                Is audit_signals not properly decremented?
                Is audit_n_rules not properly decremented?

 kernel/auditfilter.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 4cb9b44..1b110fb 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -953,7 +953,6 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	mutex_lock(&audit_filter_mutex);
 	e = audit_find_rule(entry, &list);
 	if (!e) {
-		mutex_unlock(&audit_filter_mutex);
 		ret = -ENOENT;
 		goto out;
 	}
@@ -964,9 +963,8 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	if (e->rule.tree)
 		audit_remove_tree_rule(&e->rule);
 
-	list_del_rcu(&e->list);
-	list_del(&e->rule.list);
-	call_rcu(&e->rcu, audit_free_rule_rcu);
+	if (e->rule.exe)
+		audit_remove_mark_rule(&e->rule);
 
 #ifdef CONFIG_AUDITSYSCALL
 	if (!dont_count)
@@ -975,9 +973,14 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	if (!audit_match_signal(entry))
 		audit_signals--;
 #endif
-	mutex_unlock(&audit_filter_mutex);
+
+	list_del_rcu(&e->list);
+	list_del(&e->rule.list);
+	call_rcu(&e->rcu, audit_free_rule_rcu);
 
 out:
+	mutex_unlock(&audit_filter_mutex);
+
 	if (tree)
 		audit_put_tree(tree);	/* that's the temporary one */
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1200860

On Wednesday, August 05, 2015 05:23:10 AM Richard Guy Briggs wrote:
> Move the access to the entry for audit_match_signal() to the beginning of
> the function in case the entry found is the same one passed in.  This will
> enable it to be used by audit_remove_mark_rule().
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> 
> Revision history:
> v4 -> v5:
>     Move mutex_unlock after out label.
>     Move list_del group after test for signal to remove temp variable.
> 
> ---
> This patch was split out from the audit by executable path patch set due to
> the potential to use it elsewhere.
> 
> In particular, some questions came up while assessing the potential for code
> reuse:
> 
> 	Why does audit_remove_parent_watches() not call audit_del_rule() for
> 	each entry found?
>                 Is audit_signals not properly decremented?
>                 Is audit_n_rules not properly decremented?
> 
>         Why does kill_rules() not call audit_del_rule() for each entry
> found? Is audit_signals not properly decremented?
>                 Is audit_n_rules not properly decremented?
> 
>  kernel/auditfilter.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 4cb9b44..1b110fb 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -953,7 +953,6 @@ static inline int audit_del_rule(struct audit_entry
> *entry) mutex_lock(&audit_filter_mutex);
>  	e = audit_find_rule(entry, &list);
>  	if (!e) {
> -		mutex_unlock(&audit_filter_mutex);
>  		ret = -ENOENT;
>  		goto out;
>  	}
> @@ -964,9 +963,8 @@ static inline int audit_del_rule(struct audit_entry
> *entry) if (e->rule.tree)
>  		audit_remove_tree_rule(&e->rule);
> 
> -	list_del_rcu(&e->list);
> -	list_del(&e->rule.list);
> -	call_rcu(&e->rcu, audit_free_rule_rcu);
> +	if (e->rule.exe)
> +		audit_remove_mark_rule(&e->rule);

What?

I think you munged a cut n' paste somehow.  This code doesn't even compile.

>  #ifdef CONFIG_AUDITSYSCALL
>  	if (!dont_count)
> @@ -975,9 +973,14 @@ static inline int audit_del_rule(struct audit_entry
> *entry) if (!audit_match_signal(entry))
>  		audit_signals--;
>  #endif
> -	mutex_unlock(&audit_filter_mutex);
> +
> +	list_del_rcu(&e->list);
> +	list_del(&e->rule.list);
> +	call_rcu(&e->rcu, audit_free_rule_rcu);
> 
>  out:
> +	mutex_unlock(&audit_filter_mutex);
> +
>  	if (tree)
>  		audit_put_tree(tree);	/* that's the temporary one */

-- 
paul moore
security @ redhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201073 — Re: [PATCH V5] audit: save signal match info in case entry passed in is the one deleted

On 15/08/05, Paul Moore wrote:
> On Wednesday, August 05, 2015 05:23:10 AM Richard Guy Briggs wrote:
> > Move the access to the entry for audit_match_signal() to the beginning of
> > the function in case the entry found is the same one passed in.  This will
> > enable it to be used by audit_remove_mark_rule().
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > 
> > Revision history:
> > v4 -> v5:
> >     Move mutex_unlock after out label.
> >     Move list_del group after test for signal to remove temp variable.
> > 
> > ---
> > This patch was split out from the audit by executable path patch set due to
> > the potential to use it elsewhere.
> > 
> > In particular, some questions came up while assessing the potential for code
> > reuse:
> > 
> > 	Why does audit_remove_parent_watches() not call audit_del_rule() for
> > 	each entry found?
> >                 Is audit_signals not properly decremented?
> >                 Is audit_n_rules not properly decremented?
> > 
> >         Why does kill_rules() not call audit_del_rule() for each entry
> > found? Is audit_signals not properly decremented?
> >                 Is audit_n_rules not properly decremented?
> > 
> >  kernel/auditfilter.c |   13 ++++++++-----
> >  1 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 4cb9b44..1b110fb 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -953,7 +953,6 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) mutex_lock(&audit_filter_mutex);
> >  	e = audit_find_rule(entry, &list);
> >  	if (!e) {
> > -		mutex_unlock(&audit_filter_mutex);
> >  		ret = -ENOENT;
> >  		goto out;
> >  	}
> > @@ -964,9 +963,8 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (e->rule.tree)
> >  		audit_remove_tree_rule(&e->rule);
> > 
> > -	list_del_rcu(&e->list);
> > -	list_del(&e->rule.list);
> > -	call_rcu(&e->rcu, audit_free_rule_rcu);
> > +	if (e->rule.exe)
> > +		audit_remove_mark_rule(&e->rule);
> 
> What?

Wow, whoops!  I had to stare at it a while to see what was wrong...
Those last two lines belong in a different patch set...

> I think you munged a cut n' paste somehow.  This code doesn't even compile.

That was a local git tree rebase merge conflict manual fix error...

Not a bisect, but with the other patch set, it does.  :)
Re-generating audit-by-executable patchset too...

> >  #ifdef CONFIG_AUDITSYSCALL
> >  	if (!dont_count)
> > @@ -975,9 +973,14 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (!audit_match_signal(entry))
> >  		audit_signals--;
> >  #endif
> > -	mutex_unlock(&audit_filter_mutex);
> > +
> > +	list_del_rcu(&e->list);
> > +	list_del(&e->rule.list);
> > +	call_rcu(&e->rcu, audit_free_rule_rcu);
> > 
> >  out:
> > +	mutex_unlock(&audit_filter_mutex);
> > +
> >  	if (tree)
> >  		audit_put_tree(tree);	/* that's the temporary one */
> 
> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web