: Fix copy_from_user() results interpretation.

PATCH In review Pavel Emelyanov 6 participants 14 messages Apr 25, 2008 – Apr 27, 2008

Subsystems (4)

Networking Network drivers Kbuild CAN network layer

lore

Share & tools

lore

Pavel Emelyanov

author

2008-04-25 12:45 UTC #0 lore raw

PATCH 1/1 : Fix copy_from_user() results interpretation.

Sorry for the noise, I had to check this right after facing this problem with the copy_to_user()...

Both copy_to_ and _from_user return the number of bytes, that failed to reach their destination, not the 0/-EXXX values.

Other net/ code handles this correctly.

Signed-off-by Pavel Emelyanov <[email protected]>

---

net/can/raw.c +7 -5

index ead50c7..2be8c6e 100644
@@ -438,12 +438,12 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			err = copy_from_user(filter, optval, optlen);
 			if (err) {
 				kfree(filter);
-				return err;
+				return -EFAULT;
 			}
 		} else if (count == 1) {
 			err = copy_from_user(&sfilter, optval, optlen);
 			if (err)
-				return err;
+				return -EFAULT;
 		}
 
 		lock_sock(sk);

@@ -495,7 +495,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		err = copy_from_user(&err_mask, optval, optlen);
 		if (err)
-			return err;
+			return -EFAULT;
 
 		err_mask &= CAN_ERR_MASK;

@@ -531,7 +531,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->loopback))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->loopback, optval, optlen);
+		err = copy_from_user(&ro->loopback, optval, optlen) ?
+			-EFAULT : 0;
 
 		break;

@@ -539,7 +540,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->recv_own_msgs))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->recv_own_msgs, optval, optlen);
+		err = copy_from_user(&ro->recv_own_msgs, optval, optlen) ?
+			-EFAULT : 0;
 
 		break;

Oliver Hartkopp

Acked-by on v1 1/1

2008-04-25 14:27 UTC #1 lore raw

Quoted 11 lines · Pavel Emelyanov

Sorry for the noise, I had to check this right after facing this problem with the copy_to_user()...
Both copy_to_ and _from_user return the number of bytes, that failed to reach their destination, not the 0/-EXXX values.
Other net/ code handles this correctly.
Signed-off-by Pavel Emelyanov <[email protected]>

Thanks very much for catching this, Pavel!

Acked-by Oliver Hartkopp <[email protected]>

Quoted 56 lines

---

net/can/raw.c +2 -2

index ead50c7..2be8c6e 100644
@@ -438,12 +438,12 @@ static int raw_setsockopt(struct socket *sock, 
 int level, int optname,
                        err = copy_from_user(filter, optval, optlen);
                        if (err) {
                                kfree(filter);
-                               return err;
+                               return -EFAULT;
                        }
                } else if (count == 1) {
                        err = copy_from_user(&sfilter, optval, optlen);
                        if (err)
-                               return err;
+                               return -EFAULT;
                }

                lock_sock(sk);

net/can/raw.c +4 -3

@@ -495,7 +495,7 @@ static int raw_setsockopt(struct socket *sock, int 
 level, int optname,
 
                err = copy_from_user(&err_mask, optval, optlen);
                if (err)
-                       return err;
+                       return -EFAULT;
 
                err_mask &= CAN_ERR_MASK;

@@ -531,7 +531,8 @@ static int raw_setsockopt(struct socket *sock, int 
 level, int optname,
                if (optlen != sizeof(ro->loopback))
                        return -EINVAL;
 
-               err = copy_from_user(&ro->loopback, optval, optlen);
+               err = copy_from_user(&ro->loopback, optval, optlen) ?
+                       -EFAULT : 0;
 
                break;

@@ -539,7 +540,8 @@ static int raw_setsockopt(struct socket *sock, int 
 level, int optname,
                if (optlen != sizeof(ro->recv_own_msgs))
                        return -EINVAL;
 
-               err = copy_from_user(&ro->recv_own_msgs, optval, optlen);
+               err = copy_from_user(&ro->recv_own_msgs, optval,

optlen) ?
+ -EFAULT : 0;

                break;

Wolfgang Grandegger

2008-04-26 06:19 UTC #2 lore raw

Quoted 64 lines · Oliver Hartkopp

Quoted 10 lines · Pavel Emelyanov

Sorry for the noise, I had to check this right after facing this problem with the copy_to_user()...
Both copy_to_ and _from_user return the number of bytes, that failed to reach their destination, not the 0/-EXXX values.
Other net/ code handles this correctly.
Signed-off-by Pavel Emelyanov <[email protected]>

Thanks very much for catching this, Pavel!

Acked-by Oliver Hartkopp <[email protected]>

Quoted 48 lines

---

net/can/raw.c +2 -2

index ead50c7..2be8c6e 100644
@@ -438,12 +438,12 @@ static int raw_setsockopt(struct socket *sock, 
 int level, int optname,
                        err = copy_from_user(filter, optval, optlen);
                        if (err) {
                                kfree(filter);
-                               return err;
+                               return -EFAULT;
                        }
                } else if (count == 1) {
                        err = copy_from_user(&sfilter, optval, optlen);
                        if (err)
-                               return err;
+                               return -EFAULT;
                }

                lock_sock(sk);

net/can/raw.c +3 -2

@@ -495,7 +495,7 @@ static int raw_setsockopt(struct socket *sock, int 
 level, int optname,
 
                err = copy_from_user(&err_mask, optval, optlen);
                if (err)
-                       return err;
+                       return -EFAULT;
 
                err_mask &= CAN_ERR_MASK;

@@ -531,7 +531,8 @@ static int raw_setsockopt(struct socket *sock, int 
 level, int optname,
                if (optlen != sizeof(ro->loopback))
                        return -EINVAL;
 
-               err = copy_from_user(&ro->loopback, optval, optlen);
+               err = copy_from_user(&ro->loopback, optval, optlen) ?
+                       -EFAULT : 0;
 
                break;

@@ -539,7 +540,8 @@ static int raw_setsockopt(struct socket *sock, int 
 level, int optname,
                if (optlen != sizeof(ro->recv_own_msgs))
                        return -EINVAL;

Quoted 8 lines

net/can/raw.c +1 -1

-               err = copy_from_user(&ro->recv_own_msgs, optval, optlen);
+               err = copy_from_user(&ro->recv_own_msgs, optval,

optlen) ?
+ -EFAULT : 0;

                break;

What about removing the assignment "err =" in that case. It's confusing otherwise and maybe the variable err is even obsolete.

Wolfgang.

David Miller

maintainer

2008-04-26 06:23 UTC #3 lore raw

From: Wolfgang Grandegger <[email protected]>
Date: Sat, 26 Apr 2008 08:19:31 +0200

Quoted 2 lines

What about removing the assignment "err =" in that case. It's confusing otherwise and maybe the variable err is even obsolete.

Sure, but when fixing a bug it's better to have a completely localized change like this.

Not something which restructures the code unnecessarily at the same time, that make it so much harder to review and validate.

Wolfgang Grandegger

2008-04-26 06:35 UTC #4 lore raw

Quoted 6 lines · David Miller

From: Wolfgang Grandegger <[email protected]>
Date: Sat, 26 Apr 2008 08:19:31 +0200
Quoted 2 lines
What about removing the assignment "err =" in that case. It's confusing otherwise and maybe the variable err is even obsolete.

Quoted 6 lines

Sure, but when fixing a bug it's better to have a completely localized change like this.
Not something which restructures the code unnecessarily at the same time, that make it so much harder to review and validate.

Well, really,

if (func(x))

        return -EFAULT;

instead of

err = func(x);
if (err)

	return -EFAULT;

is more straight forward and less confusing and I do not understand, why it should be harder to review.

Wolfgang.

Sam Ravnborg

2008-04-26 06:39 UTC #5 lore raw

In reply to Wolfgang Grandegger

Quoted 1 line · Wolfgang Grandegger

What about removing the assignment "err =" in that case.

Preferred.

See sample patch below (made on top of -linus
so it does likely not apply but made it only to
see the difference anyway).

Sam

diffstat:

 net/can/raw.c          |   21 ++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

net/can/raw.c +10 ~~-11~~

index 201cbfc..69877b8 100644
@@ -435,15 +435,13 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			if (!filter)
 				return -ENOMEM;
 
-			err = copy_from_user(filter, optval, optlen);
-			if (err) {
+			if (copy_from_user(filter, optval, optlen)) {
 				kfree(filter);
-				return err;
+				return -EFAULT;
 			}
 		} else if (count == 1) {
-			err = copy_from_user(&sfilter, optval, optlen);
-			if (err)
-				return err;
+			if (copy_from_user(&sfilter, optval, optlen))
+				return -EFAULT;
 		}
 
 		lock_sock(sk);

@@ -493,9 +491,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(err_mask))
 			return -EINVAL;
 
-		err = copy_from_user(&err_mask, optval, optlen);
-		if (err)
-			return err;
+		if (copy_from_user(&err_mask, optval, optlen))
+			return -EFAULT;
 
 		err_mask &= CAN_ERR_MASK;

@@ -531,7 +528,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->loopback))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->loopback, optval, optlen);
+		if (copy_from_user(&ro->loopback, optval, optlen))
+			return -EFAULT;
 
 		break;

@@ -539,7 +537,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->recv_own_msgs))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->recv_own_msgs, optval, optlen);
+		if (copy_from_user(&ro->recv_own_msgs, optval, optlen))
+			return -EFAULT;
 
 		break;

David Miller

maintainer

2008-04-26 07:03 UTC #6 lore raw

From: Sam Ravnborg <[email protected]>
Date: Sat, 26 Apr 2008 08:40:07 +0200

Quoted 7 lines · Wolfgang Grandegger

Quoted 1 line
What about removing the assignment "err =" in that case.
Preferred.
See sample patch below (made on top of -linus
so it does likely not apply but made it only to
see the difference anyway).

I want to make one comment, directed at Wolfgang.

You are absolutely wrong, Wolfgang, in saying that Pavel's original patch isn't easier to review than just changing this code to go:

	if (copy_*_user())
		return -EFAULT;

In fact, recoding things like this is an immense extra hardship on a reviewer. I'll explain why.

If I see a patch that changes:

	err = SOMETHING;
	break;

into:

	err = SOMETHING_ELSE;
	break;

I know, WITH JUST READING THE PATCH, exactly what the side effects of this change are.

I DO NOT need to bring the code into my editor and validate side effects to the surrounding code.

I know that the assignment to 'err' is being changed, and that's it.

Whereas if you change:

	err = SOMETHING;
	break;

into:

	if (SOMETHING)
		return -SOME_ERROR;
	break;

I now have to bring the code into an editor and make sure that the control flow change doesn't break things.

For example, maybe the exit of the switch statement was important, to make sure cleanup code runs at the end of the function to release locks, free allocated memory, etc.

With Pavel's patch it is not necessary to make such validations so it's INFINITELY easier to validate.

Oliver Hartkopp

Acked-by

2008-04-26 08:26 UTC #8 lore raw

In reply to Sam Ravnborg

Quoted 10 lines · Sam Ravnborg

Quoted 2 lines · Wolfgang Grandegger
What about removing the assignment "err =" in that case.
Preferred.
See sample patch below (made on top of -linus
so it does likely not apply but made it only to
see the difference anyway).

I can definitely follow Daves remarks regarding the ability to review local bugfixes. In this case two commits should be used.

Btw. the cleanup an removal of the err variable usage in the pointed cases is a good suggestion. So even when the two latest changes in Sam's patch are not that obvious to be able to be reviewed locally i would like to ack this patch.

It should apply fine on Linus' tree and the current net-2.6.

Thanks to all of you & have a nice weekend.

Oliver

Acked-by Oliver Hartkopp <[email protected]>

Quoted 65 lines

diffstat:

 net/can/raw.c          |   21 ++++++++++-----------
 2 files changed, 11 insertions(+), 12 deletions(-)

net/can/raw.c +10 ~~-11~~

index 201cbfc..69877b8 100644
@@ -435,15 +435,13 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			if (!filter)
 				return -ENOMEM;
 
-			err = copy_from_user(filter, optval, optlen);
-			if (err) {
+			if (copy_from_user(filter, optval, optlen)) {
 				kfree(filter);
-				return err;
+				return -EFAULT;
 			}
 		} else if (count == 1) {
-			err = copy_from_user(&sfilter, optval, optlen);
-			if (err)
-				return err;
+			if (copy_from_user(&sfilter, optval, optlen))
+				return -EFAULT;
 		}
 
 		lock_sock(sk);

@@ -493,9 +491,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(err_mask))
 			return -EINVAL;
 
-		err = copy_from_user(&err_mask, optval, optlen);
-		if (err)
-			return err;
+		if (copy_from_user(&err_mask, optval, optlen))
+			return -EFAULT;
 
 		err_mask &= CAN_ERR_MASK;

@@ -531,7 +528,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->loopback))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->loopback, optval, optlen);
+		if (copy_from_user(&ro->loopback, optval, optlen))
+			return -EFAULT;
 
 		break;

@@ -539,7 +537,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->recv_own_msgs))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->recv_own_msgs, optval, optlen);
+		if (copy_from_user(&ro->recv_own_msgs, optval, optlen))
+			return -EFAULT;
 
 		break;

_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core

David Miller

maintainer

2008-04-26 09:10 UTC #9 lore raw

From: Oliver Hartkopp <[email protected]>
Date: Sat, 26 Apr 2008 10:26:34 +0200

Quoted 4 lines

Btw. the cleanup an removal of the err variable usage in the pointed cases is a good suggestion. So even when the two latest changes in Sam's patch are not that obvious to be able to be reviewed locally i would like to ack this patch.

Right, I also have no problem with Sam's patch at all and plan to apply it.

Thanks.

Sam Ravnborg

2008-04-26 09:58 UTC #10 lore raw

PATCH 1/1 : Fix copy_from_user() results interpretation

Both copy_to_ and _from_user return the number of bytes, that failed to reach their destination, not the 0/-EXXX values.

Based on patch from Pavel Emelyanov <[email protected]>

Signed-off-by Sam Ravnborg <[email protected]>

Acked-by Oliver Hartkopp <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

---

Hi David.

Quoted 2 lines

Right, I also have no problem with Sam's patch at all and plan to apply it.

OK - here is a proper patch.
Actual patch is the same - I just added a proper changelog.

Sam

net/can/raw.c +10 ~~-11~~

index 201cbfc..69877b8 100644
@@ -435,15 +435,13 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			if (!filter)
 				return -ENOMEM;
 
-			err = copy_from_user(filter, optval, optlen);
-			if (err) {
+			if (copy_from_user(filter, optval, optlen)) {
 				kfree(filter);
-				return err;
+				return -EFAULT;
 			}
 		} else if (count == 1) {
-			err = copy_from_user(&sfilter, optval, optlen);
-			if (err)
-				return err;
+			if (copy_from_user(&sfilter, optval, optlen))
+				return -EFAULT;
 		}
 
 		lock_sock(sk);

@@ -493,9 +491,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(err_mask))
 			return -EINVAL;
 
-		err = copy_from_user(&err_mask, optval, optlen);
-		if (err)
-			return err;
+		if (copy_from_user(&err_mask, optval, optlen))
+			return -EFAULT;
 
 		err_mask &= CAN_ERR_MASK;

@@ -531,7 +528,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->loopback))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->loopback, optval, optlen);
+		if (copy_from_user(&ro->loopback, optval, optlen))
+			return -EFAULT;
 
 		break;

@@ -539,7 +537,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (optlen != sizeof(ro->recv_own_msgs))
 			return -EINVAL;
 
-		err = copy_from_user(&ro->recv_own_msgs, optval, optlen);
+		if (copy_from_user(&ro->recv_own_msgs, optval, optlen))
+			return -EFAULT;
 
 		break;

scripts/Makefile.build +1 -1

index 277cfe0..4a7327b 100644
@@ -105,7 +105,7 @@ endif
 
 # Do section mismatch analysis for each module/built-in.o
 ifdef CONFIG_DEBUG_SECTION_MISMATCH
-  cmd_secanalysis = ; scripts/mod/modpost $@
+  #cmd_secanalysis = ; scripts/mod/modpost $@
 endif
 
 # Compile C sources (.c)

David Miller

maintainer

2008-04-26 10:01 UTC #11 lore raw

Re: [PATCH][CAN]: Fix copy_from_user() results interpretation

From: Sam Ravnborg <[email protected]>
Date: Sat, 26 Apr 2008 11:59:23 +0200

scripts/Makefile.build endif · line 111

Quoted 13 lines

scripts/Makefile.build +1 -1

index 277cfe0..4a7327b 100644
@@ -105,7 +105,7 @@ endif
 
 # Do section mismatch analysis for each module/built-in.o
 ifdef CONFIG_DEBUG_SECTION_MISMATCH
-  cmd_secanalysis = ; scripts/mod/modpost $@
+  #cmd_secanalysis = ; scripts/mod/modpost $@
 endif
 
 # Compile C sources (.c)

What's this? :-)

Sam Ravnborg

2008-04-26 10:05 UTC #12 lore raw

Re: [PATCH][CAN]: Fix copy_from_user() results interpretation

Quoted 15 lines · David Miller

From: Sam Ravnborg <[email protected]>
Date: Sat, 26 Apr 2008 11:59:23 +0200

Quoted 12 lines

scripts/Makefile.build +1 -1

index 277cfe0..4a7327b 100644
@@ -105,7 +105,7 @@ endif
 
 # Do section mismatch analysis for each module/built-in.o
 ifdef CONFIG_DEBUG_SECTION_MISMATCH
-  cmd_secanalysis = ; scripts/mod/modpost $@
+  #cmd_secanalysis = ; scripts/mod/modpost $@
 endif

Quoted 3 lines

scripts/Makefile.build endif · line 111
Quoted 1 line
scripts/Makefile.build +0 -0
 # Compile C sources (.c)
What's this? :-)

Sorry!
Workaround for a newly introduced kbuild bug and I did not review my patch properly.
I hand edited it out in first mail but should have done so in my queued patch to prevent this.

Sam

Wolfgang Grandegger

2008-04-26 13:04 UTC #7 lore raw

In reply to David Miller

Quoted 55 lines · David Miller

From: Sam Ravnborg <[email protected]>
Date: Sat, 26 Apr 2008 08:40:07 +0200
Quoted 7 lines · Wolfgang Grandegger
Quoted 1 line
What about removing the assignment "err =" in that case.
Preferred.
See sample patch below (made on top of -linus
so it does likely not apply but made it only to
see the difference anyway).
I want to make one comment, directed at Wolfgang.
You are absolutely wrong, Wolfgang, in saying that Pavel's original patch isn't easier to review than just changing this code to go:
	if (copy_*_user())
		return -EFAULT;
In fact, recoding things like this is an immense extra hardship on a reviewer. I'll explain why.
If I see a patch that changes:
	err = SOMETHING;
	break;
into:
	err = SOMETHING_ELSE;
	break;
I know, WITH JUST READING THE PATCH, exactly what the side effects of this change are.
I DO NOT need to bring the code into my editor and validate side effects to the surrounding code.
I know that the assignment to 'err' is being changed, and that's it.
Whereas if you change:
	err = SOMETHING;
	break;
into:
	if (SOMETHING)
		return -SOME_ERROR;
	break;
I now have to bring the code into an editor and make sure that the control flow change doesn't break things.

Quoted 6 lines

For example, maybe the exit of the switch statement was important, to make sure cleanup code runs at the end of the function to release locks, free allocated memory, etc.
With Pavel's patch it is not necessary to make such validations so it's INFINITELY easier to validate.

OK, I see, if you have to review a lot of patches, it does matter, indeed.

Wolfgang.

David Miller

maintainer

2008-04-27 05:57 UTC #13 lore raw

Re: [PATCH][CAN]: Fix copy_from_user() results interpretation

In reply to Sam Ravnborg

From: Sam Ravnborg <[email protected]>
Date: Sat, 26 Apr 2008 11:59:23 +0200

Quoted 7 lines

Both copy_to_ and _from_user return the number of bytes, that failed to reach their destination, not the 0/-EXXX values.
Based on patch from Pavel Emelyanov <[email protected]>
Signed-off-by Sam Ravnborg <[email protected]>
Acked-by Oliver Hartkopp <[email protected]>

Applied, sans the local Makefile.build change in your tree, thanks Sam. :-)