[PATCH v2 net] net/tls: fix encryption error checking

[PATCH v2 net] net/tls: fix encryption error checking

[Vadim Fedorenko]

bpf_exec_tx_verdict() can return negative value for copied
variable. In that case this value will be pushed back to caller
and the real error code will be lost. Fix it using signed type and
checking for positive value.

Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
---
 net/tls/tls_sw.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e23f94a..57f8082 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -780,7 +780,7 @@ static int tls_push_record(struct sock *sk, int flags,
 
 static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
 			       bool full_record, u8 record_type,
-			       size_t *copied, int flags)
+			       ssize_t *copied, int flags)
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
@@ -916,7 +916,8 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	unsigned char record_type = TLS_RECORD_TYPE_DATA;
 	bool is_kvec = iov_iter_is_kvec(&msg->msg_iter);
 	bool eor = !(msg->msg_flags & MSG_MORE);
-	size_t try_to_copy, copied = 0;
+	size_t try_to_copy;
+	ssize_t copied = 0;
 	struct sk_msg *msg_pl, *msg_en;
 	struct tls_rec *rec;
 	int required_size;
@@ -1118,7 +1119,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 
 	release_sock(sk);
 	mutex_unlock(&tls_ctx->tx_lock);
-	return copied ? copied : ret;
+	return copied > 0 ? copied : ret;
 }
 
 static int tls_sw_do_sendpage(struct sock *sk, struct page *page,
@@ -1132,7 +1133,7 @@ static int tls_sw_do_sendpage(struct sock *sk, struct page *page,
 	struct sk_msg *msg_pl;
 	struct tls_rec *rec;
 	int num_async = 0;
-	size_t copied = 0;
+	ssize_t copied = 0;
 	bool full_record;
 	int record_room;
 	int ret = 0;
@@ -1234,7 +1235,7 @@ static int tls_sw_do_sendpage(struct sock *sk, struct page *page,
 	}
 sendpage_end:
 	ret = sk_stream_error(sk, flags, ret);
-	return copied ? copied : ret;
+	return copied > 0 ? copied : ret;
 }
 
 int tls_sw_sendpage_locked(struct sock *sk, struct page *page,
-- 
1.8.3.1

Re: [PATCH v2 net] net/tls: fix encryption error checking

[Jakub Kicinski]

On Tue, 19 May 2020 13:20:43 +0300 Vadim Fedorenko wrote:
> bpf_exec_tx_verdict() can return negative value for copied
> variable. In that case this value will be pushed back to caller
> and the real error code will be lost. Fix it using signed type and
> checking for positive value.
> 
> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
> Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>

If the error encountered is transient we will still drop some data from
the stream, because the record that was freed may have included data
from a previous send call. Still, cleaning up the error code seems like
an improvement.

John, do you have an opinion on this?

Re: [PATCH v2 net] net/tls: fix encryption error checking

[Vadim Fedorenko]

On 20.05.2020 01:04, Jakub Kicinski wrote:
> On Tue, 19 May 2020 13:20:43 +0300 Vadim Fedorenko wrote:
>> bpf_exec_tx_verdict() can return negative value for copied
>> variable. In that case this value will be pushed back to caller
>> and the real error code will be lost. Fix it using signed type and
>> checking for positive value.
>>
>> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
>> Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
>> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
> If the error encountered is transient we will still drop some data from
> the stream, because the record that was freed may have included data
> from a previous send call. Still, cleaning up the error code seems like
> an improvement.
>
> John, do you have an opinion on this?

Jakub, maybe it is better to free only in case of fatal encryption error? I mean
when sk->sk_err is EBADMSG. Because in case of ENOMEM we will iterate to
alloc_payload and in case of ENOSPC we will return good return code and send
open_rec again on next call. The EBADMSG state is the only fatal state that needs
freeing of allocated record.

Re: [PATCH v2 net] net/tls: fix encryption error checking

[Jakub Kicinski]

On Wed, 20 May 2020 01:49:33 +0300 Vadim Fedorenko wrote:
> On 20.05.2020 01:04, Jakub Kicinski wrote:
> > On Tue, 19 May 2020 13:20:43 +0300 Vadim Fedorenko wrote:  
> >> bpf_exec_tx_verdict() can return negative value for copied
> >> variable. In that case this value will be pushed back to caller
> >> and the real error code will be lost. Fix it using signed type and
> >> checking for positive value.
> >>
> >> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
> >> Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
> >> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>  
> > If the error encountered is transient we will still drop some data from
> > the stream, because the record that was freed may have included data
> > from a previous send call. Still, cleaning up the error code seems like
> > an improvement.
> >
> > John, do you have an opinion on this?  
> 
> Jakub, maybe it is better to free only in case of fatal encryption error? I mean
> when sk->sk_err is EBADMSG. Because in case of ENOMEM we will iterate to
> alloc_payload and in case of ENOSPC we will return good return code and send
> open_rec again on next call. The EBADMSG state is the only fatal state that needs
> freeing of allocated record.

Sounds good!

Re: [PATCH v2 net] net/tls: fix encryption error checking

[Vadim Fedorenko]

On 20.05.2020 02:10, Jakub Kicinski wrote:
> On Wed, 20 May 2020 01:49:33 +0300 Vadim Fedorenko wrote:
>> On 20.05.2020 01:04, Jakub Kicinski wrote:
>>> On Tue, 19 May 2020 13:20:43 +0300 Vadim Fedorenko wrote:
>>>> bpf_exec_tx_verdict() can return negative value for copied
>>>> variable. In that case this value will be pushed back to caller
>>>> and the real error code will be lost. Fix it using signed type and
>>>> checking for positive value.
>>>>
>>>> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
>>>> Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
>>>> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
>>> If the error encountered is transient we will still drop some data from
>>> the stream, because the record that was freed may have included data
>>> from a previous send call. Still, cleaning up the error code seems like
>>> an improvement.
>>>
>>> John, do you have an opinion on this?
>> Jakub, maybe it is better to free only in case of fatal encryption error? I mean
>> when sk->sk_err is EBADMSG. Because in case of ENOMEM we will iterate to
>> alloc_payload and in case of ENOSPC we will return good return code and send
>> open_rec again on next call. The EBADMSG state is the only fatal state that needs
>> freeing of allocated record.
> Sounds good!

Ok, will send v3 with both fixes.