Messages in this thread | | | Date | Wed, 16 Nov 2022 11:14:18 -0500 | Subject | Re: [PATCH] cifs: Fix problem with encrypted RDMA data read | From | Tom Talpey <> |
| |
On 11/16/2022 10:44 AM, Stefan Metzmacher wrote: > Am 16.11.22 um 16:41 schrieb Tom Talpey: >> On 11/16/2022 3:36 AM, Stefan Metzmacher wrote: >>> Am 16.11.22 um 06:19 schrieb Namjae Jeon: >>>> 2022-11-16 9:57 GMT+09:00, Stefan Metzmacher <metze@samba.org>: >>>>> Hi David, >>>>> >>>>> see below... >>>>> >>>>>> When the cifs client is talking to the ksmbd server by RDMA and >>>>>> the ksmbd >>>>>> server has "smb3 encryption = yes" in its config file, the normal PDU >>>>>> stream is encrypted, but the directly-delivered data isn't in the >>>>>> stream >>>>>> (and isn't encrypted), but is rather delivered by DDP/RDMA packets >>>>>> (at >>>>>> least with IWarp). >>>>>> >>>>>> Currently, the direct delivery fails with: >>>>>> >>>>>> buf can not contain only a part of read data >>>>>> WARNING: CPU: 0 PID: 4619 at fs/cifs/smb2ops.c:4731 >>>>>> handle_read_data+0x393/0x405 >>>>>> ... >>>>>> RIP: 0010:handle_read_data+0x393/0x405 >>>>>> ... >>>>>> smb3_handle_read_data+0x30/0x37 >>>>>> receive_encrypted_standard+0x141/0x224 >>>>>> cifs_demultiplex_thread+0x21a/0x63b >>>>>> kthread+0xe7/0xef >>>>>> ret_from_fork+0x22/0x30 >>>>>> >>>>>> The problem apparently stemming from the fact that it's trying to >>>>>> manage >>>>>> the decryption, but the data isn't in the smallbuf, the bigbuf or the >>>>>> page >>>>>> array). >>>>>> >>>>>> This can be fixed simply by inserting an extra case into >>>>>> handle_read_data() >>>>>> that checks to see if use_rdma_mr is true, and if it is, just setting >>>>>> rdata->got_bytes to the length of data delivered and allowing normal >>>>>> continuation. >>>>>> >>>>>> This can be seen in an IWarp packet trace. With the upstream >>>>>> code, it >>>>>> does >>>>>> a DDP/RDMA packet, which produces the warning above and then retries, >>>>>> retrieving the data inline, spread across several SMBDirect >>>>>> messages that >>>>>> get glued together into a single PDU. With the patch applied, >>>>>> only the >>>>>> DDP/RDMA packet is seen. >>>>>> >>>>>> Note that this doesn't happen if the server isn't told to encrypt >>>>>> stuff >>>>>> and >>>>>> it does also happen with softRoCE. >>>>>> >>>>>> Signed-off-by: David Howells <dhowells@redhat.com> >>>>>> cc: Steve French <smfrench@gmail.com> >>>>>> cc: Tom Talpey <tom@talpey.com> >>>>>> cc: Long Li <longli@microsoft.com> >>>>>> cc: Namjae Jeon <linkinjeon@kernel.org> >>>>>> cc: Stefan Metzmacher <metze@samba.org> >>>>>> cc: linux-cifs@vger.kernel.org >>>>>> --- >>>>>> >>>>>> fs/cifs/smb2ops.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c >>>>>> index 880cd494afea..8d459f60f27b 100644 >>>>>> --- a/fs/cifs/smb2ops.c >>>>>> +++ b/fs/cifs/smb2ops.c >>>>>> @@ -4726,6 +4726,9 @@ handle_read_data(struct TCP_Server_Info >>>>>> *server, >>>>>> struct mid_q_entry *mid, >>>>>> iov.iov_base = buf + data_offset; >>>>>> iov.iov_len = data_len; >>>>>> iov_iter_kvec(&iter, WRITE, &iov, 1, data_len); >>>>>> + } else if (use_rdma_mr) { >>>>>> + /* The data was delivered directly by RDMA. */ >>>>>> + rdata->got_bytes = data_len; >>>>>> } else { >>>>>> /* read response payload cannot be in both buf and >>>>>> pages */ >>>>>> WARN_ONCE(1, "buf can not contain only a part of read >>>>>> data"); >>>>> >>>>> I'm not sure I understand why this would fix anything when >>>>> encryption is >>>>> enabled. >>>>> >>>>> Is the payload still be offloaded as plaintext? Otherwise we >>>>> wouldn't have >>>>> use_rdma_mr... >>>>> So this rather looks like a fix for the non encrypted case. >>>> ksmbd doesn't encrypt RDMA payload on read/write operation, Currently >>>> only smb2 response is encrypted for this. And as you pointed out, We >>>> need to implement SMB2 RDMA Transform to encrypt it. >>> >>> I haven't tested against a windows server yet, but my hope would be that >>> and encrypted request with SMB2_CHANNEL_RDMA_V1* receive >>> NT_STATUS_ACCESS_DENIED or something similar... >>> >>> Is someone able to check that against Windows? >> >> It's not going to fail, because it's perfectly legal per the protocol. >> And the new SMB3 extension to perform pre-encryption of RDMA payload >> is not a solution, because it's only supported by one server (Windows >> 22H2) and in any case it does not alter the transfer model. The client >> will see the same two-part response (headers in the inline portion, >> data via RDMA), so this same code will be entered when processing it. >> >> I think David's change is on the right track because it actually >> processes the response. I'm a little bit skeptical of the got_bytes >> override however, still digging into that. >> >>> But the core of it is a client security problem, shown in David's >>> capture in frame 100. >> >> Sorry, what's the security problem? Both the client and server appear >> to be implementing the protocol itself correctly. > > Data goes in plaintext over the wire and a share that requires encryption!
That's a server issue, not the client. The server is the one that returned the plaintext data via RDMA. Changing the client to avoid such a request doesn't close that hole. It's an important policy question, of course.
I still think the client needs to handle the is_rdma_mr case, along the lines of David's fix. The code looks like a vestige of TCP-only response processing.
Tom.
| |