From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Grubb Subject: Re: [PATCH] Audit of POSIX Message Queue Syscalls Date: Wed, 17 May 2006 09:34:46 -0400 Message-ID: <200605170934.46091.sgrubb@redhat.com> References: <20060517014055.GA16852@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20060517014055.GA16852@us.ibm.com> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: linux-audit@redhat.com List-Id: linux-audit@redhat.com On Tuesday 16 May 2006 21:40, George C. Wilson wrote: > +++ b/include/linux/audit.h > @@ -26,6 +26,7 @@ > > #include > #include > +#include 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