From owner-p4-projects@FreeBSD.ORG Thu Jul 5 21:46:43 2007 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id E3E9C16A46C; Thu, 5 Jul 2007 21:46:42 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 94A3216A469; Thu, 5 Jul 2007 21:46:42 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.freebsd.org (Postfix) with ESMTP id 17E2113C45A; Thu, 5 Jul 2007 21:46:41 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.8/8.13.8) with ESMTP id l65LkdbK027582; Thu, 5 Jul 2007 17:46:40 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Roman Divacky Date: Thu, 5 Jul 2007 17:43:33 -0400 User-Agent: KMail/1.9.6 References: <200707030806.l63860H7000475@repoman.freebsd.org> In-Reply-To: <200707030806.l63860H7000475@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200707051743.33415.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Thu, 05 Jul 2007 17:46:40 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/3606/Thu Jul 5 13:50:01 2007 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: Perforce Change Reviews Subject: Re: PERFORCE change 122752 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jul 2007 21:46:43 -0000 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