Re: [PATCH] cifs: Fix problem with encrypted RDMA data read
From: Tom Talpey
Date: Wed Nov 16 2022 - 16:21:24 EST
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@xxxxxxxxx>:
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@xxxxxxxxxx>
cc: Steve French <smfrench@xxxxxxxxx>
cc: Tom Talpey <tom@xxxxxxxxxx>
cc: Long Li <longli@xxxxxxxxxxxxx>
cc: Namjae Jeon <linkinjeon@xxxxxxxxxx>
cc: Stefan Metzmacher <metze@xxxxxxxxx>
cc: linux-cifs@xxxxxxxxxxxxxxx
---
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.