* [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 10:15 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 10:15 UTC (permalink / raw)
To: Dave Watson
Cc: Aviad Yehezkel, Ilya Lesokhin, David S. Miller, netdev, kernel-janitors
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>
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:
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 10:15 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 10:15 UTC (permalink / raw)
To: Dave Watson
Cc: Aviad Yehezkel, Ilya Lesokhin, David S. Miller, netdev, kernel-janitors
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>
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:
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:15 ` Dan Carpenter
@ 2017-06-23 10:31 ` Joe Perches
-1 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2017-06-23 10:31 UTC (permalink / raw)
To: Dan Carpenter, Dave Watson
Cc: Aviad Yehezkel, Ilya Lesokhin, David S. Miller, netdev, kernel-janitors
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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 10:31 ` Joe Perches
0 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2017-06-23 10:31 UTC (permalink / raw)
To: Dan Carpenter, Dave Watson
Cc: Aviad Yehezkel, Ilya Lesokhin, David S. Miller, netdev, kernel-janitors
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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:31 ` Joe Perches
@ 2017-06-23 10:34 ` Dan Carpenter
-1 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 10:34 UTC (permalink / raw)
To: Joe Perches
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 10:34 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 10:34 UTC (permalink / raw)
To: Joe Perches
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:31 ` Joe Perches
@ 2017-06-23 10:36 ` Dan Carpenter
-1 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 10:36 UTC (permalink / raw)
To: Joe Perches
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 10:36 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 10:36 UTC (permalink / raw)
To: Joe Perches
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:36 ` Dan Carpenter
@ 2017-06-23 10:58 ` Joe Perches
-1 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2017-06-23 10:58 UTC (permalink / raw)
To: Dan Carpenter
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 10:58 ` Joe Perches
0 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2017-06-23 10:58 UTC (permalink / raw)
To: Dan Carpenter
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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?
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:58 ` Joe Perches
@ 2017-06-23 11:29 ` Dan Carpenter
-1 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 11:29 UTC (permalink / raw)
To: Joe Perches
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 11:29 ` Dan Carpenter
0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2017-06-23 11:29 UTC (permalink / raw)
To: Joe Perches
Cc: Dave Watson, Aviad Yehezkel, Ilya Lesokhin, David S. Miller,
netdev, kernel-janitors
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:15 ` Dan Carpenter
@ 2017-06-23 16:12 ` Dave Watson
-1 siblings, 0 replies; 16+ messages in thread
From: Dave Watson @ 2017-06-23 16:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Aviad Yehezkel, Ilya Lesokhin, David S. Miller, netdev, kernel-janitors
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:
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 16:12 ` Dave Watson
0 siblings, 0 replies; 16+ messages in thread
From: Dave Watson @ 2017-06-23 16:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Aviad Yehezkel, Ilya Lesokhin, David S. Miller, netdev, kernel-janitors
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:
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
2017-06-23 10:15 ` Dan Carpenter
@ 2017-06-23 18:20 ` David Miller
-1 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2017-06-23 18:20 UTC (permalink / raw)
To: dan.carpenter; +Cc: davejwatson, aviadye, ilyal, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH -net] tls: return -EFAULT if copy_to_user() fails
@ 2017-06-23 18:20 ` David Miller
0 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2017-06-23 18:20 UTC (permalink / raw)
To: dan.carpenter; +Cc: davejwatson, aviadye, ilyal, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-06-23 18:20 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-23 10:15 [PATCH -net] tls: return -EFAULT if copy_to_user() fails Dan Carpenter
2017-06-23 10:15 ` Dan Carpenter
2017-06-23 10:31 ` Joe Perches
2017-06-23 10:31 ` Joe Perches
2017-06-23 10:34 ` Dan Carpenter
2017-06-23 10:34 ` Dan Carpenter
2017-06-23 10:36 ` Dan Carpenter
2017-06-23 10:36 ` Dan Carpenter
2017-06-23 10:58 ` Joe Perches
2017-06-23 10:58 ` Joe Perches
2017-06-23 11:29 ` Dan Carpenter
2017-06-23 11:29 ` Dan Carpenter
2017-06-23 16:12 ` Dave Watson
2017-06-23 16:12 ` Dave Watson
2017-06-23 18:20 ` David Miller
2017-06-23 18:20 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.