Re: [PATCH] Audit of POSIX Message Queue Syscalls

["Timothy R. Chavez" <tinytim@us.ibm.com>]

Hey George,

Just some quick things.  I didn't want to reproduce any of the things
Steve pointed out already... These are just general comments...

-tim

On Tue, 2006-05-16 at 20:40 -0500, George C. Wilson wrote:
<snip>
> @@ -660,6 +663,9 @@ asmlinkage long sys_mq_open(const char _
>  	if (IS_ERR(name = getname(u_name)))
>  		return PTR_ERR(name);
>  
> +	if ((error = audit_mq_open(oflag, mode, u_attr)) != 0)
> +		return error;
> +
>  	fd = get_unused_fd();
>  	if (fd < 0)
>  		goto out_putname;
> @@ -814,6 +820,9 @@ asmlinkage long sys_mq_timedsend(mqd_t m
>  	long timeout;
>  	int ret;
>  
> +	if ((ret = audit_mq_timedsend(mqdes, msg_len, msg_prio, u_abs_timeout)) != 0)
> +		return ret;
> +

Take the assignment out of the dang conditional :)  Besides, you're over
80 characters here aren't ya?  Also, you should probably just follow the
same convention in the function..

ret = audit_mg_timedsend(..);
if (!ret)
	goto out;

[..]
>  	if (unlikely(msg_prio >= (unsigned long) MQ_PRIO_MAX))
>  		return -EINVAL;
>  
> @@ -896,6 +905,9 @@ asmlinkage ssize_t sys_mq_timedreceive(m
>  	struct mqueue_inode_info *info;
>  	struct ext_wait_queue wait;
>  
> +	if ((ret = audit_mq_timedreceive(mqdes, msg_len, u_msg_prio, u_abs_timeout)) != 0)
> +		return ret;
> +
>  	timeout = prepare_timeout(u_abs_timeout);
>  
>  	ret = -EBADF;
> @@ -975,6 +987,9 @@ asmlinkage long sys_mq_notify(mqd_t mqde
>  	struct mqueue_inode_info *info;
>  	struct sk_buff *nc;
>  
> +	if ((ret = audit_mq_notify(mqdes, u_notification)) != 0)
> +		return ret;
> +
>  	nc = NULL;
>  	sock = NULL;
>  	if (u_notification != NULL) {
> @@ -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;
>  
> +

Eh, get rid of these...

[..]
>  out_fput:
>  	fput(filp);
>  out:
> +	audret = audit_mq_getsetattr(mqdes, &mqstat, &omqstat);
> +	if (ret == 0)
> +		ret = audret;

At a cursory glance, this looks a little fishy to me...

>  	return ret;
>  }
>  
<snip>