Re: [PATCH] Audit of POSIX Message Queue Syscalls

[Steve Grubb <sgrubb@redhat.com>]

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