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

From: Oliver Hartkopp <oliver@hartkopp.net>
To: Sam Ravnborg <sam@ravnborg.org>, David Miller <davem@davemloft.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>,
	"Thuermann, Urs, Dr. \(K-EFFI/I\)" <urs.thuermann@volkswagen.de>,
	Linux Netdev List <netdev@vger.kernel.org>,
	socketcan-core@lists.berlios.de,
	Oliver Hartkopp <oliver.hartkopp@volkswagen.de>,
	Pavel Emelyanov <xemul@openvz.org>
Subject: Re: [PATCH][CAN]: Fix copy_from_user() results interpretation.
Date: Sat, 26 Apr 2008 10:26:34 +0200	[thread overview]
Message-ID: <4812E73A.6060104@hartkopp.net> (raw)
In-Reply-To: <20080426064007.GA20558@uranus.ravnborg.org>

Sam Ravnborg wrote:
> On Sat, Apr 26, 2008 at 08:19:31AM +0200, Wolfgang Grandegger wrote:
>   
>> 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 <oliver@hartkopp.net>

> diffstat:
>  net/can/raw.c          |   21 ++++++++++-----------
>  2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 201cbfc..69877b8 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -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
> Socketcan-core@lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/socketcan-core
>   