Re: [PATCH] cifs: Fix problem with encrypted RDMA data read

[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.

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.