Skip to contentSkip to conversation

sanitize AUDIT_MQ_SENDRECV

PATCH In review Al Viro 4 participants 5 messages Dec 17, 2008 Audit

lore
Share & tools
lore

Al Viro

author
#0 lore raw

PATCH 7/15 sanitize AUDIT_MQ_SENDRECV

* logging the original value of *msg_prio in mq_timedreceive(2) is insane - the argument is write-only (i.e. syscall always ignores the original value and only overwrites it).
* merge __audit_mq_timed{send,receive}
* don't do copy_from_user() twice
* don't mess with allocations in auditsc part
* ... and don't bother checking !audit_enabled and !context in there - we'd already checked for audit_dummy_context().

Signed-off-by Al Viro <[email protected]>
---
 include/linux/audit.h |   17 ++-----
 ipc/mqueue.c          |   54 ++++++++++++---------
 kernel/auditsc.c      |  127 +++++++++++-------------------------------------
 3 files changed, 63 insertions(+), 135 deletions(-)
include/linux/audit.h +4 -13
index b82b927..dd3f62c 100644
@@ -449,8 +449,7 @@ extern int audit_sockaddr(int len, void *addr);
 extern int __audit_fd_pair(int fd1, int fd2);
 extern int audit_set_macxattr(const char *name);
 extern int __audit_mq_open(int oflag, mode_t mode, struct mq_attr __user *u_attr);
-extern int __audit_mq_timedsend(mqd_t mqdes, size_t msg_len, unsigned int msg_prio, const struct timespec __user *u_abs_timeout);
-extern int __audit_mq_timedreceive(mqd_t mqdes, size_t msg_len, unsigned int __user *u_msg_prio, const struct timespec __user *u_abs_timeout);
+extern void __audit_mq_sendrecv(mqd_t mqdes, size_t msg_len, unsigned int msg_prio, const struct timespec *abs_timeout);
 extern void __audit_mq_notify(mqd_t mqdes, const struct sigevent *notification);
 extern void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat);
 

@@ -476,17 +475,10 @@ static inline int audit_mq_open(int oflag, mode_t mode, struct mq_attr __user *u
 		return __audit_mq_open(oflag, mode, u_attr);
 	return 0;
 }
-static inline int audit_mq_timedsend(mqd_t mqdes, size_t msg_len, unsigned int msg_prio, const struct timespec __user *u_abs_timeout)
+static inline void audit_mq_sendrecv(mqd_t mqdes, size_t msg_len, unsigned int msg_prio, const struct timespec *abs_timeout)
 {
 	if (unlikely(!audit_dummy_context()))
-		return __audit_mq_timedsend(mqdes, msg_len, msg_prio, u_abs_timeout);
-	return 0;
-}
-static inline int audit_mq_timedreceive(mqd_t mqdes, size_t msg_len, unsigned int __user *u_msg_prio, const struct timespec __user *u_abs_timeout)
-{
-	if (unlikely(!audit_dummy_context()))
-		return __audit_mq_timedreceive(mqdes, msg_len, u_msg_prio, u_abs_timeout);
-	return 0;
+		__audit_mq_sendrecv(mqdes, msg_len, msg_prio, abs_timeout);
 }
 static inline void audit_mq_notify(mqd_t mqdes, const struct sigevent *notification)
 {

@@ -526,8 +518,7 @@ extern int audit_signals;
 #define audit_sockaddr(len, addr) ({ 0; })
 #define audit_set_macxattr(n) do { ; } while (0)
 #define audit_mq_open(o,m,a) ({ 0; })
-#define audit_mq_timedsend(d,l,p,t) ({ 0; })
-#define audit_mq_timedreceive(d,l,p,t) ({ 0; })
+#define audit_mq_sendrecv(d,l,p,t) ((void)0)
 #define audit_mq_notify(d,n) ((void)0)
 #define audit_mq_getsetattr(d,s) ((void)0)
 #define audit_ptrace(t) ((void)0)
ipc/mqueue.c +30 -24

90 lines not on this page · raw message

kernel/auditsc.c +29 -98

167 lines not on this page · raw message

Signature · 1 line
1.5.6.5
Reviewed-by on v1 7/15
#1 lore raw
Quoted 4 lines · Al Viro

* logging the original value of *msg_prio in mq_timedreceive(2) is insane - the argument is write-only (i.e. syscall always ignores the original value and only overwrites it).

I wonder if any existing certifications need to be amended.

Quoted 7 lines

* merge __audit_mq_timed{send,receive}
* don't do copy_from_user() twice
* don't mess with allocations in auditsc part
* ... and don't bother checking !audit_enabled and !context in there - we'd already checked for audit_dummy_context().

Signed-off-by Al Viro <[email protected]>
Reviewed-by James Morris <[email protected]>
Signature · 2 lines
James Morris
<[email protected]>
#2 lore raw
Quoted 8 lines · James Morris
Quoted 4 lines · Al Viro

* logging the original value of *msg_prio in mq_timedreceive(2) is insane - the argument is write-only (i.e. syscall always ignores the original value and only overwrites it).

I wonder if any existing certifications need to be amended.

I rather doubt it, but I really don't know the bureaucracy of that. Just to clarify:
Old code logs the value of *msg_prio; that value is always
ignored by syscall (it may store something there, but that's it - pure output parameter)
New code does not change the format of log message; the value
in that field of the message is replaced with 0. That's the only difference.

Whether that requires amending of the existing certs... bugger if I know.

#3 lore raw
In reply to Al Viro
Quoted 12 lines · On Wed, 17 Dec 2008 05:12:10 +0000 Al Viro <[email protected]> wrote:

* logging the original value of *msg_prio in mq_timedreceive(2) is insane - the argument is write-only (i.e. syscall always ignores the original value and only overwrites it).
* merge __audit_mq_timed{send,receive}
* don't do copy_from_user() twice
* don't mess with allocations in auditsc part
* ... and don't bother checking !audit_enabled and !context in there - we'd already checked for audit_dummy_context().

...

Review

Quoted 2 lines
(quoted diff) +2 -0
+		if (copy_from_user(&ts, u_abs_timeout, 
+					sizeof(struct timespec)))

Well that's odd. afacit timespec has different sizes on 32-bit and 64-bit userspace.

But the old audit code did this copy too, and it presumably worked.

#4 lore raw
Quoted 7 lines · Andrew Morton

Review

Quoted 2 lines
(quoted diff) +2 -0
+		if (copy_from_user(&ts, u_abs_timeout, 
+					sizeof(struct timespec)))

Well that's odd. afacit timespec has different sizes on 32-bit and 64-bit userspace.

But the old audit code did this copy too, and it presumably worked.

compat_mq_timedsend() et.al. essentially do copy_in_user() converting to 64bit and theh call sys_mq_...(). So you get kernel-native layout in the log.

Similar for notification in compat_mq_notify(), etc.