[PATCH] audit: ia32entry.S drops useful return value sign bits

[PATCH] audit: ia32entry.S drops useful return value sign bits

[Eric Paris]

In the ia32entry syscall exit audit fastpath we have assembly code which calls
audit_syscall_exit directly.  This code was, however, incorrectly zeroing
the upper 32 bits of the return code.  It then proceeded to do a 32bit check
for positive/negative to determine the syscalls success.  This meant that
syscalls like mmap2 which might return a very large 32 bit address as the
pointer would be mistaken for a negative return code.  It also meant that
negative return codes would be mistaken for 32 bit numbers on output.

The fix is to not zero the upper 32 bits of the return value and to do a full
64bit negative/postive determination for syscall success.

Old record returning a pointer:
type=SYSCALL msg=audit(1305733850.639:224): arch=40000003 syscall=192 success=no exit=4151844864
New Record with positive/negative test fixing "success":
type=SYSCALL msg=audit(1305733850.639:224): arch=40000003 syscall=192 success=yes exit=4151844864

Old record returning an error:
type=SYSCALL msg=audit(1306197182.256:281): arch=40000003 syscall=192 success=no exit=4294967283
New record returning -13:
type=SYSCALL msg=audit(1306197182.256:281): arch=40000003 syscall=192 success=no exit=-13

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

 arch/x86/ia32/ia32entry.S |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index c1870dd..b2bea0a 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -209,14 +209,14 @@ sysexit_from_sys_call:
 	jnz ia32_ret_from_sys_call
 	TRACE_IRQS_ON
 	sti
-	movl %eax,%esi		/* second arg, syscall return value */
-	cmpl $0,%eax		/* is it < 0? */
+	movq %rax,%rsi		/* second arg, syscall return value */
+	cmpq $0,%rax		/* is it < 0? */
 	setl %al		/* 1 if so, 0 if not */
 	movzbl %al,%edi		/* zero-extend that into %edi */
 	inc %edi /* first arg, 0->1(AUDITSC_SUCCESS), 1->2(AUDITSC_FAILURE) */
 	call audit_syscall_exit
 	GET_THREAD_INFO(%r10)
-	movl RAX-ARGOFFSET(%rsp),%eax	/* reload syscall return value */
+	movq RAX-ARGOFFSET(%rsp),%rax	/* reload syscall return value */
 	movl $(_TIF_ALLWORK_MASK & ~_TIF_SYSCALL_AUDIT),%edi
 	cli
 	TRACE_IRQS_OFF

Re: [PATCH] audit: ia32entry.S drops useful return value sign bits

[Eric Paris]

On 05/23/2011 08:50 PM, H. Peter Anvin wrote:
> On 05/23/2011 05:41 PM, Eric Paris wrote:
>> In the ia32entry syscall exit audit fastpath we have assembly code which calls
>> audit_syscall_exit directly.  This code was, however, incorrectly zeroing
>> the upper 32 bits of the return code.  It then proceeded to do a 32bit check
>> for positive/negative to determine the syscalls success.  This meant that
>> syscalls like mmap2 which might return a very large 32 bit address as the
>> pointer would be mistaken for a negative return code.  It also meant that
>> negative return codes would be mistaken for 32 bit numbers on output.
>>
>> The fix is to not zero the upper 32 bits of the return value and to do a full
>> 64bit negative/postive determination for syscall success.
>>
> 
> I'm not sure that's correct.
> 
> I would argue that the fix is do a 32-bit comparison.  Comparing the
> sign value is wrong, anyway: error is indicated by a value in the range
> -4095 to -1, so to match userspace expectations it should be:
> 
> 	cmpl	$-4095, %eax
> 	setae	%al
> 
> 	-hpa

I actually have a followup patch which does this, but which
simultaneously fixes all arches.  It's bigger than this specific problem
though.  I guess we can see this as 2 problems.

1) The audit_syscall_exit function expects a long.  But if you chop off
the upper 32 bits you can't tell positive from negative.  Thus when it
prints to userspace using %ld we have a problem:
  Aka printf("%ld", (long)(u32)(-13)) = "4294967283"
   vs printf("%ld", (long)(-13)) = "-13"
2) The current code uses pos/neg to determine 'success' on many arches.
 It should use >= -MAX_ERRNO

I had patches briefly which truncated rax to 32 bits.  Checked if it was
an errno, then if so, sign extended it back to 64 bits for the call to
audit_syscall_exit.  Although that logically made sense it wasted
instructions doing truncation of rax->eax and then sometimes expansion
back when rax had the right upper 32 bits all along.

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index b2bea0a..d9170de 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -14,6 +14,7 @@
 #include <asm/segment.h>
 #include <asm/irqflags.h>
 #include <linux/linkage.h>
+#include <linux/err.h>

 /* Avoid __ASSEMBLER__'ifying <linux/audit.h> just for this.  */
 #include <linux/elf-em.h>
@@ -210,11 +211,10 @@ sysexit_from_sys_call:
        TRACE_IRQS_ON
        sti
        movq %rax,%rsi          /* second arg, syscall return value */
-       cmpq $0,%rax            /* is it < 0? */
+       cmpq $-MAX_ERRNO,%rax   /* is it < 0? */

Re: [PATCH] audit: ia32entry.S drops useful return value sign bits

[Eric Paris]

On 05/23/2011 09:19 PM, H. Peter Anvin wrote:
> On 05/23/2011 06:04 PM, Eric Paris wrote:
>> 1) The audit_syscall_exit function expects a long.  But if you chop off
>> the upper 32 bits you can't tell positive from negative.  Thus when it
>> prints to userspace using %ld we have a problem:
>>   Aka printf("%ld", (long)(u32)(-13)) = "4294967283"
>>    vs printf("%ld", (long)(-13)) = "-13"
> 
> This seems like the fundamental design error.

Possibly so (I'm not convinced), but not a fixable problem given the bug
for bug compatibility requirements of the kernel.  The syscall return
value (either rax or eax) is passed to audit_syscall_exit() which
believes it is a long (aka s64).  It builds a string buffer using
sprintf("%ld") and then exports that buffer to userspace via a netlink
socket.  That buffer gets dumped as a raw string into a file.  Some
tools may later process the strings.  Getting the right string into the
netlink socket is what I consider the unchangeable ABI.  Prior to
5cbf1565f29eb57a this was all handled by normal 64bit C code which did
exactly what I'm describing here.  It never needlessly truncated the
return code to 32 bits on ia32exit.  Solving that regression is what I'm
fixing.

> You're missing something fundamental: if userspace is 32 bits, those
> bits don't even exist.  If userspace is 64 bits (and it is possible for
> a 64-bit process to call the 32-bit entry point) those bits could at
> least theoretically contain bad information.

This is at syscall exit, in 64bit mode, so rax is going to contain a
64bit version of the return code.  I'm not trying to hand 64 bit values
back to a 32 bit process.  The code converts the return value using %ld
and then dumps it as a string to auditd.  Even if auditd was 32bit, it's
not processing the string, just writing it to a file.

> It sounds like this code is broken in some very fundamental ways, and
> that you're trying to paper it over.

Obviously we agree there is a second problem not addressed in this patch
(that many arches uses +/- instead of >=-MAX_ERRNO) but the fact that we
have a regression in which the assembly removes the sign and then passes
the now truncated value to a function expecting a long is the problem of
this patch.

I could paper over the problem in the audit code, doing my own sign
craziness based on the arch, but I think the real problem is in the
assembly dropping information needlessly.....

-Eric

Re: [PATCH] audit: ia32entry.S drops useful return value sign bits

[Eric Paris]

On Tue, 2011-05-24 at 09:18 -0700, H. Peter Anvin wrote:
> On 05/24/2011 06:55 AM, Thomas Gleixner wrote:
> >> This seems like the fundamental design error.
> > 
> > I don't think so. We run in 64bit mode here and call into 64bit code
> > which expects a long being 64bit and not a 32bit truncated value. The
> > audit code is pure kernel stuff and this is not the return to
> > userspace.
> 
> I don't agree, this is about auditing the return to userspace.  For the
> IA32 entry point, the return value is a 32-bit value even if we happen
> to return to 64-bit userspace.  Treating it as anything else is asking
> for a security hole when we audit something that isn't what we do.
> 
> As such, the right thing to do is probably:
> 
> 	movl	%eax, %esi
> 	cmpl	$-MAX_ERRNO, %eax
> 	jb	1f
> 	movslq	%eax, %rsi
> 1:	setae	%al

I'll do it that way if you want.  But you now have an extra jb and an
extra movl, neither of which do anything at all.  It's no different than

movq		%rax, %rsi
cmp{q,l}	$-MAX_ERRNO, %{r,e}ax
setae		%al

I know it's the same because I spent forever trying to hunt down movslq.
I don't understand why it's not in the Intel® 64 and IA-32 Architectures
Software Developer’s Manual Volume 2 (2A & 2B): Instruction Set
Reference, A-Z.  That's exactly what I talked about, truncating the
upper 32 bits just the sign extend them right back.

I guess it comes down to picking one of these 3:
My version:
        movq %rax,%rsi          /* second arg, syscall return value */
        cmpl $-MAX_ERRNO,%rax   /* is it < 0? */
        setbe %al                /* 1 if so, 0 if not */
        movzbl %al,%edi         /* zero-extend that into %edi */
        call __audit_syscall_exit

VS hpa version:
	movl	%eax,%esi	/* move 32bits to second arg */
	cmpl	$-MAX_ERRNO,%eax /* check if fail */
	jbe	1f
	movslq	%eax, %rsi	/* re-sign-extend eax */
1:	setbe	%al
	movzbl %al,%edi
	call __audit_syscall_exit

VS alternate of hpa version without set:
	movl	$1,%edi		/* syscall success argument */
	movl	%eax,%esi	/* move 32bits to second arg */
	cmpl	$-MAX_ERRNO,%eax /* check if fail */
	jbe	1f
	xor	%edi,%edi	/* syscall failure argument */
	movslq	%eax, %rsi	/* resign-extend eax */
1:	call __audit_syscall_exit

If I have to go with the hpa version of truncation followed by sign
extension, is it any better/cheap/faster to use just movl in the
'common' case and movl+xor in the uncommon syscall failure?  I don't
know how expensive or large the set+movzbl is....

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit