[PATCH 0/3] avoid plaintext rdma offset if encryption is required

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/3] avoid plaintext rdma offset if encryption is required
@ 2023-02-01 12:04 Stefan Metzmacher
  2023-02-01 12:04 ` [PATCH 1/3] cifs: introduce cifs_io_parms in smb2_async_writev() Stefan Metzmacher
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Stefan Metzmacher @ 2023-02-01 12:04 UTC (permalink / raw)
  To: linux-cifs
  Cc: Stefan Metzmacher, Steve French, Tom Talpey, Long Li,
	Namjae Jeon, David Howells, stable

I think it is a security problem to send confidential data in plaintext
over the wire, so we should avoid doing that even if rdma is in use.

We already have a similar check to prevent data integrity problems
for rdma offload.

Modern Windows servers support signed and encrypted rdma offload,
but we don't support this yet...

Stefan Metzmacher (3):
  cifs: introduce cifs_io_parms in smb2_async_writev()
  cifs: split out smb3_use_rdma_offload() helper
  cifs: don't try to use rdma offload on encrypted connections

 fs/cifs/smb2pdu.c | 89 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 19 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] cifs: introduce cifs_io_parms in smb2_async_writev()
  2023-02-01 12:04 [PATCH 0/3] avoid plaintext rdma offset if encryption is required Stefan Metzmacher
@ 2023-02-01 12:04 ` Stefan Metzmacher
  2023-02-01 12:04 ` [PATCH 2/3] cifs: split out smb3_use_rdma_offload() helper Stefan Metzmacher
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Stefan Metzmacher @ 2023-02-01 12:04 UTC (permalink / raw)
  To: linux-cifs
  Cc: Stefan Metzmacher, Steve French, Tom Talpey, Long Li,
	Namjae Jeon, David Howells, stable

This will simplify the following changes and makes it easy to get
in passed in from the caller in future.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
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: David Howells <dhowells@redhat.com>
Cc: linux-cifs@vger.kernel.org
Cc: stable@vger.kernel.org
---
 fs/cifs/smb2pdu.c | 53 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 2d5c3df2277d..64e2c8b438f4 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4504,10 +4504,27 @@ smb2_async_writev(struct cifs_writedata *wdata,
 	struct kvec iov[1];
 	struct smb_rqst rqst = { };
 	unsigned int total_len;
+	struct cifs_io_parms _io_parms;
+	struct cifs_io_parms *io_parms = NULL;
 
 	if (!wdata->server)
 		server = wdata->server = cifs_pick_channel(tcon->ses);
 
+	/*
+	 * in future we may get cifs_io_parms passed in from the caller,
+	 * but for now we construct it here...
+	 */
+	_io_parms = (struct cifs_io_parms) {
+		.tcon = tcon,
+		.server = server,
+		.offset = wdata->offset,
+		.length = wdata->bytes,
+		.persistent_fid = wdata->cfile->fid.persistent_fid,
+		.volatile_fid = wdata->cfile->fid.volatile_fid,
+		.pid = wdata->pid,
+	};
+	io_parms = &_io_parms;
+
 	rc = smb2_plain_req_init(SMB2_WRITE, tcon, server,
 				 (void **) &req, &total_len);
 	if (rc)
@@ -4517,26 +4534,31 @@ smb2_async_writev(struct cifs_writedata *wdata,
 		flags |= CIFS_TRANSFORM_REQ;
 
 	shdr = (struct smb2_hdr *)req;
-	shdr->Id.SyncId.ProcessId = cpu_to_le32(wdata->cfile->pid);
+	shdr->Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid);
 
-	req->PersistentFileId = wdata->cfile->fid.persistent_fid;
-	req->VolatileFileId = wdata->cfile->fid.volatile_fid;
+	req->PersistentFileId = io_parms->persistent_fid;
+	req->VolatileFileId = io_parms->volatile_fid;
 	req->WriteChannelInfoOffset = 0;
 	req->WriteChannelInfoLength = 0;
 	req->Channel = 0;
-	req->Offset = cpu_to_le64(wdata->offset);
+	req->Offset = cpu_to_le64(io_parms->offset);
 	req->DataOffset = cpu_to_le16(
 				offsetof(struct smb2_write_req, Buffer));
 	req->RemainingBytes = 0;
 
-	trace_smb3_write_enter(0 /* xid */, wdata->cfile->fid.persistent_fid,
-		tcon->tid, tcon->ses->Suid, wdata->offset, wdata->bytes);
+	trace_smb3_write_enter(0 /* xid */,
+			       io_parms->persistent_fid,
+			       io_parms->tcon->tid,
+			       io_parms->tcon->ses->Suid,
+			       io_parms->offset,
+			       io_parms->length);
+
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	/*
 	 * 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 >=
+	if (server->rdma && !server->sign && io_parms->length >=
 		server->smbd_conn->rdma_readwrite_threshold) {
 
 		struct smbd_buffer_descriptor_v1 *v1;
@@ -4590,14 +4612,14 @@ smb2_async_writev(struct cifs_writedata *wdata,
 	}
 #endif
 	cifs_dbg(FYI, "async write at %llu %u bytes\n",
-		 wdata->offset, wdata->bytes);
+		 io_parms->offset, io_parms->length);
 
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	/* For RDMA read, I/O size is in RemainingBytes not in Length */
 	if (!wdata->mr)
-		req->Length = cpu_to_le32(wdata->bytes);
+		req->Length = cpu_to_le32(io_parms->length);
 #else
-	req->Length = cpu_to_le32(wdata->bytes);
+	req->Length = cpu_to_le32(io_parms->length);
 #endif
 
 	if (wdata->credits.value > 0) {
@@ -4605,7 +4627,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
 						    SMB2_MAX_BUFFER_SIZE));
 		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
 
-		rc = adjust_credits(server, &wdata->credits, wdata->bytes);
+		rc = adjust_credits(server, &wdata->credits, io_parms->length);
 		if (rc)
 			goto async_writev_out;
 
@@ -4618,9 +4640,12 @@ smb2_async_writev(struct cifs_writedata *wdata,
 
 	if (rc) {
 		trace_smb3_write_err(0 /* no xid */,
-				     req->PersistentFileId,
-				     tcon->tid, tcon->ses->Suid, wdata->offset,
-				     wdata->bytes, rc);
+				     io_parms->persistent_fid,
+				     io_parms->tcon->tid,
+				     io_parms->tcon->ses->Suid,
+				     io_parms->offset,
+				     io_parms->length,
+				     rc);
 		kref_put(&wdata->refcount, release);
 		cifs_stats_fail_inc(tcon, SMB2_WRITE_HE);
 	}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/3] cifs: split out smb3_use_rdma_offload() helper
  2023-02-01 12:04 [PATCH 0/3] avoid plaintext rdma offset if encryption is required Stefan Metzmacher
  2023-02-01 12:04 ` [PATCH 1/3] cifs: introduce cifs_io_parms in smb2_async_writev() Stefan Metzmacher
@ 2023-02-01 12:04 ` Stefan Metzmacher
  2023-02-01 12:04 ` [PATCH 3/3] cifs: don't try to use rdma offload on encrypted connections Stefan Metzmacher
  2023-02-01 13:39 ` [PATCH 0/3] avoid plaintext rdma offset if encryption is required Christoph Hellwig
  3 siblings, 0 replies; 6+ messages in thread
From: Stefan Metzmacher @ 2023-02-01 12:04 UTC (permalink / raw)
  To: linux-cifs; +Cc: Stefan Metzmacher

We should have the logic to decide if we want rdma offload
in a single spot in order to advance it in future.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/cifs/smb2pdu.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 64e2c8b438f4..6a4d621241dd 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4063,6 +4063,32 @@ SMB2_flush(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
 	return rc;
 }
 
+#ifdef CONFIG_CIFS_SMB_DIRECT
+static inline bool smb3_use_rdma_offload(struct cifs_io_parms *io_parms)
+{
+	struct TCP_Server_Info *server = io_parms->server;
+	struct cifs_tcon *tcon = io_parms->tcon;
+
+	/* we can only offload if we're connected */
+	if (!server || !tcon)
+		return false;
+
+	/* we can only offload on an rdma connection */
+	if (!server->rdma || !server->smbd_conn)
+		return false;
+
+	/* we don't support signed offload yet */
+	if (server->sign)
+		return false;
+
+	/* offload also has its overhead, so only do it if desired */
+	if (io_parms->length < server->smbd_conn->rdma_readwrite_threshold)
+		return false;
+
+	return true;
+}
+#endif /* CONFIG_CIFS_SMB_DIRECT */
+
 /*
  * To form a chain of read requests, any read requests after the first should
  * have the end_of_chain boolean set to true.
@@ -4106,9 +4132,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 &&
-		rdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) {
-
+	if (smb3_use_rdma_offload(io_parms)) {
 		struct smbd_buffer_descriptor_v1 *v1;
 		bool need_invalidate = server->dialect == SMB30_PROT_ID;
 
@@ -4558,9 +4582,7 @@ 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 && io_parms->length >=
-		server->smbd_conn->rdma_readwrite_threshold) {
-
+	if (smb3_use_rdma_offload(io_parms)) {
 		struct smbd_buffer_descriptor_v1 *v1;
 		bool need_invalidate = server->dialect == SMB30_PROT_ID;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/3] cifs: don't try to use rdma offload on encrypted connections
  2023-02-01 12:04 [PATCH 0/3] avoid plaintext rdma offset if encryption is required Stefan Metzmacher
  2023-02-01 12:04 ` [PATCH 1/3] cifs: introduce cifs_io_parms in smb2_async_writev() Stefan Metzmacher
  2023-02-01 12:04 ` [PATCH 2/3] cifs: split out smb3_use_rdma_offload() helper Stefan Metzmacher
@ 2023-02-01 12:04 ` Stefan Metzmacher
  2023-02-01 13:39 ` [PATCH 0/3] avoid plaintext rdma offset if encryption is required Christoph Hellwig
  3 siblings, 0 replies; 6+ messages in thread
From: Stefan Metzmacher @ 2023-02-01 12:04 UTC (permalink / raw)
  To: linux-cifs
  Cc: Stefan Metzmacher, Steve French, Tom Talpey, Long Li,
	Namjae Jeon, David Howells, stable

The aim of using encryption on a connection is to keep
the data confidential, so we must not use plaintext rdma offload
for that data!

It seems that current windows servers and ksmbd would allow
this, but that's no reason to expose the users data in plaintext!
And servers hopefully reject this in future.

Note modern windows servers support signed or encrypted offload,
see MS-SMB2 2.2.3.1.6 SMB2_RDMA_TRANSFORM_CAPABILITIES, but we don't
support that yet.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
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: David Howells <dhowells@redhat.com>
Cc: linux-cifs@vger.kernel.org
Cc: stable@vger.kernel.org
---
 fs/cifs/smb2pdu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 6a4d621241dd..c5cb2639b3f1 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4081,6 +4081,10 @@ static inline bool smb3_use_rdma_offload(struct cifs_io_parms *io_parms)
 	if (server->sign)
 		return false;
 
+	/* we don't support encrypted offload yet */
+	if (smb3_encryption_required(tcon))
+		return false;
+
 	/* offload also has its overhead, so only do it if desired */
 	if (io_parms->length < server->smbd_conn->rdma_readwrite_threshold)
 		return false;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] avoid plaintext rdma offset if encryption is required
  2023-02-01 12:04 [PATCH 0/3] avoid plaintext rdma offset if encryption is required Stefan Metzmacher
                   ` (2 preceding siblings ...)
  2023-02-01 12:04 ` [PATCH 3/3] cifs: don't try to use rdma offload on encrypted connections Stefan Metzmacher
@ 2023-02-01 13:39 ` Christoph Hellwig
  2023-02-01 13:52   ` Stefan Metzmacher
  3 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2023-02-01 13:39 UTC (permalink / raw)
  To: Stefan Metzmacher
  Cc: linux-cifs, Steve French, Tom Talpey, Long Li, Namjae Jeon,
	David Howells, stable

On Wed, Feb 01, 2023 at 01:04:40PM +0100, Stefan Metzmacher wrote:
> I think it is a security problem to send confidential data in plaintext
> over the wire, so we should avoid doing that even if rdma is in use.

Yep.

> Modern Windows servers support signed and encrypted rdma offload,
> but we don't support this yet...

There is a series out on the list for encryption offload to mlx5
hardware, whch is one way to handle this.  If not you need to bounce
buffer.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] avoid plaintext rdma offset if encryption is required
  2023-02-01 13:39 ` [PATCH 0/3] avoid plaintext rdma offset if encryption is required Christoph Hellwig
@ 2023-02-01 13:52   ` Stefan Metzmacher
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Metzmacher @ 2023-02-01 13:52 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-cifs, Steve French, Tom Talpey, Long Li, Namjae Jeon,
	David Howells, stable

Am 01.02.23 um 14:39 schrieb Christoph Hellwig:
> On Wed, Feb 01, 2023 at 01:04:40PM +0100, Stefan Metzmacher wrote:
>> I think it is a security problem to send confidential data in plaintext
>> over the wire, so we should avoid doing that even if rdma is in use.
> 
> Yep.
> 
>> Modern Windows servers support signed and encrypted rdma offload,
>> but we don't support this yet...
> 
> There is a series out on the list for encryption offload to mlx5
> hardware, whch is one way to handle this.  If not you need to bounce
> buffer.

Yes, I saw that, but I don't think it's usable, windows is using
aes-{128,256}-{gcm,ccm}...

metze

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-01 13:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-01 12:04 [PATCH 0/3] avoid plaintext rdma offset if encryption is required Stefan Metzmacher
2023-02-01 12:04 ` [PATCH 1/3] cifs: introduce cifs_io_parms in smb2_async_writev() Stefan Metzmacher
2023-02-01 12:04 ` [PATCH 2/3] cifs: split out smb3_use_rdma_offload() helper Stefan Metzmacher
2023-02-01 12:04 ` [PATCH 3/3] cifs: don't try to use rdma offload on encrypted connections Stefan Metzmacher
2023-02-01 13:39 ` [PATCH 0/3] avoid plaintext rdma offset if encryption is required Christoph Hellwig
2023-02-01 13:52   ` Stefan Metzmacher

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.