Date:   Wed, 16 Nov 2022 11:14:18 -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 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.

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.

Powered by blists - more mailing lists