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

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]

---

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

index bce9ecd..447ad65 100644
@@ -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;

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

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

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

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

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

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

Steve Grubb napsal(a):

gcc performs this transformation automatically.
Mirek