Re: [PATCH 2/3] xfs_io: fix TOCTOU in openfile()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Feb 19, 2019 at 10:41:57PM -0600, Eric Sandeen wrote:
> 
> 
> On 2/19/19 7:50 PM, Dave Chinner wrote:
> > On Tue, Feb 19, 2019 at 05:23:38PM -0600, Eric Sandeen wrote:
> >> openfile() stats a path to determine whether it is a pipe, and then
> >> opens it with flags based on the type it saw during the stat.  It's
> >> possible that the file at that path changes in between, and Coverity
> >> points this out.
> >>
> >> Instead, always open O_NONBLOCK, stat the fd we got, and turn the
> >> flag back off via fcntl() if it's not a pipe.
> >>
> >> Addresses-Coverity-ID: 1442788 ("Time of check time of use")
> > 
> > For O_NONBLOCK I think there is zero harm in the code as it stands,
> > but I don't really care about it tht much.
> > 
> > I do wonder what happens if need to conditionally use O_NONBLOCK on
> > open(), though....
> > 
> >> @@ -112,6 +106,22 @@ openfile(
> >>  		}
> >>  	}
> >>  
> >> +	if (fstat(fd, &st) < 0) {
> >> +		perror("stat");
> >> +		close(fd);
> >> +		return -1;
> >> +	}
> >> +
> >> +	/* We only want to keep nonblocking behavior for pipes */
> >> +	if (!S_ISFIFO(st.st_mode)) {
> >> +		oflags &= ~O_NONBLOCK;
> >> +		if (fcntl(fd, F_SETFL, oflags) < 0) {
> > 
> > Don't you need to do oflags = fcntl(fd, F_GETFL, NULL) first?
> 
> maybe? We just opened it with oflags, but *shrug* I guess it doesn't
> hurt?

The kernel can set O_ flags on the file (e.g. O_LARGEFILE) for you,
which means that if you try to F_SETFL with the original oflags, the
kernel will think you're trying to clear those flags and misunderstand.

--D

> > 
> >> +			perror("fcntl");
> >> +			close(fd);
> >> +			return -1;
> >> +		}
> > 
> > can you add a "goto out_close;" error jump to this function now that
> > this is the fourth and fifth places that have this same close/return
> > on error...
> 
> yeah, good point.
> 
> > CHeers,
> > 
> > Dave.
> >