PERFORCE change 122752 for review

[John Baldwin]

On Tuesday 03 July 2007 04:06:00 am Roman Divacky wrote:
> http://perforce.freebsd.org/chv.cgi?CH=122752
> 
> Change 122752 by rdivacky at rdivacky_witten on 2007/07/03 08:05:17
> 
> 	O_EXEC support. it is able to fexecve "/bin/date" when opened with O_RDONLY
> 	or O_EXEC.
> 	
> 	I am a little suspicious about this patch because audacious (mp3 player) 
acts
> 	really weird now. Needs some more investigation.


> @@ -1281,6 +1283,27 @@
>  		return (ETXTBSY);
>  
>  	/*
> +	 *  Check for the mode the file was opened with
> +	 */
> +	if (fexecve) {
> +		struct file f;
> +		struct file *fp = &f;

What is the point of this?

> +
> +		FILEDESC_SLOCK(td->td_proc->p_fd);
> +		fp = fget_locked(td->td_proc->p_fd, fd);
> +		if (fp == NULL || fp->f_ops == &badfileops) {
> +			FILEDESC_SUNLOCK(td->td_proc->p_fd);
> +			return (EBADF);
> +		}
> +		fhold(fp);
> +		FILEDESC_SUNLOCK(td->td_proc->p_fd);
> +		if (!(fp->f_flag & FREAD) && !(fp->f_flag & O_EXEC)) {
> +			fdrop(fp, td);
> +			return (EACCES);
> +		}
> +		fdrop(fp, td);
> +	}
> +	/*
>  	 * Call filesystem specific open routine (which does nothing in the
>  	 * general case).
>  	 */

Any reason you didn't just use fget() here?  i.e.:

	if (fexecve) {
		struct file *fp;

		error = fget(td, fd, &fp);
		if (error)
			return (error);
		if ((fp->f_flag & (FREAD | O_EXEC)) == 0) {
			fdrop(fp, td);
			return (EACCES);
		}
		fdrop(fp, td);
	}

Also, the fdrop() here seems very, very dubious.  You shouldn't be dropping 
it, but holding the reference since the file could change out from under you 
(shared descriptor tables via rfork(), etc.) leading to a race and potential 
security problem (think of a race where a rfork()'d thread changes the binary 
you are going to execute out from under you).  The better fix may be to check 
the fd when you open it to actually use it.

> ==== //depot/projects/soc2007/rdivacky/linux_at/sys/sys/fcntl.h#8 (text+ko) 
====
> 
> @@ -107,6 +103,7 @@
>  #ifdef _KERNEL
>  #define	FHASLOCK	0x4000		/* descriptor holds advisory lock */
>  #endif
> +#define	O_EXEC		0x8000		/* open for execute only */
>  /* Defined by POSIX Extended API ... TODO: number of the spec */
>  #define	AT_FDCWD		-100	/* Use the current working directory
>  					   to determine the target of relative
> @@ -138,7 +135,7 @@

This clashes with O_NOCTTY.

-- 
John Baldwin