Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Richard Guy Briggs]

Steve,

Subject says it all...

Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?


- 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

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Eric Paris]

On Tue, 2014-03-11 at 18:15 -0400, Richard Guy Briggs wrote:
> Steve,
> 
> Subject says it all...
> 
> Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

No...

Given that userspace requests AUDIT_SIGNAL_INFO after it gets a signal,
and that audit_sig_{uid,pid,...} get filled in when some task sent
auditd that signal, the idea that the pid would be 0 doesn't make
sense...  (unless auditd requests AUDIT_SIGNAL_INFO without getting a
signal, but that's just dumb)

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Richard Guy Briggs]

On 14/03/11, Eric Paris wrote:
> On Tue, 2014-03-11 at 18:15 -0400, Richard Guy Briggs wrote:
> > Steve,
> > 
> > Subject says it all...
> > 
> > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?
> 
> No...
> 
> Given that userspace requests AUDIT_SIGNAL_INFO after it gets a signal,
> and that audit_sig_{uid,pid,...} get filled in when some task sent
> auditd that signal, the idea that the pid would be 0 doesn't make
> sense...  (unless auditd requests AUDIT_SIGNAL_INFO without getting a
> signal, but that's just dumb)

The reason I ask is that it is initialized to -1, which I assume is no
more valid than zero in your interpretation above.

I looked at converting audit_sig_pid from pid_t to struct pid *, but
then get_pid() would also be needed to protect that reference.  A
put_pid() would need to be done once it is no longer needed, which could
be immediately after it is read in the AUDIT_SIGNAL_INFO message
preparation, assuming it would never need to be read again.  If this
isn't the case, put_pid() could be called when audit_pid is nulled, but
if that message never comes, that struct pid is stuck with a stale
refcount.  (That isn't an issue if it is init or systemd, but it is
still wrong.)

This looks more and more like overkill and should probably leave
audit_sig_pid as pid_t.

- 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

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Steve Grubb]

On Tuesday, March 11, 2014 06:15:17 PM Richard Guy Briggs wrote:
> Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

Well, pid=0 would be the kernel. So, its valid but unlikely. Offhand I don't 
know why the kernel might try sending a signal. Is this a problem?

-Steve

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Steve Grubb]

On Tuesday, March 11, 2014 11:32:01 PM Richard Guy Briggs wrote:
> On 14/03/11, Eric Paris wrote:
> > On Tue, 2014-03-11 at 18:15 -0400, Richard Guy Briggs wrote:
> > > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO
> > > message?
> > 
> > No...
> > 
> > Given that userspace requests AUDIT_SIGNAL_INFO after it gets a signal,
> > and that audit_sig_{uid,pid,...} get filled in when some task sent
> > auditd that signal, the idea that the pid would be 0 doesn't make
> > sense...  (unless auditd requests AUDIT_SIGNAL_INFO without getting a
> > signal, but that's just dumb)
> 
> The reason I ask is that it is initialized to -1, which I assume is no
> more valid than zero in your interpretation above.

pid=-1 has a special meaning for signals. But in terms of seeing it in a 
sigaction handler for siginfo, not possible. So its a good init value. If you 
look at sigaction(2), there is a si_code that indicates why the signal was 
sent. One of them is SI_KERNEL. So, its possible that the kernel decides to 
send a signal on certain occasions.

> I looked at converting audit_sig_pid from pid_t to struct pid *, but
> then get_pid() would also be needed to protect that reference.  A
> put_pid() would need to be done once it is no longer needed, which could
> be immediately after it is read in the AUDIT_SIGNAL_INFO message
> preparation, assuming it would never need to be read again.  If this
> isn't the case, put_pid() could be called when audit_pid is nulled, but
> if that message never comes, that struct pid is stuck with a stale
> refcount.  (That isn't an issue if it is init or systemd, but it is
> still wrong.)
> 
> This looks more and more like overkill and should probably leave
> audit_sig_pid as pid_t.

The code has been working good for a long time. I am wondering if the original 
intent was to make it general in case we decided to add more signals that we 
are interested in.

-Steve

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Richard Guy Briggs]

On 14/03/12, Steve Grubb wrote:
> On Tuesday, March 11, 2014 06:15:17 PM Richard Guy Briggs wrote:
> > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?
> 
> Well, pid=0 would be the kernel. So, its valid but unlikely. Offhand I don't 
> know why the kernel might try sending a signal. Is this a problem?

It isn't a problem.  It is a question of trying to understand it and how
much effort to code the existing behaviour.

> -Steve

- 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

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Richard Guy Briggs]

On 14/03/12, Steve Grubb wrote:
> On Tuesday, March 11, 2014 11:32:01 PM Richard Guy Briggs wrote:
> > On 14/03/11, Eric Paris wrote:
> > > On Tue, 2014-03-11 at 18:15 -0400, Richard Guy Briggs wrote:
> > > > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO
> > > > message?
> > > 
> > > No...
> > > 
> > > Given that userspace requests AUDIT_SIGNAL_INFO after it gets a signal,
> > > and that audit_sig_{uid,pid,...} get filled in when some task sent
> > > auditd that signal, the idea that the pid would be 0 doesn't make
> > > sense...  (unless auditd requests AUDIT_SIGNAL_INFO without getting a
> > > signal, but that's just dumb)
> > 
> > The reason I ask is that it is initialized to -1, which I assume is no
> > more valid than zero in your interpretation above.
> 
> pid=-1 has a special meaning for signals. But in terms of seeing it in a 
> sigaction handler for siginfo, not possible. So its a good init value. If you 
> look at sigaction(2), there is a si_code that indicates why the signal was 
> sent. One of them is SI_KERNEL. So, its possible that the kernel decides to 
> send a signal on certain occasions.

That message is only sent on request from userspace, so I suppose
userspace could request that information at any time, but the only time
it would be meaningful is after that userspace process has received a
signal.

> > I looked at converting audit_sig_pid from pid_t to struct pid *, but
> > then get_pid() would also be needed to protect that reference.  A
> > put_pid() would need to be done once it is no longer needed, which could
> > be immediately after it is read in the AUDIT_SIGNAL_INFO message
> > preparation, assuming it would never need to be read again.  If this
> > isn't the case, put_pid() could be called when audit_pid is nulled, but
> > if that message never comes, that struct pid is stuck with a stale
> > refcount.  (That isn't an issue if it is init or systemd, but it is
> > still wrong.)
> > 
> > This looks more and more like overkill and should probably leave
> > audit_sig_pid as pid_t.
> 
> The code has been working good for a long time. I am wondering if the original 
> intent was to make it general in case we decided to add more signals that we 
> are interested in.

Such as HUP to reread config or other possibilities?

> -Steve

- 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

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Steve Grubb]

On Wednesday, March 12, 2014 11:35:56 AM Richard Guy Briggs wrote:
> > pid=-1 has a special meaning for signals. But in terms of seeing it in a 
> > sigaction handler for siginfo, not possible. So its a good init value. If
> > you  look at sigaction(2), there is a si_code that indicates why the
> > signal was sent. One of them is SI_KERNEL. So, its possible that the
> > kernel decides to send a signal on certain occasions.
> 
> That message is only sent on request from userspace, so I suppose
> userspace could request that information at any time, but the only time
> it would be meaningful is after that userspace process has received a
> signal.

Sure.

> > > I looked at converting audit_sig_pid from pid_t to struct pid *, but
> > > then get_pid() would also be needed to protect that reference.  A
> > > put_pid() would need to be done once it is no longer needed, which could
> > > be immediately after it is read in the AUDIT_SIGNAL_INFO message
> > > preparation, assuming it would never need to be read again.  If this
> > > isn't the case, put_pid() could be called when audit_pid is nulled, but
> > > if that message never comes, that struct pid is stuck with a stale
> > > refcount.  (That isn't an issue if it is init or systemd, but it is
> > > still wrong.)
> > >
> > > This looks more and more like overkill and should probably leave
> > > audit_sig_pid as pid_t.
> >
> > The code has been working good for a long time. I am wondering if the
> > original  intent was to make it general in case we decided to add more
> > signals that we are interested in.
> 
> Such as HUP to reread config or other possibilities?

I think we started with sigterm. Then we needed sighup. Then needed usr1 & 
usr2. Somewhere along the way I think it was just decided to make it open 
ended in case more were needed later.

-Steve

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Richard Guy Briggs]

On 14/03/12, Steve Grubb wrote:
> On Wednesday, March 12, 2014 11:35:56 AM Richard Guy Briggs wrote:
> > > > I looked at converting audit_sig_pid from pid_t to struct pid *, but
> > > > then get_pid() would also be needed to protect that reference.  A
> > > > put_pid() would need to be done once it is no longer needed, which could
> > > > be immediately after it is read in the AUDIT_SIGNAL_INFO message
> > > > preparation, assuming it would never need to be read again.  If this
> > > > isn't the case, put_pid() could be called when audit_pid is nulled, but
> > > > if that message never comes, that struct pid is stuck with a stale
> > > > refcount.  (That isn't an issue if it is init or systemd, but it is
> > > > still wrong.)
> > > >
> > > > This looks more and more like overkill and should probably leave
> > > > audit_sig_pid as pid_t.
> > >
> > > The code has been working good for a long time. I am wondering if the
> > > original  intent was to make it general in case we decided to add more
> > > signals that we are interested in.
> > 
> > Such as HUP to reread config or other possibilities?
> 
> I think we started with sigterm. Then we needed sighup. Then needed usr1 & 
> usr2. Somewhere along the way I think it was just decided to make it open 
> ended in case more were needed later.

Currently, TERM, HUP, USR1 and USR2 but no others cause audit_sig_pid to be set.

Other signals only cause the target task to be added to the target list.

So what is the use case to send any other signal, causing the target to
only be added to the context target list?

> -Steve

- 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

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Eric Paris]

On Wed, 2014-03-12 at 08:22 -0400, Steve Grubb wrote:
> On Tuesday, March 11, 2014 06:15:17 PM Richard Guy Briggs wrote:
> > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?
> 
> Well, pid=0 would be the kernel. So, its valid but unlikely.

Actually, signals from the kernel will never get recorded here...

> Offhand I don't 
> know why the kernel might try sending a signal. Is this a problem?

He's trying to figure out how to store this info in light of pid
namespaces.  right now, auditd can only live in the initial pid
namespace, so can only get signals from processes in the initial pid
namespace, so we can store it as a number always in the initial pid
namespace.  But if auditd were ever to not be in the initial pid
namespace, not sure what to do....
> 
> -Steve
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
> 

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Richard Guy Briggs]

On 14/03/12, Eric Paris wrote:
> On Wed, 2014-03-12 at 08:22 -0400, Steve Grubb wrote:
> > On Tuesday, March 11, 2014 06:15:17 PM Richard Guy Briggs wrote:
> > > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?
> > 
> > Well, pid=0 would be the kernel. So, its valid but unlikely.
> 
> Actually, signals from the kernel will never get recorded here...
> 
> > Offhand I don't 
> > know why the kernel might try sending a signal. Is this a problem?
> 
> He's trying to figure out how to store this info in light of pid
> namespaces.  right now, auditd can only live in the initial pid
> namespace, so can only get signals from processes in the initial pid
> namespace, so we can store it as a number always in the initial pid
> namespace.  But if auditd were ever to not be in the initial pid
> namespace, not sure what to do....

Nice word column alignment above there Eric...  ;-)

I would be inclined to always store it in the initial pid namespace and
then make a decision if it translates sanely when needed to the auditd
namespace(s).

> > -Steve

- 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

Re: Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?

[Eric Paris]

On Wed, 2014-03-12 at 14:21 -0400, Richard Guy Briggs wrote:
> On 14/03/12, Eric Paris wrote:
> > On Wed, 2014-03-12 at 08:22 -0400, Steve Grubb wrote:
> > > On Tuesday, March 11, 2014 06:15:17 PM Richard Guy Briggs wrote:
> > > > Is zero a valid value for the pid member of the AUDIT_SIGNAL_INFO message?
> > > 
> > > Well, pid=0 would be the kernel. So, its valid but unlikely.
> > 
> > Actually, signals from the kernel will never get recorded here...
> > 
> > > Offhand I don't 
> > > know why the kernel might try sending a signal. Is this a problem?
> > 
> > He's trying to figure out how to store this info in light of pid
> > namespaces.  right now, auditd can only live in the initial pid
> > namespace, so can only get signals from processes in the initial pid
> > namespace, so we can store it as a number always in the initial pid
> > namespace.  But if auditd were ever to not be in the initial pid
> > namespace, not sure what to do....
> 
> Nice word column alignment above there Eric...  ;-)
> 
> I would be inclined to always store it in the initial pid namespace and
> then make a decision if it translates sanely when needed to the auditd
> namespace(s).

since for now auditd pid_ns == init pid_ns that's fine.