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

Groups > linux.kernel > #1203274 > unrolled thread

[PATCH V10] fixup! audit: add audit by children of executable path

Back to article view | Back to linux.kernel

Contents

  [PATCH V10] fixup! audit: add audit by children of executable path Richard Guy Briggs <rgb@redhat.com> - 2015-08-08 16:30 +0200
    Re: [PATCH V10] fixup! audit: add audit by children of executable path Paul Moore <pmoore@redhat.com> - 2015-08-10 18:30 +0200
      Re: [PATCH V10] fixup! audit: add audit by children of executable  path Richard Guy Briggs <rgb@redhat.com> - 2015-08-10 19:00 +0200
        Re: [PATCH V10] fixup! audit: add audit by children of executable path Paul Moore <pmoore@redhat.com> - 2015-08-10 20:50 +0200

#1203274 — [PATCH V10] fixup! audit: add audit by children of executable path

Adding "C=1 CF=-D__CHECK_ENDIAN__" to enable sparse warnings identified a
warning with the
	[PATCH V9 3/3] audit: add audit by children of executable path
patch posted a couple of days ago (and just re-posted due to another fix):

kernel/auditsc.c:476:46: warning: dereference of noderef expression
kernel/auditsc.c:477:61: warning: dereference of noderef expression

task_struct->parent requires RCU locking for access.  This fix resolves the two
warnings.

This patch should be merged with the patch it fixes once the fix is confirmed
to be the correct approach since the original patch hasn't been accepted yet.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
My hesitation is that the rcu lock scope is too broad.  If the loop were
re-structured to hold the rcu_read_lock() and call rcu_dereference() once per
iteration, would lock release and retake action cause more overhead?

 kernel/auditsc.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e1f0da2..3ed043d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -473,13 +473,16 @@ static int audit_filter_rules(struct task_struct *tsk,
 		{
 			struct task_struct *ptsk;
 
-			for (ptsk = tsk; ptsk->parent->pid > 0;
-			     ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
+			rcu_read_lock();
+			for (ptsk = tsk; rcu_dereference(ptsk->parent)->pid > 0;
+			     ptsk = find_task_by_pid_ns(rcu_dereference(ptsk->parent)->pid
+							, &init_pid_ns)) {
 				if (audit_exe_compare(ptsk, rule->exe)) {
 					++result;
 					break;
 				}
 			}
+			rcu_read_unlock();
 		}
 			break;
 		case AUDIT_UID:
-- 
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]

#1204416

On Saturday, August 08, 2015 10:23:48 AM Richard Guy Briggs wrote:
> Adding "C=1 CF=-D__CHECK_ENDIAN__" to enable sparse warnings identified a
> warning with the
> 	[PATCH V9 3/3] audit: add audit by children of executable path
> patch posted a couple of days ago (and just re-posted due to another fix):
> 
> kernel/auditsc.c:476:46: warning: dereference of noderef expression
> kernel/auditsc.c:477:61: warning: dereference of noderef expression
> 
> task_struct->parent requires RCU locking for access.  This fix resolves the
> two warnings.
> 
> This patch should be merged with the patch it fixes once the fix is
> confirmed to be the correct approach since the original patch hasn't been
> accepted yet.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> My hesitation is that the rcu lock scope is too broad.  If the loop were
> re-structured to hold the rcu_read_lock() and call rcu_dereference() once
> per iteration, would lock release and retake action cause more overhead?
> 
>  kernel/auditsc.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)

I'm still not convinced that we need to merge exe child filtering patch so I'm 
not going to apply this, or your v10 patch, at this point in time.  If you 
want to hold on to the code in case you and/or Steve think you can convince me 
at a later date, you might as well merge this fixup patch into the v10 patch.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e1f0da2..3ed043d 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -473,13 +473,16 @@ static int audit_filter_rules(struct task_struct *tsk,
> {
>  			struct task_struct *ptsk;
> 
> -			for (ptsk = tsk; ptsk->parent->pid > 0;
> -			     ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
> +			rcu_read_lock();
> +			for (ptsk = tsk; rcu_dereference(ptsk->parent)->pid > 0;
> +			     ptsk = find_task_by_pid_ns(rcu_dereference(ptsk->parent)->pid
> +							, &init_pid_ns)) {
>  				if (audit_exe_compare(ptsk, rule->exe)) {
>  					++result;
>  					break;
>  				}
>  			}
> +			rcu_read_unlock();
>  		}
>  			break;
>  		case AUDIT_UID:

-- 
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]

#1204433 — Re: [PATCH V10] fixup! audit: add audit by children of executable path

On 15/08/10, Paul Moore wrote:
> On Saturday, August 08, 2015 10:23:48 AM Richard Guy Briggs wrote:
> > Adding "C=1 CF=-D__CHECK_ENDIAN__" to enable sparse warnings identified a
> > warning with the
> > 	[PATCH V9 3/3] audit: add audit by children of executable path
> > patch posted a couple of days ago (and just re-posted due to another fix):
> > 
> > kernel/auditsc.c:476:46: warning: dereference of noderef expression
> > kernel/auditsc.c:477:61: warning: dereference of noderef expression
> > 
> > task_struct->parent requires RCU locking for access.  This fix resolves the
> > two warnings.
> > 
> > This patch should be merged with the patch it fixes once the fix is
> > confirmed to be the correct approach since the original patch hasn't been
> > accepted yet.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > My hesitation is that the rcu lock scope is too broad.  If the loop were
> > re-structured to hold the rcu_read_lock() and call rcu_dereference() once
> > per iteration, would lock release and retake action cause more overhead?
> > 
> >  kernel/auditsc.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> I'm still not convinced that we need to merge exe child filtering patch so I'm 
> not going to apply this, or your v10 patch, at this point in time.  If you 
> want to hold on to the code in case you and/or Steve think you can convince me 
> at a later date, you might as well merge this fixup patch into the v10 patch.

I fixed the known problems and reposted it for reference and public
archive so that it wasn't buried privately in my tree.

Aside from the need for the child filtering patch, do the RCU treatments
here look reasonable?

> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index e1f0da2..3ed043d 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -473,13 +473,16 @@ static int audit_filter_rules(struct task_struct *tsk,
> > {
> >  			struct task_struct *ptsk;
> > 
> > -			for (ptsk = tsk; ptsk->parent->pid > 0;
> > -			     ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
> > +			rcu_read_lock();
> > +			for (ptsk = tsk; rcu_dereference(ptsk->parent)->pid > 0;
> > +			     ptsk = find_task_by_pid_ns(rcu_dereference(ptsk->parent)->pid
> > +							, &init_pid_ns)) {
> >  				if (audit_exe_compare(ptsk, rule->exe)) {
> >  					++result;
> >  					break;
> >  				}
> >  			}
> > +			rcu_read_unlock();
> >  		}
> >  			break;
> >  		case AUDIT_UID:
> 
> 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] | [next] | [standalone]

#1204481

On Monday, August 10, 2015 12:53:54 PM Richard Guy Briggs wrote:
> On 15/08/10, Paul Moore wrote:
> > I'm still not convinced that we need to merge exe child filtering patch so
> > I'm not going to apply this, or your v10 patch, at this point in time. 
> > If you want to hold on to the code in case you and/or Steve think you can
> > convince me at a later date, you might as well merge this fixup patch
> > into the v10 patch.
>
> I fixed the known problems and reposted it for reference and public
> archive so that it wasn't buried privately in my tree.

No problem, that's smart.  I just wanted to respond so that there wasn't any 
confusion about the status of this patch; I figure everybody who takes the 
time to post a patch deserves a response, even if it is "no thanks".
 
> Aside from the need for the child filtering patch, do the RCU treatments
> here look reasonable?

Yep, it looks reasonable to me.

-- 
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web