fcntl: return -EFAULT if copy_to_user fails

PATCH Applied Dan Carpenter 7 participants 16 messages Jun 3, 2010 – Jun 4, 2010

Subsystems (2)

VFS File locking (flock() and fcntl()/lockf())

lore

Share & tools

lore

Dan Carpenter

author

2010-06-03 10:04 UTC #0 lore raw

PATCH 1/1 fcntl: return -EFAULT if copy_to_user fails

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.

	ret = fcntl(fd, F_SETOWN_EX, NULL);

With the original code ret would be 8 here.

Signed-off-by Dan Carpenter <[email protected]>

fs/fcntl.c +1 -1

index f74d270..0ea7b0f 100644
@@ -274,7 +274,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
 
 	ret = copy_from_user(&owner, owner_p, sizeof(owner));
 	if (ret)
-		return ret;
+		return -EFAULT;
 
 	switch (owner.type) {
 	case F_OWNER_TID:

Takuya Yoshikawa

2010-06-03 10:18 UTC #1 lore raw

Quoted 8 lines · Dan Carpenter

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
Signed-off-by Dan Carpenter<[email protected]>

How about f_getown_ex() ?

	if (!ret)
		ret = copy_to_user(owner_p, &owner, sizeof(owner));
	return ret;

Fixing this too would be better, I think.

Takuya

Quoted 19 lines

fs/fcntl.c +1 -2
index f74d270..0ea7b0f 100644
@@ -274,7 +274,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
 
  	ret = copy_from_user(&owner, owner_p, sizeof(owner));
  	if (ret)
-		return ret;
+		return -EFAULT;
 
  	switch (owner.type) {
  	case F_OWNER_TID:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Takuya Yoshikawa

2010-06-03 10:18 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 8 lines · Dan Carpenter

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
Signed-off-by Dan Carpenter<[email protected]>

How about f_getown_ex() ?

	if (!ret)
		ret = copy_to_user(owner_p, &owner, sizeof(owner));
	return ret;

Fixing this too would be better, I think.

Takuya

Quoted 19 lines

fs/fcntl.c +1 -2
index f74d270..0ea7b0f 100644
@@ -274,7 +274,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
 
  	ret = copy_from_user(&owner, owner_p, sizeof(owner));
  	if (ret)
-		return ret;
+		return -EFAULT;
 
  	switch (owner.type) {
  	case F_OWNER_TID:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Dan Carpenter

author

2010-06-03 10:35 UTC #3 lore raw

PATCH v2 1/1 fcntl: return -EFAULT if copy_to_user fails

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.

	ret = fcntl(fd, F_SETOWN_EX, NULL);

With the original code ret would be 8 here.

V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()

Signed-off-by Dan Carpenter <[email protected]>

fs/fcntl.c +5 -2

index f74d270..51e11bf 100644
@@ -274,7 +274,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
 
 	ret = copy_from_user(&owner, owner_p, sizeof(owner));
 	if (ret)
-		return ret;
+		return -EFAULT;
 
 	switch (owner.type) {
 	case F_OWNER_TID:

@@ -332,8 +332,11 @@ static int f_getown_ex(struct file *filp, unsigned long arg)
 	}
 	read_unlock(&filp->f_owner.lock);
 
-	if (!ret)
+	if (!ret) {
 		ret = copy_to_user(owner_p, &owner, sizeof(owner));
+		if (ret)
+			ret = -EFAULT;
+	}
 	return ret;
 }

Peter Zijlstra

Acked-by on v2 1/1

2010-06-03 10:42 UTC #4 lore raw

Quoted 8 lines · Dan Carpenter

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()
Signed-off-by Dan Carpenter <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Quoted 26 lines

fs/fcntl.c +5 -2

index f74d270..51e11bf 100644
@@ -274,7 +274,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
 
 	ret = copy_from_user(&owner, owner_p, sizeof(owner));
 	if (ret)
-		return ret;
+		return -EFAULT;
 
 	switch (owner.type) {
 	case F_OWNER_TID:

@@ -332,8 +332,11 @@ static int f_getown_ex(struct file *filp, unsigned long arg)
 	}
 	read_unlock(&filp->f_owner.lock);
 
-	if (!ret)
+	if (!ret) {
 		ret = copy_to_user(owner_p, &owner, sizeof(owner));
+		if (ret)
+			ret = -EFAULT;
+	}
 	return ret;
 }

Jens Axboe

Acked-by on v2 1/1

2010-06-03 11:59 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Dan Carpenter

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()

Pretty basic bug, how long has this been there?

Acked-by Jens Axboe <[email protected]>

Signature · 1 line

Jens Axboe

Jens Axboe

Acked-by on v2 1/1

2010-06-03 11:59 UTC #6 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Dan Carpenter

copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()

Pretty basic bug, how long has this been there?

Acked-by Jens Axboe <[email protected]>

Signature · 1 line

Jens Axboe

Takuya Yoshikawa

2010-06-03 12:12 UTC #7 lore raw

Quoted 9 lines · Jens Axboe

Quoted 6 lines · Dan Carpenter
copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()
Pretty basic bug, how long has this been there?

IIUC, from the beginning, when these were introduced.

And I recently sent similar bug fixes for other parts.

Ah, I also saw somebody sent cleanup patches using memdup_user() which does not reside in uaccess.h.

Though I'm personally using private *_user documentation, are there any good official doc for these?

Takuya

Quoted 3 lines

Acked-by Jens Axboe<[email protected]>

Eric Dumazet

2010-06-03 12:38 UTC #8 lore raw

Quoted 7 lines · Takuya Yoshikawa

Quoted 6 lines · Jens Axboe
Quoted 5 lines · Dan Carpenter
copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()
Pretty basic bug, how long has this been there?
IIUC, from the beginning, when these were introduced.

Maybe copy_to_user() was changed sometime to return a partial count instead of EFAULT ?

I do think we should have a set of helper functions, instead of spreading special EFAULT cases in one housand places...

This is really ugly.

static inline int sec_copy_to_user(arg1, arg2, arg3)

{
	int res = copy_to_user(arg1, arg2, arg3);

	return (res > 0) ? -EFAULT : res;
}

Nick Piggin

2010-06-03 13:10 UTC #9 lore raw

Quoted 8 lines · Eric Dumazet

Quoted 7 lines · Takuya Yoshikawa
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Dan Carpenter
copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()
Pretty basic bug, how long has this been there?
IIUC, from the beginning, when these were introduced.
Maybe copy_to_user() was changed sometime to return a partial count instead of EFAULT ?

I think it's been like that since first introduced. Some functions do need to know in order to do partial copies.

Quoted 11 lines

I do think we should have a set of helper functions, instead of spreading special EFAULT cases in one housand places...
This is really ugly.
static inline int sec_copy_to_user(arg1, arg2, arg3)
{
	int res = copy_to_user(arg1, arg2, arg3);

	return (res > 0) ? -EFAULT : res;
}

It would be unfortunate if it adds more confusion. I'd prefer to have a sufficiently different name. memcpy_to_user/memcpy_from_user perhaps?

Takuya Yoshikawa

2010-06-03 13:20 UTC #10 lore raw

Quoted 31 lines · Nick Piggin

Quoted 8 lines · Eric Dumazet
Quoted 7 lines · Takuya Yoshikawa
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Dan Carpenter
copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()
Pretty basic bug, how long has this been there?
IIUC, from the beginning, when these were introduced.
Maybe copy_to_user() was changed sometime to return a partial count instead of EFAULT ?
I think it's been like that since first introduced. Some functions do need to know in order to do partial copies.
Quoted 9 lines
I do think we should have a set of helper functions, instead of spreading special EFAULT cases in one housand places...
This is really ugly.
static inline int sec_copy_to_user(arg1, arg2, arg3)
{
	int res = copy_to_user(arg1, arg2, arg3);

Quoted 6 lines

Quoted 2 lines
	return (res>  0) ? -EFAULT : res;
}
It would be unfortunate if it adds more confusion. I'd prefer to have a sufficiently different name. memcpy_to_user/memcpy_from_user perhaps?

Then, and memclear_user() ?

Quoted 1 line

Eric Dumazet

2010-06-03 13:42 UTC #11 lore raw

Quoted 9 lines · Takuya Yoshikawa

Quoted 8 lines · Nick Piggin
Quoted 6 lines · Eric Dumazet
This is really ugly.
static inline int sec_copy_to_user(arg1, arg2, arg3)
{
	int res = copy_to_user(arg1, arg2, arg3);

Quoted 11 lines

Quoted 6 lines
Quoted 2 lines
	return (res>  0) ? -EFAULT : res;
}
It would be unfortunate if it adds more confusion. I'd prefer to have a sufficiently different name. memcpy_to_user/memcpy_from_user perhaps?
Then, and memclear_user() ?
Quoted 1 line

We are interested by the fact that full copy is done, so maybe use the 'full' prefix ?

fullcopy_to_user() , fullclear_user() ?

Dan Carpenter

author

2010-06-03 12:45 UTC #12 lore raw

In reply to Takuya Yoshikawa

Quoted 7 lines · Takuya Yoshikawa

Quoted 6 lines · Jens Axboe
Quoted 5 lines · Dan Carpenter
copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex()
Pretty basic bug, how long has this been there?
IIUC, from the beginning, when these were introduced.
And I recently sent similar bug fixes for other parts.

It was your clear_user() patch which inspired me. I wrote a smatch check to find these. I've pushed the code to the smatch repo.
http://repo.or.cz/r/smatch.git

The heuristic I use is that if we return a variable which is the return value of copy_to_user() and it's non-zero then complain. It didn't find the f_getown_ex() because that return value could come from copy_to_user() or it could be -EINVAL.

I'll mess with it a bit and see if I can make it catch the f_getown_ex() bug.

regards,
dan carpenter

Dan Carpenter

author

2010-06-04 11:14 UTC #13 lore raw

Quoted 8 lines · Dan Carpenter

The heuristic I use is that if we return a variable which is the return value of copy_to_user() and it's non-zero then complain. It didn't find the f_getown_ex() because that return value could come from copy_to_user() or it could be -EINVAL.
I'll mess with it a bit and see if I can make it catch the f_getown_ex() bug.

I changed the heuristic to complain if we return a non-zero return from copy_to_user() and the minimum possible value of the return is not zero.

	ret = copy_to_user();
	if (ret)
		return ret;  // <- Complain.  The minimum value is 1.

Or:

	if (!foo)
		ret = -ENOMEM;
	if (!ret)
		ret = copy_to_user();
	return ret;  // <- Complain.  The minimum value is -ENOMEM.

This seems to work pretty well. I've fixed all the bugs this found and I've pushed the check to the smatch repo.

regards,
dan carpenter

Dan Carpenter

author

2010-06-04 11:14 UTC #14 lore raw

In reply to Dan Carpenter

Quoted 8 lines · Dan Carpenter

The heuristic I use is that if we return a variable which is the return value of copy_to_user() and it's non-zero then complain. It didn't find the f_getown_ex() because that return value could come from copy_to_user() or it could be -EINVAL.
I'll mess with it a bit and see if I can make it catch the f_getown_ex() bug.

I changed the heuristic to complain if we return a non-zero return from copy_to_user() and the minimum possible value of the return is not zero.

	ret = copy_to_user();
	if (ret)
		return ret;  // <- Complain.  The minimum value is 1.

Or:

	if (!foo)
		ret = -ENOMEM;
	if (!ret)
		ret = copy_to_user();
	return ret;  // <- Complain.  The minimum value is -ENOMEM.

This seems to work pretty well. I've fixed all the bugs this found and I've pushed the check to the smatch repo.

regards,
dan carpenter

Al Viro

maintainer Applied

2010-06-03 12:45 UTC #15 lore raw

In reply to Takuya Yoshikawa

Quoted 12 lines · Takuya Yoshikawa

Quoted 8 lines · Dan Carpenter
copy_to_user() returns the number of bytes remaining, but we want to return -EFAULT.
	ret = fcntl(fd, F_SETOWN_EX, NULL);
With the original code ret would be 8 here.
Signed-off-by Dan Carpenter<[email protected]>
How about f_getown_ex() ?

Quoted 7 lines

	if (!ret)
		ret = copy_to_user(owner_p, &owner, sizeof(owner));
	return ret;
Fixing this too would be better, I think.
Takuya

Applied, will push today.