[PATCH] Audit of POSIX Message Queue Syscalls
Steve Grubb
sgrubb at redhat.com
Wed May 17 13:34:46 UTC 2006
On Tuesday 16 May 2006 21:40, George C. Wilson wrote:
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>
> #include <linux/sched.h>
> #include <linux/elf.h>
> +#include <linux/mqueue.h>
Do we really need the header or just a forward declaration for mq structs?
> @@ -85,6 +86,10 @@
> #define AUDIT_CWD 1307 /* Current working directory */
> #define AUDIT_EXECVE 1309 /* execve arguments */
> #define AUDIT_IPC_SET_PERM 1311 /* IPC new permissions record type */
> +#define AUDIT_MQ_OPEN 1312 /* POSIX MQ open record type */
> +#define AUDIT_MQ_SENDRECV 1313 /* POSIX MQ send/receive record type */
> +#define AUDIT_MQ_NOTIFY 1314 /* POSIX MQ notify record type */
> +#define AUDIT_MQ_GETSETATTR 1315 /* POSIX MQ get/set attribute record type
Do we really need 4 message types? IPC was able to get by on 2 of them. Are we
recording more than IPC?
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -1087,7 +1102,7 @@ asmlinkage long sys_mq_getsetattr(mqd_t
> const struct mq_attr __user *u_mqstat,
> struct mq_attr __user *u_omqstat)
> {
> - int ret;
> + int ret, audret;
> struct mq_attr mqstat, omqstat;
> struct file *filp;
> struct inode *inode;
> @@ -1130,9 +1145,13 @@ asmlinkage long sys_mq_getsetattr(mqd_t
> sizeof(struct mq_attr)))
> ret = -EFAULT;
>
> +
Is this additional space really needed?
> out_fput:
> fput(filp);
> out:
> + audret = audit_mq_getsetattr(mqdes, &mqstat, &omqstat);
> + if (ret == 0)
> + ret = audret;
> return ret;
> }
what if u_mqstat==NULL and the fd was bad upon syscall entry? All the data
structures are whatever the stack contained, and therefore completely bogus.
I think there are a couple ways that the structs could be uninitialized.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index c30f146..2fdead5 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -709,6 +740,46 @@ static void audit_log_exit(struct audit_
> continue; /* audit_panic has been called */
>
> switch (aux->type) {
> + case AUDIT_MQ_OPEN: {
> + struct audit_aux_data_mq_open *axi = (void *)aux;
> + audit_log_format(ab,
> + "oflag=0x%x mode=%#o mq_flags=0x%lx mq_maxmsg=%ld "
> + "mq_msgsize=%ld mq_curmsgs=%ld",
> + axi->oflag, axi->mode, axi->attr.mq_flags,
> + axi->attr.mq_maxmsg, axi->attr.mq_msgsize,
Do we really need to record the size? Is there anything here that's not sec
relevant? The same comment applies to the next 3 message types.
> @@ -1242,6 +1313,187 @@ uid_t audit_get_loginuid(struct audit_co
> }
>
> /**
> + * audit_mq_open - record audit data for a POSIX MQ open
> + * @oflag: open flag
> + * @mode: mode bits
> + * @u_attr: queue attributes
> + *
> + * Returns 0 for success or NULL context or < 0 on error.
> + */
> +int audit_mq_open(int oflag, mode_t mode, struct mq_attr __user *u_attr)
> +{
> + struct audit_aux_data_mq_open *ax;
> + struct audit_context *context = current->audit_context;
> +
> + if (likely(!context))
> + return 0;
What if audit is not enabled? Need to check for it and bail out.
> +
> + ax = kmalloc(sizeof(*ax), GFP_ATOMIC);
> + if (!ax)
> + return -ENOMEM;
> +
> + if (u_attr != NULL) {
> + if (copy_from_user(&ax->attr, u_attr, sizeof(ax->attr)))
> + return -EFAULT;
memory leak from above allocation.
> +int audit_mq_timedsend(mqd_t mqdes, size_t msg_len, unsigned int msg_prio,
> + const struct timespec __user *u_abs_timeout)
> +{
> + struct audit_aux_data_mq_sendrecv *ax;
> + struct audit_context *context = current->audit_context;
> +
> + if (likely(!context))
> + return 0;
audit_enabled?
> + ax = kmalloc(sizeof(*ax), GFP_ATOMIC);
> + if (!ax)
> + return -ENOMEM;
> +
> + if (u_abs_timeout != NULL) {
> + if (copy_from_user(&ax->abs_timeout, u_abs_timeout,
> sizeof(ax->abs_timeout))) + return -EFAULT;
memleak
> +int audit_mq_timedreceive(mqd_t mqdes, size_t msg_len,
> + unsigned int __user *u_msg_prio,
> + const struct timespec __user *u_abs_timeout)
> +{
> + struct audit_aux_data_mq_sendrecv *ax;
> + struct audit_context *context = current->audit_context;
> +
> + if (likely(!context))
> + return 0;
audit_enabled?
> + ax = kmalloc(sizeof(*ax), GFP_ATOMIC);
> + if (!ax)
> + return -ENOMEM;
> +
> + if (u_msg_prio != NULL) {
> + if (get_user(ax->msg_prio, u_msg_prio))
> + return -EFAULT;
memleak
> + } else
> + ax->msg_prio = 0;
> +
> + if (u_abs_timeout != NULL) {
> + if (copy_from_user(&ax->abs_timeout, u_abs_timeout,
> sizeof(ax->abs_timeout))) + return -EFAULT;
memleak
> +int audit_mq_notify(mqd_t mqdes, const struct sigevent __user
> *u_notification) +{
> + struct audit_aux_data_mq_notify *ax;
> + struct audit_context *context = current->audit_context;
> +
> + if (likely(!context))
> + return 0;
audit_enabled?
> + ax = kmalloc(sizeof(*ax), GFP_ATOMIC);
> + if (!ax)
> + return -ENOMEM;
> +
> + if (u_notification != NULL) {
> + if (copy_from_user(&ax->notification, u_notification,
> sizeof(ax->notification))) + return -EFAULT;
memleak
> +int audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat, struct
> mq_attr *omqstat) +{
> + struct audit_aux_data_mq_getsetattr *ax;
> + struct audit_context *context = current->audit_context;
> +
> + if (likely(!context))
> + return 0;
audit_enabled?
-Steve
More information about the Linux-audit
mailing list