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

[Eric Paris <eparis@redhat.com>]

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? */