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: Tue, 24 May 2011 09:13:01 -0400 Message-ID: <4DDBAEDD.5070703@redhat.com> References: <20110524004135.6110.61381.stgit@paris.rdu.redhat.com> <4DDB00CC.1050802@zytor.com> <4DDB040B.4050404@redhat.com> <4DDB07B2.2080400@zytor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4DDB07B2.2080400@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 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