svn commit: r265327 - in user/dchagin/lemul/sys: amd64/linux amd64/linux32 compat/linux conf i386/linux modules/linux modules/linux64

[Chagin Dmitry]

On Mon, May 05, 2014 at 05:32:18PM +0200, Mateusz Guzik wrote:
> On Mon, May 05, 2014 at 09:02:04AM +0400, Chagin Dmitry wrote:
> > On Sun, May 04, 2014 at 08:07:49PM +0200, Mateusz Guzik wrote:
> > > On Sun, May 04, 2014 at 03:59:32PM +0000, Dmitry Chagin wrote:
> > > > Author: dchagin
> > > > Date: Sun May  4 15:59:32 2014
> > > > New Revision: 265327
> > > > URL: http://svnweb.freebsd.org/changeset/base/265327
> > > > 
> > > > Log:
> > > >   Implement epoll family system calls. This is a tiny wrapper around kqueue
> > > >   to implement epoll subset of functionality. The kqueue user data are
> > > >   32bit on i386 which is not enough for epoll user data, so we keep user
> > > >   data in the proc emuldata.
> > > >   
> > > >   Initial patch developed by rdivacky in 2007, then extended by
> > > >   Yuri Victorovich @ r255672 and finished by me.
> > > > 
> > > 
> > > I'm not happy with this. The code is full of fp <-> fd lookup races.
> > > 
> > 
> > Hi, Mateusz. Thanks for the reply.
> > 
> > As far as I understand you, check epfd against fd is useless.
> > FreeBSD does not check that it's the same file.But we must.
> > Is it correct?:
> > 
> > 
> > diff --git a/sys/compat/linux/linux_event.c b/sys/compat/linux/linux_event.c
> > index ee70b9c..054df14 100644
> > --- a/sys/compat/linux/linux_event.c
> > +++ b/sys/compat/linux/linux_event.c
> > @@ -339,9 +339,6 @@ linux_epoll_ctl(struct thread *td, struct linux_epoll_ctl_args *args)
> >  	int nchanges = 0;
> >  	int error;
> >  
> > -	if (args->epfd == args->fd)
> > -		return (EINVAL);
> > -
> >  	if (args->op != LINUX_EPOLL_CTL_DEL) {
> >  		error = copyin(args->event, &le, sizeof(le));
> >  		if (error != 0)
> > @@ -359,6 +356,12 @@ linux_epoll_ctl(struct thread *td, struct linux_epoll_ctl_args *args)
> >  	if (error != 0)
> >  		goto leave1;
> >  
> > +	/* Linux disallows spying on himself */
> > +	if (epfp == fp) {
> > +		error = EINVAL;
> > +		goto leave0;
> > +	}
> > +
> >  	ciargs.changelist = kev;
> >  
> 
> yeah, that's much better :)
> 

commited, thanks! I was really concentrated on the main path and missied
this obvious detail.




> > > There is no validation you got epoll fd either.
> > > 
> > > Further down:
> > >         switch (args->op) {
> > >         case LINUX_EPOLL_CTL_MOD:
> > >                 /*
> > >                  * We don't memorize which events were set for this FD
> > >                  * on this level, so just delete all we could have set:
> > >                  * EVFILT_READ and EVFILT_WRITE, ignoring any errors
> > >                  */
> > >                 error = epoll_delete_all_events(td, epfp, args->fd);
> > > 
> > > Again a lookup.
> > > 
> > > Whether this particular problem could be used to do something nasty I don't
> > > know, but playing like this is asking for trouble.
> > > 
> > > The only solution I see is to modify kqueue functions to accept fps.
> > > 
> > 
> > reason? to prevent extra fget? or something else?
> > 
> 
> Having multpiple lookups for the same fd number may lead to different
> fps, which may or may not be used to cause inconsistencies which in turn
> may or may not be exploitable to either crash the kernel or escalate
> privileges.
> 
> That said, the concern is that a malicious user could try to work
> something out from this.
> 

Hi, may be enough to keep file ref?

-- 
Have fun!
chd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-user/attachments/20140506/c242700b/attachment.sig>