Message-ID: <1712a4c3-459e-bb25-5b02-fc3fb96145f2@talpey.com>
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