Date: Thu, 5 Jul 2007 17:43:33 -0400 From: John Baldwin <jhb@freebsd.org> To: Roman Divacky <rdivacky@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org> Subject: Re: PERFORCE change 122752 for review Message-ID: <200707051743.33415.jhb@freebsd.org> In-Reply-To: <200707030806.l63860H7000475@repoman.freebsd.org> References: <200707030806.l63860H7000475@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 03 July 2007 04:06:00 am Roman Divacky wrote: > http://perforce.freebsd.org/chv.cgi?CH=122752 > > Change 122752 by rdivacky@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200707051743.33415.jhb>