From mboxrd@z Thu Jan 1 00:00:00 1970 From: Klaus Weidner Subject: Re: [PATCH] fix broken class-based syscall audit Date: Mon, 14 May 2007 16:32:22 -0500 Message-ID: <20070514213222.GC11536@w-m-p.com> References: <20070514154636.GA11536@w-m-p.com> <20070514155148.GB23829@suse.de> <20070514155608.GB11536@w-m-p.com> <1179175649.3964.5.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mx1.redhat.com (mx1.redhat.com [172.16.48.31]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id l4ELWX38021772 for ; Mon, 14 May 2007 17:32:33 -0400 Received: from mail.atsec.com (mail.atsec.com [195.30.252.105]) by mx1.redhat.com (8.13.1/8.13.1) with ESMTP id l4ELWVHN008591 for ; Mon, 14 May 2007 17:32:31 -0400 Content-Disposition: inline In-Reply-To: <1179175649.3964.5.camel@localhost.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Eric Paris Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com On Mon, May 14, 2007 at 04:47:29PM -0400, Eric Paris wrote: > On Mon, 2007-05-14 at 10:56 -0500, Klaus Weidner wrote: > > On Mon, May 14, 2007 at 05:51:50PM +0200, Marcus Meissner wrote: > > > On Mon, May 14, 2007 at 10:46:36AM -0500, Klaus Weidner wrote: > > > > The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is > > > > 64, providing space for 2048 syscalls in 64 * 32bit integers. The > > > > comparison only supports 256 syscalls (sizeof __u32 is 4), and silently > > > > returns "no match" for valid higher-numbered syscalls. > > [...] > > > > --- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80 2007-05-11 17:06:08.000000000 -0500 > > > > +++ linux-2.6.18.i686/kernel/auditfilter.c 2007-05-11 17:09:37.000000000 -0500 > > > > @@ -306,7 +306,7 @@ > > > > > > > > int audit_match_class(int class, unsigned syscall) > > > > { > > > > - if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32))) > > > > + if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32)) > > > > return 0; > > > > if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class])) > > > > return 0; > > > > > > You likely need to fix audit_register_class() if this is true. > > > > I don't see a problem in audit_register_class() - it correctly uses > > sizeof(__u32) for allocating the memory since that's counted in bytes, > > only the comparison needs to count bits. > > If that's the problem wouldn't we be better to use sizeof(__u32) *8 > rather than hard code the 32 in there? (sidebar: is it portable to > assume sizeof() returns bytes and *8 is the right way to go on all > archs?) That would make it easier to do u32 search/replace in the > future if we ever have to grow this stuff.... The AUDIT_WORD and AUDIT_BIT macros in include/linux/audit.h assume that they'll fit 32 bits into each __u32: #define AUDIT_WORD(nr) ((__u32)((nr)/32)) #define AUDIT_BIT(nr) (1 << ((nr) - AUDIT_WORD(nr)*32)) (If the actual capacity of the __u32 were larger they would continue using only 32 bits.) I think the hardcoded 32 is appropriate. I'm not sure about the exact sizeof() standard semantics, but I remember that the standard doesn't assume 8-bit bytes. However, I think it's safe to assume that a __u32 can fit 32 bits, if not a lot of other code would break also. -Klaus