[PATCH] Audit: EINTR instead of kernel private return codes in audit records

[PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Eric Paris]

When a syscall gets interrupted by a signal and that signal is set to
not restart the syscall its return code will get collected by the audit
system before the registers are changed to the userspace valid EINTR;
See the discussion in include/linux/errno.h

Thus it is possible to get a syscall audit such as:

type=SYSCALL msg=audit(11/13/2007 23:47:34.648:80314) : arch=x86_64
syscall=accept success=no exit=-512(Unknown error 512) a0=3 [snip]

with this patch we clean up those kernel only return codes and give the
userspace equivalent.

type=SYSCALL msg=audit(11/13/2007 23:06:04.017:898) : arch=x86_64
syscall=accept success=no exit=-4(Interrupted system call) a0=3 [snip]

Signed-off-by: Eric Paris <eparis@redhat.com>

---

 kernel/auditsc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index bce9ecd..447ad65 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -702,7 +702,14 @@ static inline struct audit_context *audit_get_context(struct task_struct *tsk,
 	if (likely(!context))
 		return NULL;
 	context->return_valid = return_valid;
-	context->return_code  = return_code;
+
+	if (unlikely((return_code == -ERESTART_RESTARTBLOCK) ||
+		     (return_code == -ERESTARTNOHAND) ||
+		     (return_code == -ERESTARTSYS) ||
+		     (return_code == -ERESTARTNOINTR)))
+		context->return_code = -EINTR;
+	else
+		context->return_code  = return_code;
 
 	if (context->in_syscall && !context->dummy && !context->auditable) {
 		enum audit_state state;

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Paul Moore]

On Wednesday 14 November 2007 3:22:08 pm Eric Paris wrote:
> When a syscall gets interrupted by a signal and that signal is set to
> not restart the syscall its return code will get collected by the audit
> system before the registers are changed to the userspace valid EINTR;
> See the discussion in include/linux/errno.h

I have no idea if the fix is the "right way" of dealing with the problem (I'll 
let the audit experts vote on that), but thanks for looking into the problem 
and coming up with a possible solution.

-- 
paul moore
linux security @ hp

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Steve Grubb]

On Wednesday 14 November 2007 15:22:08 Eric Paris wrote:
> +       if (unlikely((return_code == -ERESTART_RESTARTBLOCK) ||
> +                    (return_code == -ERESTARTNOHAND) ||
> +                    (return_code == -ERESTARTSYS) ||
> +                    (return_code == -ERESTARTNOINTR)))

Would it be more efficient to say:


if (unlikely(return_code <= -ERESTARTSYS && 
               return_code >= -ERESTART_RESTARTBLOCK))

That gets it down to 2 compares and 1 logical op.

-Steve

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Eric Paris]

On Wed, 2007-11-14 at 15:30 -0500, Steve Grubb wrote:
> On Wednesday 14 November 2007 15:22:08 Eric Paris wrote:
> > +       if (unlikely((return_code == -ERESTART_RESTARTBLOCK) ||
> > +                    (return_code == -ERESTARTNOHAND) ||
> > +                    (return_code == -ERESTARTSYS) ||
> > +                    (return_code == -ERESTARTNOINTR)))
> 
> Would it be more efficient to say:
> 
> 
> if (unlikely(return_code <= -ERESTARTSYS && 
>                return_code >= -ERESTART_RESTARTBLOCK))
> 
> That gets it down to 2 compares and 1 logical op.
> 
> -Steve

It should be slightly faster (although already on the unlikely() path so
we are tuning the highly unlikely bad perf path anyway, remember by
default linux restarts syscalls when receiving a signal and I don't know
how many people actually change this)

We would also be picking up ENOIOCTLCMD but that shoudln't be seen on
this code path, so I guess it doesn't matter.

Al, thoughts?

-Eric

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Miloslav Trmac]

Steve Grubb napsal(a):
> On Wednesday 14 November 2007 15:22:08 Eric Paris wrote:
>> +       if (unlikely((return_code == -ERESTART_RESTARTBLOCK) ||
>> +                    (return_code == -ERESTARTNOHAND) ||
>> +                    (return_code == -ERESTARTSYS) ||
>> +                    (return_code == -ERESTARTNOINTR)))
> 
> Would it be more efficient to say:
> 
> if (unlikely(return_code <= -ERESTARTSYS && 
>                return_code >= -ERESTART_RESTARTBLOCK))
> 
> That gets it down to 2 compares and 1 logical op.
gcc performs this transformation automatically.
	Mirek

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Steve Grubb]

On Wednesday 14 November 2007 16:07:42 Eric Paris wrote:
> It should be slightly faster <snip>
>
> We would also be picking up ENOIOCTLCMD but that shoudln't be seen on
> this code path, so I guess it doesn't matter.

Right, the note in the top of include/linux/errno.h says these should not be 
seen by userspace. So, ENOIOCTLCMD is invalid if we ever saw it. But what 
about the NFSv3 errnos (in the same file)? Do we need to worry about those?

Thanks,
-Steve

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Eric Paris]

On Wed, 2007-11-14 at 16:17 -0500, Steve Grubb wrote:
> On Wednesday 14 November 2007 16:07:42 Eric Paris wrote:
> > It should be slightly faster <snip>
> >
> > We would also be picking up ENOIOCTLCMD but that shoudln't be seen on
> > this code path, so I guess it doesn't matter.
> 
> Right, the note in the top of include/linux/errno.h says these should not be 
> seen by userspace. So, ENOIOCTLCMD is invalid if we ever saw it. But what 
> about the NFSv3 errnos (in the same file)? Do we need to worry about those?

So I started looking at these and I like my patch the way I sent it.
Although ENOIOCTLCMD, ENOTSUPP, ETOOSMALL, ESERVERFAULT, EIOCBQUEUED,
and EIOCBRETRY are not supposed to be valid return codes first glance
leads me to believe they could all slip back into userspace.  None of
them are later rewritten after we collect the return code in the audit
system.  It would be absolutely wrong for the audit system to translate
even ENOIOCTLCMD to anything else if this is what was actually returned
to the process.

I suggest the patch be added exactly the way I sent it, as the speed up
suggested (using >= && <=) could give incorrect audit messages vs what
actually happened.

-Eric

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Steve Grubb]

On Saturday 17 November 2007 08:58:33 pm Eric Paris wrote:
> It would be absolutely wrong for the audit system to translate
> even ENOIOCTLCMD to anything else if this is what was actually returned
> to the process.

The point is, none of the errnos in that file should *ever* go to user space. 
glibc has no translation (strerror()) for them because they are internal to 
the kernel. So, they need to get translated to something. My only question is 
would these extra ones ever get picked up in this code path?

-Steve

Re: [PATCH] Audit: EINTR instead of kernel private return codes in audit records

[Eric Paris]

On Sun, 2007-11-18 at 05:58 -0500, Steve Grubb wrote:
> On Saturday 17 November 2007 08:58:33 pm Eric Paris wrote:
> > It would be absolutely wrong for the audit system to translate
> > even ENOIOCTLCMD to anything else if this is what was actually returned
> > to the process.
> 
> The point is, none of the errnos in that file should *ever* go to user space. 
> glibc has no translation (strerror()) for them because they are internal to 
> the kernel. So, they need to get translated to something. My only question is 
> would these extra ones ever get picked up in this code path?

They "should not" get returned to userspace.  I agree.  That doesn't
mean they don't.  All occurences of these error codes being returned to
userspace are bugs, but that has nothing at all to do with this
conversation.  My patch is correct, complete, and the right thing to do.
All syscalls are going to come through here and if they have an
'invalid' return code we need to just audit it, not audit the wrong
thing and then return the 'real' retval to userspace.  Remember this
patch isn't changing what goes back to userspace, its just changing what
we put in the audit log.  Look in any arch/signal.c to see why the 4 I
am dealing with are special and why it is right to audit EINTR even when
at this point it looks like ERESTART*.  

The 4 return codes I am dealing with are changed to be EINTR before they
get back to userspace, thus we should audit EINTR, not ERESTART*.  If
the kernel has a bug which returns ETOOSMALL to userspace the audit
system needs to audit ETOOSMALL, its as simple as that.

A crusade to clean up all of the issues with the 6 things I mentioned in
the last e-mail which may be slipping back to userspace would be a good
thing, heck, I even cleanup up the issue in the audit/selinux system
when they missused ENOTSUPP in a seperate patch, but lying about any of
the 'invalid' return codes in the audit wrong, even if it is a bug they
are there in the first place is flat wrong.

Make sense?

-Eric