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

Groups > linux.kernel > #1220332 > unrolled thread

[PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

Back to article view | Back to linux.kernel

Contents

  [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-07 18:50 +0200
    Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-07 19:00 +0200
      Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd Paul Moore <pmoore@redhat.com> - 2015-09-09 23:00 +0200
        Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-11 12:30 +0200
          Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Paul Moore <paul@paul-moore.com> - 2015-09-11 21:00 +0200
            Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-13 18:10 +0200
              Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd Paul Moore <paul@paul-moore.com> - 2015-09-14 21:40 +0200
                Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-16 12:30 +0200
                  Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Paul Moore <paul@paul-moore.com> - 2015-09-16 23:50 +0200
                    Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-17 13:40 +0200
                      Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Paul Moore <paul@paul-moore.com> - 2015-09-18 00:50 +0200
    Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Eric Paris <eparis@redhat.com> - 2015-09-08 17:00 +0200
      Re: [PATCH V1] audit: add warning that an old auditd may be starved  out by a new auditd Richard Guy Briggs <rgb@redhat.com> - 2015-09-09 08:40 +0200

#1220332 — [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

Nothing prevents a new auditd starting up and replacing a valid
audit_pid when an old auditd is still running, effectively starving out
the old auditd since audit_pid no longer points to the old valid auditd.

There isn't an easy way to detect if an old auditd is still running on
the existing audit_pid other than attempting to send a message to see if
it fails.  If no message to auditd has been attempted since auditd died
unnaturally or got killed, audit_pid will still indicate it is alive.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
Note: Would it be too bold to actually block the registration of a new
auditd if the netlink_getsockbyportid() call succeeded?  Would other
checks be appropriate?

 kernel/audit.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 18cdfe2..1fa1e0d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -872,6 +872,11 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		if (s.mask & AUDIT_STATUS_PID) {
 			int new_pid = s.pid;
 
+			if (audit_pid && new_pid &&
+			    !IS_ERR(netlink_getsockbyportid(audit_sock, audit_nlk_portid)))
+				pr_warn("auditd replaced by new auditd before normal shutdown: "
+					"(old)audit_pid=%d (by)pid=%d new_pid=%d",
+					audit_pid, pid, new_pid);
 			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
 				return -EACCES;
 			if (audit_enabled != AUDIT_OFF)
-- 
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]

#1220337 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On 15/09/07, Richard Guy Briggs wrote:
> Nothing prevents a new auditd starting up and replacing a valid
> audit_pid when an old auditd is still running, effectively starving out
> the old auditd since audit_pid no longer points to the old valid auditd.
> 
> There isn't an easy way to detect if an old auditd is still running on
> the existing audit_pid other than attempting to send a message to see if
> it fails.  If no message to auditd has been attempted since auditd died
> unnaturally or got killed, audit_pid will still indicate it is alive.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

Ok, self-nack on this one for a couple of problems...
netlink_getsockbyportid() is static to af_netlink.c and "pid" should be
task_tgid_vnr(current).  Otherwise, any opinions on this approach?

> ---
> Note: Would it be too bold to actually block the registration of a new
> auditd if the netlink_getsockbyportid() call succeeded?  Would other
> checks be appropriate?
> 
>  kernel/audit.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 18cdfe2..1fa1e0d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -872,6 +872,11 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  		if (s.mask & AUDIT_STATUS_PID) {
>  			int new_pid = s.pid;
>  
> +			if (audit_pid && new_pid &&
> +			    !IS_ERR(netlink_getsockbyportid(audit_sock, audit_nlk_portid)))
> +				pr_warn("auditd replaced by new auditd before normal shutdown: "
> +					"(old)audit_pid=%d (by)pid=%d new_pid=%d",
> +					audit_pid, pid, new_pid);
>  			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
>  				return -EACCES;
>  			if (audit_enabled != AUDIT_OFF)
> -- 
> 1.7.1
> 

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

#1221705

On Monday, September 07, 2015 12:58:18 PM Richard Guy Briggs wrote:
> On 15/09/07, Richard Guy Briggs wrote:
> > Nothing prevents a new auditd starting up and replacing a valid
> > audit_pid when an old auditd is still running, effectively starving out
> > the old auditd since audit_pid no longer points to the old valid auditd.
> > 
> > There isn't an easy way to detect if an old auditd is still running on
> > the existing audit_pid other than attempting to send a message to see if
> > it fails.  If no message to auditd has been attempted since auditd died
> > unnaturally or got killed, audit_pid will still indicate it is alive.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> 
> Ok, self-nack on this one for a couple of problems...
> netlink_getsockbyportid() is static to af_netlink.c and "pid" should be
> task_tgid_vnr(current).  Otherwise, any opinions on this approach?
> 
> > ---
> > Note: Would it be too bold to actually block the registration of a new
> > auditd if the netlink_getsockbyportid() call succeeded?  Would other
> > checks be appropriate?

Hmm.  It seems like we should prevent the registration of a new auditd if we 
already have an auditd instance connected, although as you say, that isn't the 
easiest thing to do.

How painful would it be to return -EAGAIN to the new auditd while sending some 
sort of keep-alive/ping/etc. message to the old daemon to check its status?

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

#1222697 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On 15/09/09, Paul Moore wrote:
> On Monday, September 07, 2015 12:58:18 PM Richard Guy Briggs wrote:
> > On 15/09/07, Richard Guy Briggs wrote:
> > > Nothing prevents a new auditd starting up and replacing a valid
> > > audit_pid when an old auditd is still running, effectively starving out
> > > the old auditd since audit_pid no longer points to the old valid auditd.
> > > 
> > > There isn't an easy way to detect if an old auditd is still running on
> > > the existing audit_pid other than attempting to send a message to see if
> > > it fails.  If no message to auditd has been attempted since auditd died
> > > unnaturally or got killed, audit_pid will still indicate it is alive.
> > > 
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > 
> > Ok, self-nack on this one for a couple of problems...
> > netlink_getsockbyportid() is static to af_netlink.c and "pid" should be
> > task_tgid_vnr(current).  Otherwise, any opinions on this approach?
> > 
> > > ---
> > > Note: Would it be too bold to actually block the registration of a new
> > > auditd if the netlink_getsockbyportid() call succeeded?  Would other
> > > checks be appropriate?
> 
> Hmm.  It seems like we should prevent the registration of a new auditd if we 
> already have an auditd instance connected, although as you say, that isn't the 
> easiest thing to do.

I wanted to do that, but I feared it would carry some risk that an
intentional act would be blocked.  In that case, an intentional act
could include explicitly killing the old auditd (or process registered
as such).

> How painful would it be to return -EAGAIN to the new auditd while sending some 
> sort of keep-alive/ping/etc. message to the old daemon to check its status?

Well, if it turns out that the only reason it ever fails is
-ECONNREFUSED, then we just need to check with netlink_getsockbyportid()
to see if it fails before accepting the new auditd.

If it is one of the others, can we put the new auditd task on a wait
queue until we hear back one way or the other or just timeout on
contacting the old auditd?

I'll let Steve speak to dealing with -EAGAIN.  auditlib already deals
with -EAGAIN and -EINTR for some cases.  I have a patch that added
-ENOBUFS to those cases since I had seen some reports that -ENOBUFS had
been returned in some cases (don't remember the circumstances).

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

#1223043 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On Fri, Sep 11, 2015 at 6:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 15/09/09, Paul Moore wrote:
>> On Monday, September 07, 2015 12:58:18 PM Richard Guy Briggs wrote:
>> > On 15/09/07, Richard Guy Briggs wrote:
>> > > Nothing prevents a new auditd starting up and replacing a valid
>> > > audit_pid when an old auditd is still running, effectively starving out
>> > > the old auditd since audit_pid no longer points to the old valid auditd.
>> > >
>> > > There isn't an easy way to detect if an old auditd is still running on
>> > > the existing audit_pid other than attempting to send a message to see if
>> > > it fails.  If no message to auditd has been attempted since auditd died
>> > > unnaturally or got killed, audit_pid will still indicate it is alive.
>> > >
>> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >
>> > Ok, self-nack on this one for a couple of problems...
>> > netlink_getsockbyportid() is static to af_netlink.c and "pid" should be
>> > task_tgid_vnr(current).  Otherwise, any opinions on this approach?
>> >
>> > > ---
>> > > Note: Would it be too bold to actually block the registration of a new
>> > > auditd if the netlink_getsockbyportid() call succeeded?  Would other
>> > > checks be appropriate?
>>
>> Hmm.  It seems like we should prevent the registration of a new auditd if we
>> already have an auditd instance connected, although as you say, that isn't the
>> easiest thing to do.
>
> I wanted to do that, but I feared it would carry some risk that an
> intentional act would be blocked.  In that case, an intentional act
> could include explicitly killing the old auditd (or process registered
> as such).

Well, if you are running another instance of auditd you should have
the right level of access to kill an older, existing auditd instance
so I'm not too worried about preventing an intentional act.  Also, if
we check an existing connection with some sort of heartbeat/ping
message we wouldn't be preventing it for more than a few times I
believe.

>> How painful would it be to return -EAGAIN to the new auditd while sending some
>> sort of keep-alive/ping/etc. message to the old daemon to check its status?
>
> Well, if it turns out that the only reason it ever fails is
> -ECONNREFUSED, then we just need to check with netlink_getsockbyportid()
> to see if it fails before accepting the new auditd.

I'm not sure if the netdev crowd would be interested in exporting
_getsockbyportid(); for the sake of discussion let's assume no changes
to the netlink layer, if we get stuck we can revisit this idea.

> If it is one of the others, can we put the new auditd task on a wait
> queue until we hear back one way or the other or just timeout on
> contacting the old auditd?

Well, what if we don't have anything queued for the old auditd?
Although I suppose if nothing else we could send a record indicating
that another auditd attempted to replace it ... if we can send it
great, drop the new request and be glad we audited it, if we can't
send it, reset the auditd tracking.

> I'll let Steve speak to dealing with -EAGAIN.  auditlib already deals
> with -EAGAIN and -EINTR for some cases.  I have a patch that added
> -ENOBUFS to those cases since I had seen some reports that -ENOBUFS had
> been returned in some cases (don't remember the circumstances).

-- 
paul moore
www.paul-moore.com
--
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]

#1223715 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On 15/09/11, Paul Moore wrote:
> On Fri, Sep 11, 2015 at 6:21 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 15/09/09, Paul Moore wrote:
> >> On Monday, September 07, 2015 12:58:18 PM Richard Guy Briggs wrote:
> >> > On 15/09/07, Richard Guy Briggs wrote:
> >> > > Nothing prevents a new auditd starting up and replacing a valid
> >> > > audit_pid when an old auditd is still running, effectively starving out
> >> > > the old auditd since audit_pid no longer points to the old valid auditd.
> >> > >
> >> > > There isn't an easy way to detect if an old auditd is still running on
> >> > > the existing audit_pid other than attempting to send a message to see if
> >> > > it fails.  If no message to auditd has been attempted since auditd died
> >> > > unnaturally or got killed, audit_pid will still indicate it is alive.
> >> > >
> >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> >
> >> > Ok, self-nack on this one for a couple of problems...
> >> > netlink_getsockbyportid() is static to af_netlink.c and "pid" should be
> >> > task_tgid_vnr(current).  Otherwise, any opinions on this approach?
> >> >
> >> > > ---
> >> > > Note: Would it be too bold to actually block the registration of a new
> >> > > auditd if the netlink_getsockbyportid() call succeeded?  Would other
> >> > > checks be appropriate?
> >>
> >> Hmm.  It seems like we should prevent the registration of a new auditd if we
> >> already have an auditd instance connected, although as you say, that isn't the
> >> easiest thing to do.
> >
> > I wanted to do that, but I feared it would carry some risk that an
> > intentional act would be blocked.  In that case, an intentional act
> > could include explicitly killing the old auditd (or process registered
> > as such).
> 
> Well, if you are running another instance of auditd you should have
> the right level of access to kill an older, existing auditd instance
> so I'm not too worried about preventing an intentional act.  Also, if
> we check an existing connection with some sort of heartbeat/ping
> message we wouldn't be preventing it for more than a few times I
> believe.

Agreed.

> >> How painful would it be to return -EAGAIN to the new auditd while sending some
> >> sort of keep-alive/ping/etc. message to the old daemon to check its status?
> >
> > Well, if it turns out that the only reason it ever fails is
> > -ECONNREFUSED, then we just need to check with netlink_getsockbyportid()
> > to see if it fails before accepting the new auditd.
> 
> I'm not sure if the netdev crowd would be interested in exporting
> _getsockbyportid(); for the sake of discussion let's assume no changes
> to the netlink layer, if we get stuck we can revisit this idea.

Agreed.

> > If it is one of the others, can we put the new auditd task on a wait
> > queue until we hear back one way or the other or just timeout on
> > contacting the old auditd?
> 
> Well, what if we don't have anything queued for the old auditd?

Then it won't notice until something does get queued.  That would be the
purpose of sending a ping.

> Although I suppose if nothing else we could send a record indicating
> that another auditd attempted to replace it ... if we can send it
> great, drop the new request and be glad we audited it, if we can't
> send it, reset the auditd tracking.

This is actually a good idea.

> > I'll let Steve speak to dealing with -EAGAIN.  auditlib already deals
> > with -EAGAIN and -EINTR for some cases.  I have a patch that added
> > -ENOBUFS to those cases since I had seen some reports that -ENOBUFS had
> > been returned in some cases (don't remember the circumstances).
> 
> 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]

#1224423

On Sunday, September 13, 2015 12:08:19 PM Richard Guy Briggs wrote:
> On 15/09/11, Paul Moore wrote:
> > Although I suppose if nothing else we could send a record indicating
> > that another auditd attempted to replace it ... if we can send it
> > great, drop the new request and be glad we audited it, if we can't
> > send it, reset the auditd tracking.
> 
> This is actually a good idea.

This would go well with your last patch to try harder on netlink send 
failures.  On a related note, with the merge window closed I just rotated the 
audit tree so that patch is now in linux-audit#next.

-- 
paul moore
www.paul-moore.com

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

#1225965 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On 15/09/14, Paul Moore wrote:
> On Sunday, September 13, 2015 12:08:19 PM Richard Guy Briggs wrote:
> > On 15/09/11, Paul Moore wrote:
> > > Although I suppose if nothing else we could send a record indicating
> > > that another auditd attempted to replace it ... if we can send it
> > > great, drop the new request and be glad we audited it, if we can't
> > > send it, reset the auditd tracking.
> > 
> > This is actually a good idea.
> 
> This would go well with your last patch to try harder on netlink send 
> failures.

Re-looking at the AUDIT_STATUS_PID case, I'm noticing we only
audit_log_config_change() on success.  At the moment, auditd userspace
doesn't know about this new AUDIT_PING netlink message type I'm adding
for testing the health of the existing audit, so it will just be dropped
by existing auditd.  I think it makes sense to add
audit_log_config_change() on both the orphaning and starving cases
indicating the result=0 so that there is a record.  Arguably the
orphaning case can never happen again since the starving fix will
prevent a newer auditd from running.

>  On a related note, with the merge window closed I just rotated the 
> audit tree so that patch is now in linux-audit#next.

Thanks.

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

#1226486 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On Wed, Sep 16, 2015 at 6:24 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 15/09/14, Paul Moore wrote:
>> On Sunday, September 13, 2015 12:08:19 PM Richard Guy Briggs wrote:
>> > On 15/09/11, Paul Moore wrote:
>> > > Although I suppose if nothing else we could send a record indicating
>> > > that another auditd attempted to replace it ... if we can send it
>> > > great, drop the new request and be glad we audited it, if we can't
>> > > send it, reset the auditd tracking.
>> >
>> > This is actually a good idea.
>>
>> This would go well with your last patch to try harder on netlink send
>> failures.
>
> Re-looking at the AUDIT_STATUS_PID case, I'm noticing we only
> audit_log_config_change() on success.  At the moment, auditd userspace
> doesn't know about this new AUDIT_PING netlink message type I'm adding
> for testing the health of the existing audit, so it will just be dropped
> by existing auditd.  I think it makes sense to add
> audit_log_config_change() on both the orphaning and starving cases
> indicating the result=0 so that there is a record.  Arguably the
> orphaning case can never happen again since the starving fix will
> prevent a newer auditd from running.

Just so I'm clear, the "starving" case is when a new auditd tries to
evict a perfectly good auditd?

Otherwise, I think adding a result/success field to the
AUDIT_CONFIG_CHANGE record makes sense as long as it doesn't break
Steve's parsing code (I don't think it will, although it may simply
ignore it, which is okay).

-- 
paul moore
www.paul-moore.com
--
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]

#1226930 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On 15/09/16, Paul Moore wrote:
> On Wed, Sep 16, 2015 at 6:24 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 15/09/14, Paul Moore wrote:
> >> On Sunday, September 13, 2015 12:08:19 PM Richard Guy Briggs wrote:
> >> > On 15/09/11, Paul Moore wrote:
> >> > > Although I suppose if nothing else we could send a record indicating
> >> > > that another auditd attempted to replace it ... if we can send it
> >> > > great, drop the new request and be glad we audited it, if we can't
> >> > > send it, reset the auditd tracking.
> >> >
> >> > This is actually a good idea.
> >>
> >> This would go well with your last patch to try harder on netlink send
> >> failures.
> >
> > Re-looking at the AUDIT_STATUS_PID case, I'm noticing we only
> > audit_log_config_change() on success.  At the moment, auditd userspace
> > doesn't know about this new AUDIT_PING netlink message type I'm adding
> > for testing the health of the existing audit, so it will just be dropped
> > by existing auditd.  I think it makes sense to add
> > audit_log_config_change() on both the orphaning and starving cases
> > indicating the result=0 so that there is a record.  Arguably the
> > orphaning case can never happen again since the starving fix will
> > prevent a newer auditd from running.
> 
> Just so I'm clear, the "starving" case is when a new auditd tries to
> evict a perfectly good auditd?

Not evict so much as trample.  It just stomps on the existing audit_pid
reference and the old one isn't aware (unless it sends a status request
and checks the PID value) that it has been supplanted.

> Otherwise, I think adding a result/success field to the
> AUDIT_CONFIG_CHANGE record makes sense as long as it doesn't break
> Steve's parsing code (I don't think it will, although it may simply
> ignore it, which is okay).

It is already there, but never used for anything but success.  I'm
proposing to add code to actually report the failures too.

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

#1227469 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On Thu, Sep 17, 2015 at 7:35 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 15/09/16, Paul Moore wrote:
>> Otherwise, I think adding a result/success field to the
>> AUDIT_CONFIG_CHANGE record makes sense as long as it doesn't break
>> Steve's parsing code (I don't think it will, although it may simply
>> ignore it, which is okay).
>
> It is already there, but never used for anything but success.  I'm
> proposing to add code to actually report the failures too.

So it is, even better as there shouldn't be a userspace problem.

-- 
paul moore
www.paul-moore.com
--
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]

#1220908 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

This is already going to be in the audit log, right? We're going to
send a CONFIG_CHANGE record with old_pid == the existing auditd. I bet
it gets delivered to the old auditd.

But why is this a printk(KERN_WARN) ?

On Mon, 2015-09-07 at 12:48 -0400, Richard Guy Briggs wrote:
> Nothing prevents a new auditd starting up and replacing a valid
> audit_pid when an old auditd is still running, effectively starving
> out
> the old auditd since audit_pid no longer points to the old valid
> auditd.
> 
> There isn't an easy way to detect if an old auditd is still running
> on
> the existing audit_pid other than attempting to send a message to see
> if
> it fails.  If no message to auditd has been attempted since auditd
> died
> unnaturally or got killed, audit_pid will still indicate it is alive.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Note: Would it be too bold to actually block the registration of a
> new
> auditd if the netlink_getsockbyportid() call succeeded?  Would other
> checks be appropriate?
> 
>  kernel/audit.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 18cdfe2..1fa1e0d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -872,6 +872,11 @@ static int audit_receive_msg(struct sk_buff
> *skb, struct nlmsghdr *nlh)
>  		if (s.mask & AUDIT_STATUS_PID) {
>  			int new_pid = s.pid;
>  
> +			if (audit_pid && new_pid &&
> +			   
>  !IS_ERR(netlink_getsockbyportid(audit_sock, audit_nlk_portid)))
> +				pr_warn("auditd replaced by new
> auditd before normal shutdown: "
> +					"(old)audit_pid=%d
> (by)pid=%d new_pid=%d",
> +					audit_pid, pid, new_pid);
>  			if ((!new_pid) && (task_tgid_vnr(current) !=
> audit_pid))
>  				return -EACCES;
>  			if (audit_enabled != AUDIT_OFF)
--
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]

#1221274 — Re: [PATCH V1] audit: add warning that an old auditd may be starved out by a new auditd

On 15/09/08, Eric Paris wrote:
> This is already going to be in the audit log, right? We're going to
> send a CONFIG_CHANGE record with old_pid == the existing auditd. I bet
> it gets delivered to the old auditd.

Actually, delivered by the new auditd is what I'm seeing...  (Tested by
running "auditd -f" to have it show up in the debug output and not in
the log file.)  I did see it once as two seperate messages, one setting
to zero and the next to the new PID, but that may have been a testing
procedure error...

(Note: Why does auditd run in a terminal with -n or -f not respond to ^C?)

> But why is this a printk(KERN_WARN) ?

Because we're still trying to figure out what is going on...  But point
taken, we already should have that information.  If it is listed as a
warning it has a better chance of getting reported.  What do you suggest
instead?


Eric, thanks for taking the time to review this...


> On Mon, 2015-09-07 at 12:48 -0400, Richard Guy Briggs wrote:
> > Nothing prevents a new auditd starting up and replacing a valid
> > audit_pid when an old auditd is still running, effectively starving out
> > the old auditd since audit_pid no longer points to the old valid auditd.
> > 
> > There isn't an easy way to detect if an old auditd is still running on
> > the existing audit_pid other than attempting to send a message to see if
> > it fails.  If no message to auditd has been attempted since auditd died
> > unnaturally or got killed, audit_pid will still indicate it is alive.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > Note: Would it be too bold to actually block the registration of a new
> > auditd if the netlink_getsockbyportid() call succeeded?  Would other
> > checks be appropriate?
> > 
> >  kernel/audit.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 18cdfe2..1fa1e0d 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -872,6 +872,11 @@ static int audit_receive_msg(struct sk_buff
> > *skb, struct nlmsghdr *nlh)
> >  		if (s.mask & AUDIT_STATUS_PID) {
> >  			int new_pid = s.pid;
> >  
> > +			if (audit_pid && new_pid &&
> > +			   !IS_ERR(netlink_getsockbyportid(audit_sock, audit_nlk_portid)))
> > +				pr_warn("auditd replaced by new auditd before normal shutdown: "
> > +					"(old)audit_pid=%d (by)pid=%d new_pid=%d",
> > +					audit_pid, pid, new_pid);
> >  			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> >  				return -EACCES;
> >  			if (audit_enabled != AUDIT_OFF)

- 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