| Message ID | 1605326982-2487-1-git-send-email-vfedorenko@novek.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/tls: fix corrupted data in recvmsg | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sat, 14 Nov 2020 07:09:42 +0300 Vadim Fedorenko wrote: > If tcp socket has more data than Encrypted Handshake Message then > tls_sw_recvmsg will try to decrypt next record instead of returning > full control message to userspace as mentioned in comment. The next > message - usually Application Data - gets corrupted because it uses > zero copy for decryption that's why the data is not stored in skb > for next iteration. Disable zero copy for this case. > > Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records") > Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> Do you have some BPF in the mix? I don't see how we would try to go past a non-data record otherwise, since the loop ends like this: if (tls_sw_advance_skb(sk, skb, chunk)) { /* Return full control message to * userspace before trying to parse * another message type */ msg->msg_flags |= MSG_EOR; if (ctx->control != TLS_RECORD_TYPE_DATA) goto recv_end; } else { break; }
No, I don't have any BPFs in test. If we have Application Data in TCP queue then tls_sw_advance_skb will change ctx->control from 0x16 to 0x17 (TLS_RECORD_TYPE_DATA) and the loop will continue. The patched if will make zc = true and data will be decrypted into msg->msg_iter. After that the loop will break on: if (!control) control = tlm->control; else if (control != tlm->control) goto recv_end; and the data will be lost. Next call to recvmsg will find ctx->decrypted set to true and will copy the unencrypted data from skb assuming that it has been decrypted already. The patch that I put into Fixes: changed the check you mentioned to ctx->control, but originally it was checking the value of control that was stored before calling to tls_sw_advance_skb. On 15.11.2020 02:12, Jakub Kicinski wrote: > On Sat, 14 Nov 2020 07:09:42 +0300 Vadim Fedorenko wrote: >> If tcp socket has more data than Encrypted Handshake Message then >> tls_sw_recvmsg will try to decrypt next record instead of returning >> full control message to userspace as mentioned in comment. The next >> message - usually Application Data - gets corrupted because it uses >> zero copy for decryption that's why the data is not stored in skb >> for next iteration. Disable zero copy for this case. >> >> Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records") >> Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> > Do you have some BPF in the mix? > > I don't see how we would try to go past a non-data record otherwise, > since the loop ends like this: > > if (tls_sw_advance_skb(sk, skb, chunk)) { > /* Return full control message to > * userspace before trying to parse > * another message type > */ > msg->msg_flags |= MSG_EOR; > if (ctx->control != TLS_RECORD_TYPE_DATA) > goto recv_end; > } else { > break; > }
Please don't top post. On Sun, 15 Nov 2020 02:26:30 +0000 Vadim Fedorenko wrote: > No, I don't have any BPFs in test. > If we have Application Data in TCP queue then tls_sw_advance_skb > will change ctx->control from 0x16 to 0x17 (TLS_RECORD_TYPE_DATA) > and the loop will continue. Ah! Missed that, unpausing the parser will make it serve us another message and overwrite ctx. > The patched if will make zc = true and > data will be decrypted into msg->msg_iter. > After that the loop will break on: > if (!control) > control = tlm->control; > else if (control != tlm->control) > goto recv_end; > > and the data will be lost. > Next call to recvmsg will find ctx->decrypted set to true and will > copy the unencrypted data from skb assuming that it has been decrypted > already. > > The patch that I put into Fixes: changed the check you mentioned to > ctx->control, but originally it was checking the value of control that > was stored before calling to tls_sw_advance_skb. Is there a reason why we wouldn't just go back to checking the stored control? Or better - put your condition there (control != ctx->control)? Decrypting the next record seems unnecessary given we can't return it.
On 15.11.2020 03:54, Jakub Kicinski wrote: > Please don't top post. > > On Sun, 15 Nov 2020 02:26:30 +0000 Vadim Fedorenko wrote: >> No, I don't have any BPFs in test. >> If we have Application Data in TCP queue then tls_sw_advance_skb >> will change ctx->control from 0x16 to 0x17 (TLS_RECORD_TYPE_DATA) >> and the loop will continue. > Ah! Missed that, unpausing the parser will make it serve us another > message and overwrite ctx. > >> The patched if will make zc = true and >> data will be decrypted into msg->msg_iter. >> After that the loop will break on: >> if (!control) >> control = tlm->control; >> else if (control != tlm->control) >> goto recv_end; >> >> and the data will be lost. >> Next call to recvmsg will find ctx->decrypted set to true and will >> copy the unencrypted data from skb assuming that it has been decrypted >> already. >> >> The patch that I put into Fixes: changed the check you mentioned to >> ctx->control, but originally it was checking the value of control that >> was stored before calling to tls_sw_advance_skb. > Is there a reason why we wouldn't just go back to checking the stored > control? Or better - put your condition there (control != ctx->control)? > Decrypting the next record seems unnecessary given we can't return it. Going back to checking stored control is working in TLS 1.2 but I cannot test it with TLS 1.3. But it looks like it will work too in that case. Variable control should store correct value to pass it in control message and it's not chaged after that. Ok, will send v2 with check reverted, thanks!
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 95ab5545..e040be1 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1808,6 +1808,7 @@ int tls_sw_recvmsg(struct sock *sk, if (to_decrypt <= len && !is_kvec && !is_peek && ctx->control == TLS_RECORD_TYPE_DATA && + (!control || ctx->control == control) && prot->version != TLS_1_3_VERSION && !bpf_strp_enabled) zc = true;
If tcp socket has more data than Encrypted Handshake Message then tls_sw_recvmsg will try to decrypt next record instead of returning full control message to userspace as mentioned in comment. The next message - usually Application Data - gets corrupted because it uses zero copy for decryption that's why the data is not stored in skb for next iteration. Disable zero copy for this case. Fixes: 692d7b5d1f91 ("tls: Fix recvmsg() to be able to peek across multiple records") Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> --- net/tls/tls_sw.c | 1 + 1 file changed, 1 insertion(+)