[PATCH V4 (was V6)] generalize audit_del_rule

[PATCH V4 (was V6)] generalize audit_del_rule

[Richard Guy Briggs]

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?


Richard Guy Briggs (1):
  audit: save signal match info in case entry passed in is the one
    deleted

 kernel/auditfilter.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

[PATCH V4 (was V6)] audit: save signal match info in case entry passed in is the one deleted

[Richard Guy Briggs]

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>
---
 kernel/auditfilter.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 4cb9b44..afb63b3 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -943,6 +943,7 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	int ret = 0;
 #ifdef CONFIG_AUDITSYSCALL
 	int dont_count = 0;
+	int match_signal = !audit_match_signal(entry);
 
 	/* If either of these, don't count towards total */
 	if (entry->rule.listnr == AUDIT_FILTER_USER ||
@@ -972,7 +973,7 @@ static inline int audit_del_rule(struct audit_entry *entry)
 	if (!dont_count)
 		audit_n_rules--;
 
-	if (!audit_match_signal(entry))
+	if (match_signal)
 		audit_signals--;
 #endif
 	mutex_unlock(&audit_filter_mutex);
-- 
1.7.1

Re: [PATCH V4 (was V6)] audit: save signal match info in case entry passed in is the one deleted

[Paul Moore]

On Saturday, August 01, 2015 03:44:01 PM 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>
> ---
>  kernel/auditfilter.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 4cb9b44..afb63b3 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -943,6 +943,7 @@ static inline int audit_del_rule(struct audit_entry
> *entry) int ret = 0;
>  #ifdef CONFIG_AUDITSYSCALL
>  	int dont_count = 0;
> +	int match_signal = !audit_match_signal(entry);
> 
>  	/* If either of these, don't count towards total */
>  	if (entry->rule.listnr == AUDIT_FILTER_USER ||
> @@ -972,7 +973,7 @@ static inline int audit_del_rule(struct audit_entry
> *entry) if (!dont_count)
>  		audit_n_rules--;
> 
> -	if (!audit_match_signal(entry))
> +	if (match_signal)
>  		audit_signals--;
>  #endif
>  	mutex_unlock(&audit_filter_mutex);

Why not simply move this second CONFIG_AUDITSYSCALL above the list_del() 
calls?  Am I missing something?

Also, while we're fixing up audit_del_rule(), why not also move the 
mutex_unlock() call to after the "out" jump target and then drop the 
mutex_unlock() call in the audit_find_rule() error case?  Not your fault, but 
the code seems silly as-is.

-- 
paul moore
security @ redhat

Re: [PATCH V4 (was V6)] audit: save signal match info in case entry passed in is the one deleted

[Richard Guy Briggs]

On 15/08/04, Paul Moore wrote:
> On Saturday, August 01, 2015 03:44:01 PM 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>
> > ---
> >  kernel/auditfilter.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 4cb9b44..afb63b3 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -943,6 +943,7 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) int ret = 0;
> >  #ifdef CONFIG_AUDITSYSCALL
> >  	int dont_count = 0;
> > +	int match_signal = !audit_match_signal(entry);
> > 
> >  	/* If either of these, don't count towards total */
> >  	if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > @@ -972,7 +973,7 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (!dont_count)
> >  		audit_n_rules--;
> > 
> > -	if (!audit_match_signal(entry))
> > +	if (match_signal)
> >  		audit_signals--;
> >  #endif
> >  	mutex_unlock(&audit_filter_mutex);
> 
> Why not simply move this second CONFIG_AUDITSYSCALL above the list_del() 
> calls?  Am I missing something?

Good point.  That did occur to me at one point when I wasn't in front of
the code and promptly forgot once I was.  That will neatly remove the
temporary variable.

> Also, while we're fixing up audit_del_rule(), why not also move the 
> mutex_unlock() call to after the "out" jump target and then drop the 
> mutex_unlock() call in the audit_find_rule() error case?  Not your fault, but 
> the code seems silly as-is.

Yes, agreed.  Another nice catch.

These changes will affect "audit by executable" so I'll re-spin that
patch set to make it easier to apply.

> 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