* [PATCH] net/tls: fix encryption error checking
@ 2020-05-16 23:48 Vadim Fedorenko
2020-05-18 22:30 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Vadim Fedorenko @ 2020-05-16 23:48 UTC (permalink / raw)
To: Boris Pismenny, Aviad Yehezkel, Daniel Borkmann, Jakub Kicinski
Cc: netdev, Vadim Fedorenko
tls_push_record can return -EAGAIN because of tcp layer. In that
case open_rec is already in the tx_record list and should not be
freed.
Also the record size can be more than the size requested to write
in tls_sw_do_sendpage(). That leads to overflow of copied variable
and wrong return code.
Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error")
Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru>
---
net/tls/tls_sw.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index e23f94a..d4acbd1 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -796,7 +796,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
psock = sk_psock_get(sk);
if (!psock || !policy) {
err = tls_push_record(sk, flags, record_type);
- if (err && err != -EINPROGRESS) {
+ if (err && err != -EINPROGRESS && err != -EAGAIN) {
*copied -= sk_msg_free(sk, msg);
tls_free_open_rec(sk);
}
@@ -824,7 +824,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
switch (psock->eval) {
case __SK_PASS:
err = tls_push_record(sk, flags, record_type);
- if (err && err != -EINPROGRESS) {
+ if (err && err != -EINPROGRESS && err != -EAGAIN) {
*copied -= sk_msg_free(sk, msg);
tls_free_open_rec(sk);
goto out_err;
@@ -1132,7 +1132,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 +1234,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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net/tls: fix encryption error checking 2020-05-16 23:48 [PATCH] net/tls: fix encryption error checking Vadim Fedorenko @ 2020-05-18 22:30 ` Jakub Kicinski [not found] ` <CANN+EMpn2ZkquAdK5WFC-bmioSoAbAtNvovXtgTyTHW+-eDPhw@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2020-05-18 22:30 UTC (permalink / raw) To: Vadim Fedorenko; +Cc: Boris Pismenny, Aviad Yehezkel, Daniel Borkmann, netdev On Sun, 17 May 2020 02:48:39 +0300 Vadim Fedorenko wrote: > tls_push_record can return -EAGAIN because of tcp layer. In that > case open_rec is already in the tx_record list and should not be > freed. > Also the record size can be more than the size requested to write > in tls_sw_do_sendpage(). That leads to overflow of copied variable > and wrong return code. > > Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error") > Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> Doesn't this return -EAGAIN back to user space? Meaning even tho we queued the user space will try to send it again? ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CANN+EMpn2ZkquAdK5WFC-bmioSoAbAtNvovXtgTyTHW+-eDPhw@mail.gmail.com>]
* Re: [PATCH] net/tls: fix encryption error checking [not found] ` <CANN+EMpn2ZkquAdK5WFC-bmioSoAbAtNvovXtgTyTHW+-eDPhw@mail.gmail.com> @ 2020-05-18 23:05 ` Vadim Fedorenko 2020-05-18 23:23 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Vadim Fedorenko @ 2020-05-18 23:05 UTC (permalink / raw) To: Jakub Kicinski Cc: Boris Pismenny, Aviad Yehezkel, Daniel Borkmann, Linux Kernel Network Developers On 19.05.2020 01:30, Jakub Kicinski wrote: > > tls_push_record can return -EAGAIN because of tcp layer. In that > > case open_rec is already in the tx_record list and should not be > > freed. > > Also the record size can be more than the size requested to write > > in tls_sw_do_sendpage(). That leads to overflow of copied variable > > and wrong return code. > > > > Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error") > > Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> > > Doesn't this return -EAGAIN back to user space? Meaning even tho we > queued the user space will try to send it again? Before patch it was sending negative value back to user space. After patch it sends the amount of data encrypted in last call. It is checked by: return (copied > 0) ? copied : ret; and returns -EAGAIN only if data is not sent to open record. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/tls: fix encryption error checking 2020-05-18 23:05 ` Vadim Fedorenko @ 2020-05-18 23:23 ` Jakub Kicinski 2020-05-18 23:55 ` Vadim Fedorenko 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2020-05-18 23:23 UTC (permalink / raw) To: Vadim Fedorenko Cc: Boris Pismenny, Aviad Yehezkel, Daniel Borkmann, Linux Kernel Network Developers On Tue, 19 May 2020 02:05:29 +0300 Vadim Fedorenko wrote: > On 19.05.2020 01:30, Jakub Kicinski wrote: > > > tls_push_record can return -EAGAIN because of tcp layer. In that > > > case open_rec is already in the tx_record list and should not be > > > freed. > > > Also the record size can be more than the size requested to write > > > in tls_sw_do_sendpage(). That leads to overflow of copied variable > > > and wrong return code. > > > > > > Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error") > > > Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> > > > > Doesn't this return -EAGAIN back to user space? Meaning even tho we > > queued the user space will try to send it again? > Before patch it was sending negative value back to user space. > After patch it sends the amount of data encrypted in last call. It is checked > by: > return (copied > 0) ? copied : ret; > and returns -EAGAIN only if data is not sent to open record. I see, you're fixing two different bugs in one patch. Could you please split the fixes into two? (BTW no need for parenthesis around the condition in the ternary operator.) I think you need more fixes tags, too. Commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling") already added one instance of the problem, right? What do you think about Pooja's patch to consume the EAGAIN earlier? There doesn't seem to be anything reasonable we can do with the error anyway, not sure there is a point checking for it.. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/tls: fix encryption error checking 2020-05-18 23:23 ` Jakub Kicinski @ 2020-05-18 23:55 ` Vadim Fedorenko 2020-05-19 0:26 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Vadim Fedorenko @ 2020-05-18 23:55 UTC (permalink / raw) To: Jakub Kicinski Cc: Boris Pismenny, Aviad Yehezkel, Daniel Borkmann, Linux Kernel Network Developers On 19.05.2020 02:23, Jakub Kicinski wrote: > On Tue, 19 May 2020 02:05:29 +0300 Vadim Fedorenko wrote: >> On 19.05.2020 01:30, Jakub Kicinski wrote: >>>> tls_push_record can return -EAGAIN because of tcp layer. In that >>>> case open_rec is already in the tx_record list and should not be >>>> freed. >>>> Also the record size can be more than the size requested to write >>>> in tls_sw_do_sendpage(). That leads to overflow of copied variable >>>> and wrong return code. >>>> >>>> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error") >>>> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> >>> Doesn't this return -EAGAIN back to user space? Meaning even tho we >>> queued the user space will try to send it again? >> Before patch it was sending negative value back to user space. >> After patch it sends the amount of data encrypted in last call. It is checked >> by: >> return (copied > 0) ? copied : ret; >> and returns -EAGAIN only if data is not sent to open record. > I see, you're fixing two different bugs in one patch. Could you please > split the fixes into two? (BTW no need for parenthesis around the > condition in the ternary operator.) I think you need more fixes tags, > too. Commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling") > already added one instance of the problem, right? Sure, will split it into two. Also the problem with overflow is possible in tls_sw_sendmsg(). But I'm not sure about correctness of freeing whole open record in bpf_exec_tx_verdict. > What do you think about Pooja's patch to consume the EAGAIN earlier? > There doesn't seem to be anything reasonable we can do with the error > anyway, not sure there is a point checking for it.. Yes, it's a good idea to consume this error earlier. I think it's better to fix tls_push_record() instead of dealing with it every possible caller. So I suggest to accept Pooja's patch and will resend only ssize_t checking fix. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/tls: fix encryption error checking 2020-05-18 23:55 ` Vadim Fedorenko @ 2020-05-19 0:26 ` Jakub Kicinski 0 siblings, 0 replies; 6+ messages in thread From: Jakub Kicinski @ 2020-05-19 0:26 UTC (permalink / raw) To: Vadim Fedorenko Cc: Boris Pismenny, Aviad Yehezkel, Daniel Borkmann, Linux Kernel Network Developers On Tue, 19 May 2020 02:55:16 +0300 Vadim Fedorenko wrote: > On 19.05.2020 02:23, Jakub Kicinski wrote: > > On Tue, 19 May 2020 02:05:29 +0300 Vadim Fedorenko wrote: > >> On 19.05.2020 01:30, Jakub Kicinski wrote: > >>>> tls_push_record can return -EAGAIN because of tcp layer. In that > >>>> case open_rec is already in the tx_record list and should not be > >>>> freed. > >>>> Also the record size can be more than the size requested to write > >>>> in tls_sw_do_sendpage(). That leads to overflow of copied variable > >>>> and wrong return code. > >>>> > >>>> Fixes: d10523d0b3d7 ("net/tls: free the record on encryption error") > >>>> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> > >>> Doesn't this return -EAGAIN back to user space? Meaning even tho we > >>> queued the user space will try to send it again? > >> Before patch it was sending negative value back to user space. > >> After patch it sends the amount of data encrypted in last call. It is checked > >> by: > >> return (copied > 0) ? copied : ret; > >> and returns -EAGAIN only if data is not sent to open record. > > I see, you're fixing two different bugs in one patch. Could you please > > split the fixes into two? (BTW no need for parenthesis around the > > condition in the ternary operator.) I think you need more fixes tags, > > too. Commit d3b18ad31f93 ("tls: add bpf support to sk_msg handling") > > already added one instance of the problem, right? > Sure, will split it into two. Also the problem with overflow is possible in > tls_sw_sendmsg(). But I'm not sure about correctness of freeing whole > open record in bpf_exec_tx_verdict. Yeah, as a matter of fact checking if copied is negative is just papering over the issue. Cleaning up the record so it can be re-submitted again would be better. > > What do you think about Pooja's patch to consume the EAGAIN earlier? > > There doesn't seem to be anything reasonable we can do with the error > > anyway, not sure there is a point checking for it.. > Yes, it's a good idea to consume this error earlier. I think it's better to fix > tls_push_record() instead of dealing with it every possible caller. > > So I suggest to accept Pooja's patch and will resend only ssize_t checking fix. Cool, thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-19 0:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-16 23:48 [PATCH] net/tls: fix encryption error checking Vadim Fedorenko
2020-05-18 22:30 ` Jakub Kicinski
[not found] ` <CANN+EMpn2ZkquAdK5WFC-bmioSoAbAtNvovXtgTyTHW+-eDPhw@mail.gmail.com>
2020-05-18 23:05 ` Vadim Fedorenko
2020-05-18 23:23 ` Jakub Kicinski
2020-05-18 23:55 ` Vadim Fedorenko
2020-05-19 0:26 ` Jakub Kicinski
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.