From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Paris Subject: Re: [PATCH] audit: ia32entry.S drops useful return value sign bits Date: Mon, 23 May 2011 21:04:11 -0400 Message-ID: <4DDB040B.4050404@redhat.com> References: <20110524004135.6110.61381.stgit@paris.rdu.redhat.com> <4DDB00CC.1050802@zytor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4DDB00CC.1050802@zytor.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: "H. Peter Anvin" Cc: x86@kernel.org, tglx@linutronix.de, linux-audit@redhat.com, mingo@redhat.com, viro@zeniv.linux.org.uk List-Id: linux-audit@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 #include #include +#include /* Avoid __ASSEMBLER__'ifying just for this. */ #include @@ -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? */