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

Eric Paris
2011-05-24T01:04:11Z

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