[PATCH 1/3] smb: client: improve compound padding in encryption

[PATCH 1/3] smb: client: improve compound padding in encryption

[Paulo Alcantara]

After commit f7f291e14dde ("cifs: fix oops during encryption"), the
encryption layer can handle vmalloc'd buffers as well as kmalloc'd
buffers, so there is no need to inefficiently squash request iovs
into a single one to handle padding in compound requests.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
---
 fs/smb/client/cifsglob.h  |  4 ++--
 fs/smb/client/smb2ops.c   | 37 +++---------------------------------
 fs/smb/client/transport.c | 40 +++++++++++++--------------------------
 3 files changed, 18 insertions(+), 63 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 63d194ebbd7d..fc33dfe7e925 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -2230,7 +2230,7 @@ static inline int cifs_get_num_sgs(const struct smb_rqst *rqst,
 			struct kvec *iov = &rqst[i].rq_iov[j];
 
 			addr = (unsigned long)iov->iov_base + skip;
-			if (unlikely(is_vmalloc_addr((void *)addr))) {
+			if (is_vmalloc_or_module_addr((void *)addr)) {
 				len = iov->iov_len - skip;
 				nents += DIV_ROUND_UP(offset_in_page(addr) + len,
 						      PAGE_SIZE);
@@ -2257,7 +2257,7 @@ static inline void cifs_sg_set_buf(struct sg_table *sgtable,
 	unsigned int off = offset_in_page(addr);
 
 	addr &= PAGE_MASK;
-	if (unlikely(is_vmalloc_addr((void *)addr))) {
+	if (is_vmalloc_or_module_addr((void *)addr)) {
 		do {
 			unsigned int len = min_t(unsigned int, buflen, PAGE_SIZE - off);
 
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 24a2aa04a108..6c4a7335a8a2 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2606,7 +2606,7 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst)
 	struct cifs_ses *ses = tcon->ses;
 	struct TCP_Server_Info *server = ses->server;
 	unsigned long len = smb_rqst_len(server, rqst);
-	int i, num_padding;
+	int num_padding;
 
 	shdr = (struct smb2_hdr *)(rqst->rq_iov[0].iov_base);
 	if (shdr == NULL) {
@@ -2615,44 +2615,13 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst)
 	}
 
 	/* SMB headers in a compound are 8 byte aligned. */
-
-	/* No padding needed */
-	if (!(len & 7))
-		goto finished;
-
-	num_padding = 8 - (len & 7);
-	if (!smb3_encryption_required(tcon)) {
-		/*
-		 * If we do not have encryption then we can just add an extra
-		 * iov for the padding.
-		 */
+	if (!IS_ALIGNED(len, 8)) {
+		num_padding = 8 - (len & 7);
 		rqst->rq_iov[rqst->rq_nvec].iov_base = smb2_padding;
 		rqst->rq_iov[rqst->rq_nvec].iov_len = num_padding;
 		rqst->rq_nvec++;
 		len += num_padding;
-	} else {
-		/*
-		 * We can not add a small padding iov for the encryption case
-		 * because the encryption framework can not handle the padding
-		 * iovs.
-		 * We have to flatten this into a single buffer and add
-		 * the padding to it.
-		 */
-		for (i = 1; i < rqst->rq_nvec; i++) {
-			memcpy(rqst->rq_iov[0].iov_base +
-			       rqst->rq_iov[0].iov_len,
-			       rqst->rq_iov[i].iov_base,
-			       rqst->rq_iov[i].iov_len);
-			rqst->rq_iov[0].iov_len += rqst->rq_iov[i].iov_len;
-		}
-		memset(rqst->rq_iov[0].iov_base + rqst->rq_iov[0].iov_len,
-		       0, num_padding);
-		rqst->rq_iov[0].iov_len += num_padding;
-		len += num_padding;
-		rqst->rq_nvec = 1;
 	}
-
- finished:
 	shdr->NextCommand = cpu_to_le32(len);
 }
 
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 91812150186c..0dc80959ce48 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -418,19 +418,16 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
 	return rc;
 }
 
-struct send_req_vars {
-	struct smb2_transform_hdr tr_hdr;
-	struct smb_rqst rqst[MAX_COMPOUND];
-	struct kvec iov;
-};
-
 static int
 smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
 	      struct smb_rqst *rqst, int flags)
 {
-	struct send_req_vars *vars;
-	struct smb_rqst *cur_rqst;
-	struct kvec *iov;
+	struct smb2_transform_hdr tr_hdr;
+	struct smb_rqst new_rqst[MAX_COMPOUND] = {};
+	struct kvec iov = {
+		.iov_base = &tr_hdr,
+		.iov_len = sizeof(tr_hdr),
+	};
 	int rc;
 
 	if (flags & CIFS_COMPRESS_REQ)
@@ -447,26 +444,15 @@ smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
 		return -EIO;
 	}
 
-	vars = kzalloc(sizeof(*vars), GFP_NOFS);
-	if (!vars)
-		return -ENOMEM;
-	cur_rqst = vars->rqst;
-	iov = &vars->iov;
-
-	iov->iov_base = &vars->tr_hdr;
-	iov->iov_len = sizeof(vars->tr_hdr);
-	cur_rqst[0].rq_iov = iov;
-	cur_rqst[0].rq_nvec = 1;
+	new_rqst[0].rq_iov = &iov;
+	new_rqst[0].rq_nvec = 1;
 
 	rc = server->ops->init_transform_rq(server, num_rqst + 1,
-					    &cur_rqst[0], rqst);
-	if (rc)
-		goto out;
-
-	rc = __smb_send_rqst(server, num_rqst + 1, &cur_rqst[0]);
-	smb3_free_compound_rqst(num_rqst, &cur_rqst[1]);
-out:
-	kfree(vars);
+					    new_rqst, rqst);
+	if (!rc) {
+		rc = __smb_send_rqst(server, num_rqst + 1, new_rqst);
+		smb3_free_compound_rqst(num_rqst, &new_rqst[1]);
+	}
 	return rc;
 }
 
-- 
2.47.0

[PATCH 2/3] smb: client: get rid of bounds check in SMB2_ioctl_init()

[Paulo Alcantara]

smb2_set_next_command() no longer squashes request iovs into a single
iov, so the bounds check can be dropped.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
---
 fs/smb/client/smb2pdu.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index ab3a2ca66be3..055236835537 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -3313,15 +3313,6 @@ SMB2_ioctl_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
 		return rc;
 
 	if (indatalen) {
-		unsigned int len;
-
-		if (WARN_ON_ONCE(smb3_encryption_required(tcon) &&
-				 (check_add_overflow(total_len - 1,
-						     ALIGN(indatalen, 8), &len) ||
-				  len > MAX_CIFS_SMALL_BUFFER_SIZE))) {
-			cifs_small_buf_release(req);
-			return -EIO;
-		}
 		/*
 		 * indatalen is usually small at a couple of bytes max, so
 		 * just allocate through generic pool
-- 
2.47.0

[PATCH 3/3] smb: client: handle max length for SMB symlinks

[Paulo Alcantara]

We can't use PATH_MAX for SMB symlinks because

  (1) Windows Server will fail FSCTL_SET_REPARSE_POINT with
      STATUS_IO_REPARSE_DATA_INVALID when input buffer is larger than
      16K, as specified in MS-FSA 2.1.5.10.37.

  (2) The client won't be able to parse large SMB responses that
      includes SMB symlink path within SMB2_CREATE or SMB2_IOCTL
      responses.

Fix this by defining a maximum length value (4060) for SMB symlinks
that both client and server can handle.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
---
 fs/smb/client/reparse.c | 5 ++++-
 fs/smb/client/reparse.h | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
index 74abbdf5026c..90da1e2b6217 100644
--- a/fs/smb/client/reparse.c
+++ b/fs/smb/client/reparse.c
@@ -35,6 +35,9 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
 	u16 len, plen;
 	int rc = 0;
 
+	if (strlen(symname) > REPARSE_SYM_PATH_MAX)
+		return -ENAMETOOLONG;
+
 	sym = kstrdup(symname, GFP_KERNEL);
 	if (!sym)
 		return -ENOMEM;
@@ -64,7 +67,7 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
 	if (rc < 0)
 		goto out;
 
-	plen = 2 * UniStrnlen((wchar_t *)path, PATH_MAX);
+	plen = 2 * UniStrnlen((wchar_t *)path, REPARSE_SYM_PATH_MAX);
 	len = sizeof(*buf) + plen * 2;
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf) {
diff --git a/fs/smb/client/reparse.h b/fs/smb/client/reparse.h
index 158e7b7aae64..2a9f4f9f79de 100644
--- a/fs/smb/client/reparse.h
+++ b/fs/smb/client/reparse.h
@@ -12,6 +12,8 @@
 #include "fs_context.h"
 #include "cifsglob.h"
 
+#define REPARSE_SYM_PATH_MAX 4060
+
 /*
  * Used only by cifs.ko to ignore reparse points from files when client or
  * server doesn't support FSCTL_GET_REPARSE_POINT.
-- 
2.47.0

Re: [PATCH 3/3] smb: client: handle max length for SMB symlinks

[Steve French]

Presumably a symlink could be up to PATH_MAX (4096), is there any
reason not to fix the client side to be able to handle at least 4096?

Any ideas how common long symlink target paths are?

On Mon, Nov 18, 2024 at 9:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> We can't use PATH_MAX for SMB symlinks because
>
>   (1) Windows Server will fail FSCTL_SET_REPARSE_POINT with
>       STATUS_IO_REPARSE_DATA_INVALID when input buffer is larger than
>       16K, as specified in MS-FSA 2.1.5.10.37.
>
>   (2) The client won't be able to parse large SMB responses that
>       includes SMB symlink path within SMB2_CREATE or SMB2_IOCTL
>       responses.
>
> Fix this by defining a maximum length value (4060) for SMB symlinks
> that both client and server can handle.
>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> ---
>  fs/smb/client/reparse.c | 5 ++++-
>  fs/smb/client/reparse.h | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> index 74abbdf5026c..90da1e2b6217 100644
> --- a/fs/smb/client/reparse.c
> +++ b/fs/smb/client/reparse.c
> @@ -35,6 +35,9 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
>         u16 len, plen;
>         int rc = 0;
>
> +       if (strlen(symname) > REPARSE_SYM_PATH_MAX)
> +               return -ENAMETOOLONG;
> +
>         sym = kstrdup(symname, GFP_KERNEL);
>         if (!sym)
>                 return -ENOMEM;
> @@ -64,7 +67,7 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
>         if (rc < 0)
>                 goto out;
>
> -       plen = 2 * UniStrnlen((wchar_t *)path, PATH_MAX);
> +       plen = 2 * UniStrnlen((wchar_t *)path, REPARSE_SYM_PATH_MAX);
>         len = sizeof(*buf) + plen * 2;
>         buf = kzalloc(len, GFP_KERNEL);
>         if (!buf) {
> diff --git a/fs/smb/client/reparse.h b/fs/smb/client/reparse.h
> index 158e7b7aae64..2a9f4f9f79de 100644
> --- a/fs/smb/client/reparse.h
> +++ b/fs/smb/client/reparse.h
> @@ -12,6 +12,8 @@
>  #include "fs_context.h"
>  #include "cifsglob.h"
>
> +#define REPARSE_SYM_PATH_MAX 4060
> +
>  /*
>   * Used only by cifs.ko to ignore reparse points from files when client or
>   * server doesn't support FSCTL_GET_REPARSE_POINT.
> --
> 2.47.0
>


-- 
Thanks,

Steve

Re: [PATCH 3/3] smb: client: handle max length for SMB symlinks

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> Presumably a symlink could be up to PATH_MAX (4096), is there any
> reason not to fix the client side to be able to handle at least 4096?

Even if we fix client to handle larger responses, Windows Server will
not allow more than 4091 characters due to MS-FSA 2.1.5.10.37.  It's
even worst with encryption that we only support parsing large responses
with SMB2_READ.

Yes, we could fix the client to support up to 4091 chars, but that is
out of scope for this series.  This patch is for allowing users to
create SMB symlinks and read them back with and without encryption.