fix broken class-based syscall audit

Bug description: When I add an audit watch on a file with no arguments, I get perm=rwxa but on ia64, changes to the mode and context aren't audited. I get audit records on i386 and x86_64.

See also: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=239887

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.

This breaks class-based audit for all syscalls on ia64 since on that architecture syscall numbers start at 1024. It breaks some syscall audit on other architectures also, for example __NR_fchmodat is 306 on x86.

I'd suggest adding a printk() in addition to returning 0 - you don't want to silently ignore unknown or unsupported syscalls when auditing.

Signed-off-by Klaus Weidner <[email protected]>

Quoted 4 lines · Klaus Weidner

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.

Quoted 4 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +0 -0

@@ -306,7 +306,7 @@
 

kernel/auditfilter.c line 312

Quoted 7 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +1 -1

 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.

Quoted 5 lines · Marcus Meissner

Quoted 4 lines · Klaus Weidner

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.

[...]

Quoted 4 lines

Quoted 4 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +0 -0

@@ -306,7 +306,7 @@
 

Quoted 9 lines

kernel/auditfilter.c line 312

Quoted 7 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +1 -1

 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.

Quoted 21 lines · Klaus Weidner

Quoted 5 lines · Marcus Meissner

Quoted 4 lines · Klaus Weidner

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.

[...]

Quoted 4 lines

Quoted 4 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +0 -0

@@ -306,7 +306,7 @@
 

Quoted 9 lines

kernel/auditfilter.c line 312

Quoted 7 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +1 -1

 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.

Quoted 3 lines

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....

Quoted 26 lines · Eric Paris

Quoted 21 lines · Klaus Weidner

Quoted 5 lines · Marcus Meissner

Quoted 4 lines · Klaus Weidner

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.

[...]

Quoted 4 lines

Quoted 4 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +0 -0

@@ -306,7 +306,7 @@
 

Quoted 9 lines

kernel/auditfilter.c line 312

Quoted 7 lines

kernel/auditfilter.c renamed from kernel/auditfilter.c.lspp.80 +1 -1

 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.

Quoted 3 lines

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.

Quoted 5 lines

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.