| Message ID | 166855224228.1998592.2212551359609792175.stgit@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Fix problem with encrypted RDMA data read | expand |
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. Before smbd_register_mr() is called we typically have a check like this: if (server->rdma && !server->sign && wdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) { I'm wondering if server->sign is true for the encryption case, otherwise we would have to add a !encrypt check in addition as we should never use RDMA offload for encrypted connections. Latest Windows servers allow encrypted/signed offload, but that needs to be negotiated via MS-SMB2 2.2.3.1.6 SMB2_RDMA_TRANSFORM_CAPABILITIES, see https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/52b74a74-9838-4f51-b2b0-efeb23bd79d6 And SMB2_READFLAG_RESPONSE_RDMA_TRANSFORM in MS-SMB2 2.2.20 SMB2 READ Response https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/3e3d2f2c-0e2f-41ea-ad07-fbca6ffdfd90 As well as SMB2_CHANNEL_RDMA_TRANSFORM in 2.2.21 SMB2 WRITE Request https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/e7046961-3318-4350-be2a-a8d69bb59ce8 But none of this is implemented in Linux yet. metze
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. > > Before smbd_register_mr() is called we typically have a check like this: > > if (server->rdma && !server->sign && wdata->bytes >= > server->smbd_conn->rdma_readwrite_threshold) { > > I'm wondering if server->sign is true for the encryption case, otherwise > we would have to add a !encrypt check in addition as we should never use > RDMA offload for encrypted connections. > > Latest Windows servers allow encrypted/signed offload, but that needs to be > negotiated via MS-SMB2 2.2.3.1.6 SMB2_RDMA_TRANSFORM_CAPABILITIES, see > https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/52b74a74-9838-4f51-b2b0-efeb23bd79d6 > And SMB2_READFLAG_RESPONSE_RDMA_TRANSFORM in MS-SMB2 2.2.20 SMB2 READ > Response > https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/3e3d2f2c-0e2f-41ea-ad07-fbca6ffdfd90 > As well as SMB2_CHANNEL_RDMA_TRANSFORM in 2.2.21 SMB2 WRITE Request > https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/e7046961-3318-4350-be2a-a8d69bb59ce8 > But none of this is implemented in Linux yet. > > metze >
Stefan Metzmacher <metze@samba.org> wrote: > 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. The "inline"[*] PDUs are encrypted, but the direct RDMA data transmission is not. I'm not sure if this is a bug in ksmbd. As I understand it, encrypting and decrypting the directly transferred data would need to be done by the NIC, not the cifs driver. David [*] I don't know the correct RDMA terminology for these things.
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? But the core of it is a client security problem, shown in David's capture in frame 100. metze
Am 16.11.22 um 08:00 schrieb David Howells: > Stefan Metzmacher <metze@samba.org> wrote: > >> 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. > > The "inline"[*] PDUs are encrypted, but the direct RDMA data transmission is > not. I'm not sure if this is a bug in ksmbd. It's a bug in the client! > As I understand it, encrypting and decrypting the directly transferred > data would need to be done by the NIC, not the cifs driver. No, the encryption needs to happen above the RDMA/NIC layer. metze
Stefan Metzmacher <metze@samba.org> wrote: > > Stefan Metzmacher <metze@samba.org> wrote: > > > >> 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. > > The "inline"[*] PDUs are encrypted, but the direct RDMA data transmission is > > not. I'm not sure if this is a bug in ksmbd. > > It's a bug in the client! Well, if you can fix it the right way, I can test the patch. I'm not sure what that would be if not what I suggested. David
Am 16.11.22 um 13:14 schrieb David Howells: > Stefan Metzmacher <metze@samba.org> wrote: > >>> Stefan Metzmacher <metze@samba.org> wrote: >>> >>>> 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. >>> The "inline"[*] PDUs are encrypted, but the direct RDMA data transmission is >>> not. I'm not sure if this is a bug in ksmbd. >> >> It's a bug in the client! > > Well, if you can fix it the right way, I can test the patch. I'm not sure > what that would be if not what I suggested. As written in the other mail something like this: fs/cifs/smb2pdu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index a5695748a89b..d478b0a89890 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -4090,7 +4090,7 @@ smb2_new_read_req(void **buf, unsigned int *total_len, * If we want to do a RDMA write, fill in and append * smbd_buffer_descriptor_v1 to the end of read request */ - if (server->rdma && rdata && !server->sign && + if (server->rdma && rdata && !server->sign && !smb3_encryption_required(io_parms->tcon) && rdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) { struct smbd_buffer_descriptor_v1 *v1; @@ -4517,8 +4517,8 @@ smb2_async_writev(struct cifs_writedata *wdata, * If we want to do a server RDMA read, fill in and append * smbd_buffer_descriptor_v1 to the end of write request */ - if (server->rdma && !server->sign && wdata->bytes >= - server->smbd_conn->rdma_readwrite_threshold) { + if (server->rdma && !server->sign && !smb3_encryption_required(tcon) && + wdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) { struct smbd_buffer_descriptor_v1 *v1; bool need_invalidate = server->dialect == SMB30_PROT_ID;
Okay, that works - though, probably unsurprisingly, it's 5x slower.
If you want to submit this patch, you can add:
Tested-by: David Howells <dhowells@redhat.com>
David
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. Tom.
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! metze
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.
Stefan Metzmacher <metze@samba.org> wrote: > But the core of it is a client security problem, shown in David's capture in > frame 100. Does the client actually know about the server's settings, though? David
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@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. 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... metze
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@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. > > 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.
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");
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(+)