[2/3] xfs_io: fix TOCTOU in openfile()
diff mbox series

Message ID acf1deea-a637-571b-e501-c7cbcde95c38@redhat.com
State New
Headers show
Series
  • xfsprogs: minor 4.20 fixups
Related show

Commit Message

Eric Sandeen Feb. 19, 2019, 11:23 p.m. UTC
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")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Comments

Dave Chinner Feb. 20, 2019, 1:50 a.m. UTC | #1
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?

> +			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...

CHeers,

Dave.
Eric Sandeen Feb. 20, 2019, 4:41 a.m. UTC | #2
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?

> 
>> +			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.
>
Darrick J. Wong Feb. 20, 2019, 5:23 p.m. UTC | #3
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.
> >
Eric Sandeen Feb. 20, 2019, 8:25 p.m. UTC | #4
On 2/20/19 11:23 AM, Darrick J. Wong wrote:
>>> 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.

Ok, thanks.

-Eric

Patch
diff mbox series

diff --git a/io/open.c b/io/open.c
index f5fbd2c..856018b 100644
--- a/io/open.c
+++ b/io/open.c
@@ -62,6 +62,12 @@  openfile(
 	int		oflags;
 
 	oflags = flags & IO_READONLY ? O_RDONLY : O_RDWR;
+	/*
+	 * In case we've been passed a pipe to open, don't block waiting for a
+	 * reader or writer to appear. We want to either succeed or error out
+	 * immediately.  We'll clear O_NONBLOCK if it's not a pipe.
+	 */
+	oflags |= O_NONBLOCK;
 	if (flags & IO_APPEND)
 		oflags |= O_APPEND;
 	if (flags & IO_CREAT)
@@ -81,18 +87,6 @@  openfile(
 	if (flags & IO_NOFOLLOW)
 		oflags |= O_NOFOLLOW;
 
-	/*
-	 * if we've been passed a pipe to open, don't block waiting for a
-	 * reader or writer to appear. We want to either succeed or error out
-	 * immediately.
-	 */
-	if (stat(path, &st) < 0 && errno != ENOENT) {
-		perror("stat");
-		return -1;
-	}
-	if (S_ISFIFO(st.st_mode))
-		oflags |= O_NONBLOCK;
-
 	fd = open(path, oflags, mode);
 	if (fd < 0) {
 		if (errno == EISDIR &&
@@ -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) {
+			perror("fcntl");
+			close(fd);
+			return -1;
+		}
+	}
+
 	if (!geom || !platform_test_xfs_fd(fd))
 		return fd;