lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 16 Nov 2022 16:21:10 -0500 From: Tom Talpey <tom@...pey.com> To: Stefan Metzmacher <metze@...ba.org>, Namjae Jeon <linkinjeon@...nel.org> Cc: David Howells <dhowells@...hat.com>, smfrench@...il.com, Long Li <longli@...rosoft.com>, linux-cifs@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] cifs: Fix problem with encrypted RDMA data read On 11/16/2022 2:53 PM, Stefan Metzmacher wrote: > Am 16.11.22 um 17:14 schrieb 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@...ba.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@...hat.com> >>>>>>>> cc: Steve French <smfrench@...il.com> >>>>>>>> cc: Tom Talpey <tom@...pey.com> >>>>>>>> cc: Long Li <longli@...rosoft.com> >>>>>>>> cc: Namjae Jeon <linkinjeon@...nel.org> >>>>>>>> cc: Stefan Metzmacher <metze@...ba.org> >>>>>>>> cc: linux-cifs@...r.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. > > No, it's the client how decides to use SMB2_CHANNEL_RDMA_V1* or > SMB2_CHANNEL_NONE. And for any read or write over an signed or encrypted > connection > it must use SMB2_CHANNEL_NONE! Otherwise the clients memory can be > written or read > by any untrusted machine in the middle. > > MS-SMB2 says this: > > 3.2.4.6 Application Requests Reading from a File or Named Pipe > ... > If the Connection is established in RDMA mode and the size of any single > operation exceeds an > implementation-specific threshold <138>, and if > Open.TreeConnect.Session.SigningRequired and > Open.TreeConnect.Session.EncryptData are both FALSE, then the interface > in [MS-SMBD] section > 3.1.4.3 Register Buffer MUST be used to register the buffer provided by > the calling application on the > Connection with write permissions, which will receive the data to be > read. The returned list of > SMB_DIRECT_BUFFER_DESCRIPTOR_V1 structures MUST be stored in > Request.BufferDescriptorList. The following fields of the request MUST > be initialized as follows: > ... > > 3.2.4.7 Application Requests Writing to a File or Named Pipe > ... > If the connection is not established in RDMA mode or if the size of the > operation is less than or equal > to an implementation-specific threshold <141>or if either > Open.TreeConnect.Session.SigningRequired or > Open.TreeConnect.Session.EncryptData is > TRUE, the following fields of the request MUST be initialized as follows: > - If Connection.Dialect belongs to the SMB 3.x dialect family, > - The Channel field MUST be set to SMB2_CHANNEL_NONE. > - The WriteChannelInfoOffset field MUST be set to 0. > - The WriteChannelInfoLength field MUST be set to 0. > > For sure it would be great if servers would also reject > SMB2_CHANNEL_RDMA_V1* > on signed/encrypted connections with INVALID_PARAMETER or ACCESS_DENIED, > buth the problem we currently see is a client security problem. > >> 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. > > I'm not saying David's change is wrong, but I think it has nothing todo > with encrypted or signed traffic... Ok, I agree that this uncovered two issues. And they're independent. I'm digging into dead code and questionable read response parsing in smb2ops.c. I guess we can be thankful it failed. :) I've also asked Microsoft for clarification on the Windows SMB3 server behavior regarding non-encrypted RDMA. Tom.
Powered by blists - more mailing lists