Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[David Miller]

From: Dan Carpenter 
Date: Fri, 23 Jun 2017 13:15:44 +0300

> The copy_to_user() function returns the number of bytes remaining but we
> want to return -EFAULT here.
> 
> Fixes: 3c4d7559159b ("tls: kernel TLS support")
> Signed-off-by: Dan Carpenter 

Dan, I happened to realize that tls is only in net-next, but please
indicate the target tree properly in your Subject lines in the
future.

Applied, thanks.

Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Dave Watson]

On 06/23/17 01:15 PM, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes remaining but we
> want to return -EFAULT here.
> 
> Fixes: 3c4d7559159b ("tls: kernel TLS support")
> Signed-off-by: Dan Carpenter 

Acked-by: Dave Watson 

Yes, -EFAULT seems like the correct choice here, the return from
copy_to_user isn't useful.  Thanks

> 
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 2ebc328bda96..a03130a47b85 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -273,7 +273,8 @@ static int do_tls_getsockopt_tx(struct sock *sk, char 
> __user *optval,
>   }
>  
>   if (len == sizeof(crypto_info)) {
> - rc = copy_to_user(optval, crypto_info, sizeof(*crypto_info));
> + if (copy_to_user(optval, crypto_info, sizeof(*crypto_info)))
> + rc = -EFAULT;
>   goto out;
>   }
>  
> @@ -293,9 +294,10 @@ static int do_tls_getsockopt_tx(struct sock *sk, char 
> __user *optval,
>   memcpy(crypto_info_aes_gcm_128->iv, ctx->iv,
>  TLS_CIPHER_AES_GCM_128_IV_SIZE);
>   release_sock(sk);
> - rc = copy_to_user(optval,
> -   crypto_info_aes_gcm_128,
> -   sizeof(*crypto_info_aes_gcm_128));
> + if (copy_to_user(optval,
> +  crypto_info_aes_gcm_128,
> +  sizeof(*crypto_info_aes_gcm_128)))
> + rc = -EFAULT;
>   break;
>   }
>   default:

Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Dan Carpenter]

On Fri, Jun 23, 2017 at 03:58:35AM -0700, Joe Perches wrote:
> getsockopt says:
> 
> For getsockopt(), optlen is a value-result argument, initially containing the 
> size
> of the buffer pointed to by optval, and modified on return to indicate the 
> actual
> size of the value returned

In the original code, it's not returning the "actual size of the value
returned". It's returning a smaller or equal value... The man page is
correct that this is how some getsockopts work, of course.  But here
-EFAULT is expected.

regards,
dan carpenter

Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Joe Perches]

On Fri, 2017-06-23 at 13:36 +0300, Dan Carpenter wrote:
> On Fri, Jun 23, 2017 at 03:31:44AM -0700, Joe Perches wrote:
> > On Fri, 2017-06-23 at 13:15 +0300, Dan Carpenter wrote:
> > > The copy_to_user() function returns the number of bytes remaining but we
> > > want to return -EFAULT here.
> > 
> > because?
> > 
> 
> Rhetorical questions don't work over email.  Are you honestly confused
> by this patch?

There doesn't seem to be a fault here, just a
return of less than the expected number of bytes.

It's non-obvious why -EFAULT is the appropriate
return value.

Why is changing the return value from number of
bytes transferred, even if less than desired,
the right thing to do?  Your commit message
doesn't describe any rationale.

getsockopt says:

For getsockopt(), optlen is a value-result argument, initially containing the 
size
of the buffer pointed to by optval, and modified on return to indicate the 
actual
size of the value returned

The generic EFAULT description in getsockopt is:

   EFAULTThe  address  pointed  to by optval is not in a valid part of 
the
 process address space.  For getsockopt(), this error may also  
be
 returned  if optlen is not in a valid part of the process 
address
 space.

Is tls different?

Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Dan Carpenter]

On Fri, Jun 23, 2017 at 03:31:44AM -0700, Joe Perches wrote:
> On Fri, 2017-06-23 at 13:15 +0300, Dan Carpenter wrote:
> > The copy_to_user() function returns the number of bytes remaining but we
> > want to return -EFAULT here.
> 
> because?
> 

Rhetorical questions don't work over email.  Are you honestly confused
by this patch?

regards,
dan carpenter

Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Dan Carpenter]

On Fri, Jun 23, 2017 at 03:31:44AM -0700, Joe Perches wrote:
> On Fri, 2017-06-23 at 13:15 +0300, Dan Carpenter wrote:
> > The copy_to_user() function returns the number of bytes remaining but we
> > want to return -EFAULT here.
> 
> because?

Because it's a failure path?

regards,
dan carpenter

Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Joe Perches]

On Fri, 2017-06-23 at 13:15 +0300, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes remaining but we
> want to return -EFAULT here.

because?

[PATCH -net] tls: return -EFAULT if copy_to_user() fails

[Dan Carpenter]

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

Fixes: 3c4d7559159b ("tls: kernel TLS support")
Signed-off-by: Dan Carpenter 

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 2ebc328bda96..a03130a47b85 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -273,7 +273,8 @@ static int do_tls_getsockopt_tx(struct sock *sk, char 
__user *optval,
}
 
if (len == sizeof(crypto_info)) {
-   rc = copy_to_user(optval, crypto_info, sizeof(*crypto_info));
+   if (copy_to_user(optval, crypto_info, sizeof(*crypto_info)))
+   rc = -EFAULT;
goto out;
}
 
@@ -293,9 +294,10 @@ static int do_tls_getsockopt_tx(struct sock *sk, char 
__user *optval,
memcpy(crypto_info_aes_gcm_128->iv, ctx->iv,
   TLS_CIPHER_AES_GCM_128_IV_SIZE);
release_sock(sk);
-   rc = copy_to_user(optval,
- crypto_info_aes_gcm_128,
- sizeof(*crypto_info_aes_gcm_128));
+   if (copy_to_user(optval,
+crypto_info_aes_gcm_128,
+sizeof(*crypto_info_aes_gcm_128)))
+   rc = -EFAULT;
break;
}
default: