Message ID | 20170623101544.c5bvwe5cd6e46bd5@mwanda |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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?
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
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
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: EFAULT The 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?
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
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 <dan.carpenter@oracle.com> Acked-by: Dave Watson <davejwatson@fb.com> 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:
From: Dan Carpenter <dan.carpenter@oracle.com> 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.carpenter@oracle.com> 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.
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:
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.carpenter@oracle.com>