[PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size

Linux CIFS filesystem development
 help / color / mirror / Atom feed

* [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size
@ 2017-08-27  3:23 shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w
       [not found] ` <1503804209-5419-1-git-send-email-shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 6+ messages in thread
From: shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w @ 2017-08-27  3:23 UTC (permalink / raw)
  To: jlayton-eUNUBHrolfbYtjvyW6yDsg
  Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, Shirish Pargaonkar

From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


Instead of sending allocated buffer size of a security descriptor,
send the actual size of the security descriptor during set cifs acl
operation.


Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Paul van Schayck <polleke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 setcifsacl.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/setcifsacl.c b/setcifsacl.c
index 7eeeaa6..0d4b15f 100644
--- a/setcifsacl.c
+++ b/setcifsacl.c
@@ -50,24 +50,33 @@ enum setcifsacl_actions {
 static void *plugin_handle;
 static bool plugin_loaded;
 
-static void
+static int
 copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src)
 {
-	int i;
+	int i, size = 0;
 
 	dst->revision = src->revision;
+	size += sizeof(uint8_t);
+
 	dst->num_subauth = src->num_subauth;
+	size += sizeof(uint8_t);
+
 	for (i = 0; i < NUM_AUTHS; i++)
 		dst->authority[i] = src->authority[i];
+	size += (sizeof(uint8_t) * NUM_AUTHS);
+
 	for (i = 0; i < src->num_subauth; i++)
 		dst->sub_auth[i] = src->sub_auth[i];
+	size += (sizeof(uint32_t) * src->num_subauth);
+
+	return size;
 }
 
 static void
 copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
-		int numaces, int acessize)
+		int numaces, int acessize, ssize_t *bufsize)
 {
-	int osidsoffset, gsidsoffset, dacloffset;
+	int size, osidsoffset, gsidsoffset, dacloffset;
 	struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
 	struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
 	struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr;
@@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
 	gsidsoffset = le32toh(pntsd->gsidoffset);
 	dacloffset = le32toh(pntsd->dacloffset);
 
+	size = sizeof(struct cifs_ntsd);
 	pnntsd->revision = pntsd->revision;
 	pnntsd->type = pntsd->type;
 	pnntsd->osidoffset = pntsd->osidoffset;
 	pnntsd->gsidoffset = pntsd->gsidoffset;
 	pnntsd->dacloffset = pntsd->dacloffset;
+	*bufsize = *bufsize + size;
 
 	dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd + dacloffset);
 	ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd + dacloffset);
 
+	size = acessize + sizeof(struct cifs_ctrl_acl);
 	ndacl_ptr->revision = dacl_ptr->revision;
-	ndacl_ptr->size = htole16(acessize + sizeof(struct cifs_ctrl_acl));
+	ndacl_ptr->size = htole16(size);
 	ndacl_ptr->num_aces = htole32(numaces);
+	*bufsize = *bufsize + size;
 
 	/* copy owner sid */
 	owner_sid_ptr = (struct cifs_sid *)((char *)pntsd + osidsoffset);
 	nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + osidsoffset);
-	copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
+	size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
+	*bufsize = *bufsize + size;
 
 	/* copy group sid */
 	group_sid_ptr = (struct cifs_sid *)((char *)pntsd + gsidsoffset);
 	ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + gsidsoffset);
-	copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
+	size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
+	*bufsize = *bufsize + size;
 
 	return;
 }
@@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct cifs_ace *dace, int compflags)
 }
 
 static int
-get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
-			int aces, ssize_t *bufsize, size_t *acesoffset)
+alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
+			int aces, size_t *acesoffset)
 {
-	unsigned int size, acessize, dacloffset;
+	unsigned int size, acessize, bufsize, dacloffset;
 
 	size = sizeof(struct cifs_ntsd) +
 		2 * sizeof(struct cifs_sid) +
@@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
 
 	*acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl);
 	acessize = aces * sizeof(struct cifs_ace);
-	*bufsize = size + acessize;
+	bufsize = size + acessize;
 
-	*npntsd = malloc(*bufsize);
+	*npntsd = malloc(bufsize);
 	if (!*npntsd) {
 		printf("%s: Memory allocation failure", __func__);
 		return errno;
@@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 	size_t acesoffset;
 	char *acesptr;
 
-	rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize, &acesoffset);
+	rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset);
 	if (rc)
 		return rc;
 
@@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 		acessize += size;
 		acesptr += size;
 	}
-	copy_sec_desc(pntsd, *npntsd, numcaces, acessize);
-	acesptr = (char *)*npntsd + acesoffset;
 
+	copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize);
 
 	return 0;
 }
@@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 	char *acesptr;
 
 	numaces = numfaces + numcaces;
-	rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize, &acesoffset);
+	rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset);
 	if (rc)
 		return rc;
 
@@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 		acesptr += size;
 		acessize += size;
 	}
-	copy_sec_desc(pntsd, *npntsd, numaces, acessize);
+
+	copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
 
 	return 0;
 }
@@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 		return -1;
 	}
 
-	rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
+	rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
 	if (rc)
 		return rc;
 
@@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 		acessize += size;
 	}
 
-	copy_sec_desc(pntsd, *npntsd, numfaces, acessize);
+	copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize);
 
 	return 0;
 }
@@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 		return -1;
 	}
 
-	rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
+	rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
 	if (rc)
 		return rc;
 
@@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
 		printf("%s: Nothing to delete\n", __func__);
 		return 1;
 	}
-	copy_sec_desc(pntsd, *npntsd, numaces, acessize);
+
+	copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
 
 	return 0;
 }
-- 
1.9.1

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

[parent not found: <1503804209-5419-1-git-send-email-shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

* Re: [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size
       [not found] ` <1503804209-5419-1-git-send-email-shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-08-27  3:40   ` Steve French
  2017-08-27 10:09   ` Jeff Layton
  1 sibling, 0 replies; 6+ messages in thread
From: Steve French @ 2017-08-27  3:40 UTC (permalink / raw)
  To: Shirish Pargaonkar
  Cc: Jeff Layton, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

seems plausible to add cc: stable

On Sat, Aug 26, 2017 at 10:23 PM,  <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
>
> Instead of sending allocated buffer size of a security descriptor,
> send the actual size of the security descriptor during set cifs acl
> operation.
>
>
> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Paul van Schayck <polleke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  setcifsacl.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
>
> diff --git a/setcifsacl.c b/setcifsacl.c
> index 7eeeaa6..0d4b15f 100644
> --- a/setcifsacl.c
> +++ b/setcifsacl.c
> @@ -50,24 +50,33 @@ enum setcifsacl_actions {
>  static void *plugin_handle;
>  static bool plugin_loaded;
>
> -static void
> +static int
>  copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src)
>  {
> -       int i;
> +       int i, size = 0;
>
>         dst->revision = src->revision;
> +       size += sizeof(uint8_t);
> +
>         dst->num_subauth = src->num_subauth;
> +       size += sizeof(uint8_t);
> +
>         for (i = 0; i < NUM_AUTHS; i++)
>                 dst->authority[i] = src->authority[i];
> +       size += (sizeof(uint8_t) * NUM_AUTHS);
> +
>         for (i = 0; i < src->num_subauth; i++)
>                 dst->sub_auth[i] = src->sub_auth[i];
> +       size += (sizeof(uint32_t) * src->num_subauth);
> +
> +       return size;
>  }
>
>  static void
>  copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
> -               int numaces, int acessize)
> +               int numaces, int acessize, ssize_t *bufsize)
>  {
> -       int osidsoffset, gsidsoffset, dacloffset;
> +       int size, osidsoffset, gsidsoffset, dacloffset;
>         struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
>         struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
>         struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr;
> @@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>         gsidsoffset = le32toh(pntsd->gsidoffset);
>         dacloffset = le32toh(pntsd->dacloffset);
>
> +       size = sizeof(struct cifs_ntsd);
>         pnntsd->revision = pntsd->revision;
>         pnntsd->type = pntsd->type;
>         pnntsd->osidoffset = pntsd->osidoffset;
>         pnntsd->gsidoffset = pntsd->gsidoffset;
>         pnntsd->dacloffset = pntsd->dacloffset;
> +       *bufsize = *bufsize + size;
>
>         dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd + dacloffset);
>         ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd + dacloffset);
>
> +       size = acessize + sizeof(struct cifs_ctrl_acl);
>         ndacl_ptr->revision = dacl_ptr->revision;
> -       ndacl_ptr->size = htole16(acessize + sizeof(struct cifs_ctrl_acl));
> +       ndacl_ptr->size = htole16(size);
>         ndacl_ptr->num_aces = htole32(numaces);
> +       *bufsize = *bufsize + size;
>
>         /* copy owner sid */
>         owner_sid_ptr = (struct cifs_sid *)((char *)pntsd + osidsoffset);
>         nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + osidsoffset);
> -       copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
> +       size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
> +       *bufsize = *bufsize + size;
>
>         /* copy group sid */
>         group_sid_ptr = (struct cifs_sid *)((char *)pntsd + gsidsoffset);
>         ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + gsidsoffset);
> -       copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
> +       size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
> +       *bufsize = *bufsize + size;
>
>         return;
>  }
> @@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct cifs_ace *dace, int compflags)
>  }
>
>  static int
> -get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
> -                       int aces, ssize_t *bufsize, size_t *acesoffset)
> +alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
> +                       int aces, size_t *acesoffset)
>  {
> -       unsigned int size, acessize, dacloffset;
> +       unsigned int size, acessize, bufsize, dacloffset;
>
>         size = sizeof(struct cifs_ntsd) +
>                 2 * sizeof(struct cifs_sid) +
> @@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>
>         *acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl);
>         acessize = aces * sizeof(struct cifs_ace);
> -       *bufsize = size + acessize;
> +       bufsize = size + acessize;
>
> -       *npntsd = malloc(*bufsize);
> +       *npntsd = malloc(bufsize);
>         if (!*npntsd) {
>                 printf("%s: Memory allocation failure", __func__);
>                 return errno;
> @@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>         size_t acesoffset;
>         char *acesptr;
>
> -       rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize, &acesoffset);
> +       rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset);
>         if (rc)
>                 return rc;
>
> @@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>                 acessize += size;
>                 acesptr += size;
>         }
> -       copy_sec_desc(pntsd, *npntsd, numcaces, acessize);
> -       acesptr = (char *)*npntsd + acesoffset;
>
> +       copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize);
>
>         return 0;
>  }
> @@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>         char *acesptr;
>
>         numaces = numfaces + numcaces;
> -       rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize, &acesoffset);
> +       rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset);
>         if (rc)
>                 return rc;
>
> @@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>                 acesptr += size;
>                 acessize += size;
>         }
> -       copy_sec_desc(pntsd, *npntsd, numaces, acessize);
> +
> +       copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>
>         return 0;
>  }
> @@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>                 return -1;
>         }
>
> -       rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
> +       rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>         if (rc)
>                 return rc;
>
> @@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>                 acessize += size;
>         }
>
> -       copy_sec_desc(pntsd, *npntsd, numfaces, acessize);
> +       copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize);
>
>         return 0;
>  }
> @@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>                 return -1;
>         }
>
> -       rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
> +       rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>         if (rc)
>                 return rc;
>
> @@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>                 printf("%s: Nothing to delete\n", __func__);
>                 return 1;
>         }
> -       copy_sec_desc(pntsd, *npntsd, numaces, acessize);
> +
> +       copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>
>         return 0;
>  }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

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

* Re: [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size
       [not found] ` <1503804209-5419-1-git-send-email-shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2017-08-27  3:40   ` Steve French
@ 2017-08-27 10:09   ` Jeff Layton
       [not found]     ` <1503828595.4743.5.camel-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
  1 sibling, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2017-08-27 10:09 UTC (permalink / raw)
  To: shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w
  Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> 
> Instead of sending allocated buffer size of a security descriptor,
> send the actual size of the security descriptor during set cifs acl
> operation.
> 
> 
> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Paul van Schayck <polleke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  setcifsacl.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/setcifsacl.c b/setcifsacl.c
> index 7eeeaa6..0d4b15f 100644
> --- a/setcifsacl.c
> +++ b/setcifsacl.c
> @@ -50,24 +50,33 @@ enum setcifsacl_actions {
>  static void *plugin_handle;
>  static bool plugin_loaded;
>  
> -static void
> +static int
>  copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src)
>  {
> -	int i;
> +	int i, size = 0;
>  
>  	dst->revision = src->revision;
> +	size += sizeof(uint8_t);
> +
>  	dst->num_subauth = src->num_subauth;
> +	size += sizeof(uint8_t);
> +
>  	for (i = 0; i < NUM_AUTHS; i++)
>  		dst->authority[i] = src->authority[i];
> +	size += (sizeof(uint8_t) * NUM_AUTHS);
> +
>  	for (i = 0; i < src->num_subauth; i++)
>  		dst->sub_auth[i] = src->sub_auth[i];
> +	size += (sizeof(uint32_t) * src->num_subauth);
> +
> +	return size;
>  }
>  
>  static void
>  copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
> -		int numaces, int acessize)
> +		int numaces, int acessize, ssize_t *bufsize)

A return pointer in a void return function is a little inefficient. How
about making this function return ssize_t instead?

>  {
> -	int osidsoffset, gsidsoffset, dacloffset;
> +	int size, osidsoffset, gsidsoffset, dacloffset;
>  	struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
>  	struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
>  	struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr;
> @@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>  	gsidsoffset = le32toh(pntsd->gsidoffset);
>  	dacloffset = le32toh(pntsd->dacloffset);
>  
> +	size = sizeof(struct cifs_ntsd);
>  	pnntsd->revision = pntsd->revision;
>  	pnntsd->type = pntsd->type;
>  	pnntsd->osidoffset = pntsd->osidoffset;
>  	pnntsd->gsidoffset = pntsd->gsidoffset;
>  	pnntsd->dacloffset = pntsd->dacloffset;
> +	*bufsize = *bufsize + size;

Here you're assuming that the size being passed in is always 0. Is that
a safe assumption? Would it be better to have it just do this there?

    *bufsize = size;

>  
>  	dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd + dacloffset);
>  	ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd + dacloffset);
>  
> +	size = acessize + sizeof(struct cifs_ctrl_acl);
>  	ndacl_ptr->revision = dacl_ptr->revision;
> -	ndacl_ptr->size = htole16(acessize + sizeof(struct cifs_ctrl_acl));
> +	ndacl_ptr->size = htole16(size);
>  	ndacl_ptr->num_aces = htole32(numaces);
> +	*bufsize = *bufsize + size;
>  
>  	/* copy owner sid */
>  	owner_sid_ptr = (struct cifs_sid *)((char *)pntsd + osidsoffset);
>  	nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + osidsoffset);
> -	copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
> +	size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
> +	*bufsize = *bufsize + size;
>  
>  	/* copy group sid */
>  	group_sid_ptr = (struct cifs_sid *)((char *)pntsd + gsidsoffset);
>  	ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + gsidsoffset);
> -	copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
> +	size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
> +	*bufsize = *bufsize + size;
>  
>  	return;
>  }
> @@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct cifs_ace *dace, int compflags)
>  }
>  
>  static int
> -get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
> -			int aces, ssize_t *bufsize, size_t *acesoffset)
> +alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
> +			int aces, size_t *acesoffset)
>  {
> -	unsigned int size, acessize, dacloffset;
> +	unsigned int size, acessize, bufsize, dacloffset;
>  
>  	size = sizeof(struct cifs_ntsd) +
>  		2 * sizeof(struct cifs_sid) +
> @@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>  
>  	*acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl);
>  	acessize = aces * sizeof(struct cifs_ace);
> -	*bufsize = size + acessize;
> +	bufsize = size + acessize;
>  
> -	*npntsd = malloc(*bufsize);
> +	*npntsd = malloc(bufsize);
>  	if (!*npntsd) {
>  		printf("%s: Memory allocation failure", __func__);
>  		return errno;
> @@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  	size_t acesoffset;
>  	char *acesptr;
>  
> -	rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize, &acesoffset);
> +	rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset);
>  	if (rc)
>  		return rc;
>  
> @@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  		acessize += size;
>  		acesptr += size;
>  	}
> -	copy_sec_desc(pntsd, *npntsd, numcaces, acessize);
> -	acesptr = (char *)*npntsd + acesoffset;
>  
> +	copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize);
>  
>  	return 0;
>  }
> @@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  	char *acesptr;
>  
>  	numaces = numfaces + numcaces;
> -	rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize, &acesoffset);
> +	rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset);
>  	if (rc)
>  		return rc;
>  
> @@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  		acesptr += size;
>  		acessize += size;
>  	}
> -	copy_sec_desc(pntsd, *npntsd, numaces, acessize);
> +
> +	copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>  
>  	return 0;
>  }
> @@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  		return -1;
>  	}
>  
> -	rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
> +	rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>  	if (rc)
>  		return rc;
>  
> @@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  		acessize += size;
>  	}
>  
> -	copy_sec_desc(pntsd, *npntsd, numfaces, acessize);
> +	copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize);
>  
>  	return 0;
>  }
> @@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  		return -1;
>  	}
>  
> -	rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
> +	rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>  	if (rc)
>  		return rc;
>  
> @@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>  		printf("%s: Nothing to delete\n", __func__);
>  		return 1;
>  	}
> -	copy_sec_desc(pntsd, *npntsd, numaces, acessize);
> +
> +	copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>  
>  	return 0;
>  }

Thanks for the patch. I'll plan to look more in detail tomorrow. Could
you explain a little about this bug was manifested? Is there a public
bug tracker link of some sort?

(PS for Steve: we don't really have a stable branch for cifs-utils.
Distros are on their own there.)
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

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

[parent not found: <1503828595.4743.5.camel-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

* Re: [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size
       [not found]     ` <1503828595.4743.5.camel-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
@ 2017-08-28  3:01       ` Shirish Pargaonkar
  2017-08-28  3:44       ` Shirish Pargaonkar
  1 sibling, 0 replies; 6+ messages in thread
From: Shirish Pargaonkar @ 2017-08-28  3:01 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-cifs

Jeff,

There is  no public bug tracker.  The problem was posted on the mailing list
and I did respond on the mailing list.
The problem is, in cifssetacl, we send a buffer and size that is longer than the
security descriptor and some smb servers did not accept it.

Regards,

Shirish

On Sun, Aug 27, 2017 at 5:09 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>>
>> Instead of sending allocated buffer size of a security descriptor,
>> send the actual size of the security descriptor during set cifs acl
>> operation.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Tested-by: Paul van Schayck <polleke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  setcifsacl.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/setcifsacl.c b/setcifsacl.c
>> index 7eeeaa6..0d4b15f 100644
>> --- a/setcifsacl.c
>> +++ b/setcifsacl.c
>> @@ -50,24 +50,33 @@ enum setcifsacl_actions {
>>  static void *plugin_handle;
>>  static bool plugin_loaded;
>>
>> -static void
>> +static int
>>  copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src)
>>  {
>> -     int i;
>> +     int i, size = 0;
>>
>>       dst->revision = src->revision;
>> +     size += sizeof(uint8_t);
>> +
>>       dst->num_subauth = src->num_subauth;
>> +     size += sizeof(uint8_t);
>> +
>>       for (i = 0; i < NUM_AUTHS; i++)
>>               dst->authority[i] = src->authority[i];
>> +     size += (sizeof(uint8_t) * NUM_AUTHS);
>> +
>>       for (i = 0; i < src->num_subauth; i++)
>>               dst->sub_auth[i] = src->sub_auth[i];
>> +     size += (sizeof(uint32_t) * src->num_subauth);
>> +
>> +     return size;
>>  }
>>
>>  static void
>>  copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>> -             int numaces, int acessize)
>> +             int numaces, int acessize, ssize_t *bufsize)
>
> A return pointer in a void return function is a little inefficient. How
> about making this function return ssize_t instead?
>
>>  {
>> -     int osidsoffset, gsidsoffset, dacloffset;
>> +     int size, osidsoffset, gsidsoffset, dacloffset;
>>       struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
>>       struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
>>       struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr;
>> @@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>>       gsidsoffset = le32toh(pntsd->gsidoffset);
>>       dacloffset = le32toh(pntsd->dacloffset);
>>
>> +     size = sizeof(struct cifs_ntsd);
>>       pnntsd->revision = pntsd->revision;
>>       pnntsd->type = pntsd->type;
>>       pnntsd->osidoffset = pntsd->osidoffset;
>>       pnntsd->gsidoffset = pntsd->gsidoffset;
>>       pnntsd->dacloffset = pntsd->dacloffset;
>> +     *bufsize = *bufsize + size;
>
> Here you're assuming that the size being passed in is always 0. Is that
> a safe assumption? Would it be better to have it just do this there?
>
>     *bufsize = size;
>
>>
>>       dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd + dacloffset);
>>       ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd + dacloffset);
>>
>> +     size = acessize + sizeof(struct cifs_ctrl_acl);
>>       ndacl_ptr->revision = dacl_ptr->revision;
>> -     ndacl_ptr->size = htole16(acessize + sizeof(struct cifs_ctrl_acl));
>> +     ndacl_ptr->size = htole16(size);
>>       ndacl_ptr->num_aces = htole32(numaces);
>> +     *bufsize = *bufsize + size;
>>
>>       /* copy owner sid */
>>       owner_sid_ptr = (struct cifs_sid *)((char *)pntsd + osidsoffset);
>>       nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + osidsoffset);
>> -     copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
>> +     size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
>> +     *bufsize = *bufsize + size;
>>
>>       /* copy group sid */
>>       group_sid_ptr = (struct cifs_sid *)((char *)pntsd + gsidsoffset);
>>       ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + gsidsoffset);
>> -     copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
>> +     size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
>> +     *bufsize = *bufsize + size;
>>
>>       return;
>>  }
>> @@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct cifs_ace *dace, int compflags)
>>  }
>>
>>  static int
>> -get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>> -                     int aces, ssize_t *bufsize, size_t *acesoffset)
>> +alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>> +                     int aces, size_t *acesoffset)
>>  {
>> -     unsigned int size, acessize, dacloffset;
>> +     unsigned int size, acessize, bufsize, dacloffset;
>>
>>       size = sizeof(struct cifs_ntsd) +
>>               2 * sizeof(struct cifs_sid) +
>> @@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>>
>>       *acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl);
>>       acessize = aces * sizeof(struct cifs_ace);
>> -     *bufsize = size + acessize;
>> +     bufsize = size + acessize;
>>
>> -     *npntsd = malloc(*bufsize);
>> +     *npntsd = malloc(bufsize);
>>       if (!*npntsd) {
>>               printf("%s: Memory allocation failure", __func__);
>>               return errno;
>> @@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>       size_t acesoffset;
>>       char *acesptr;
>>
>> -     rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               acessize += size;
>>               acesptr += size;
>>       }
>> -     copy_sec_desc(pntsd, *npntsd, numcaces, acessize);
>> -     acesptr = (char *)*npntsd + acesoffset;
>>
>> +     copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>> @@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>       char *acesptr;
>>
>>       numaces = numfaces + numcaces;
>> -     rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               acesptr += size;
>>               acessize += size;
>>       }
>> -     copy_sec_desc(pntsd, *npntsd, numaces, acessize);
>> +
>> +     copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>> @@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               return -1;
>>       }
>>
>> -     rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               acessize += size;
>>       }
>>
>> -     copy_sec_desc(pntsd, *npntsd, numfaces, acessize);
>> +     copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>> @@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               return -1;
>>       }
>>
>> -     rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               printf("%s: Nothing to delete\n", __func__);
>>               return 1;
>>       }
>> -     copy_sec_desc(pntsd, *npntsd, numaces, acessize);
>> +
>> +     copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>
> Thanks for the patch. I'll plan to look more in detail tomorrow. Could
> you explain a little about this bug was manifested? Is there a public
> bug tracker link of some sort?
>
> (PS for Steve: we don't really have a stable branch for cifs-utils.
> Distros are on their own there.)
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

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

* Re: [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size
       [not found]     ` <1503828595.4743.5.camel-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
  2017-08-28  3:01       ` Shirish Pargaonkar
@ 2017-08-28  3:44       ` Shirish Pargaonkar
       [not found]         ` <CADT32eJdOx7z9jb41M=ZQmdKrOrGwH5YF-GxzSGx0Z52UvHcew-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 6+ messages in thread
From: Shirish Pargaonkar @ 2017-08-28  3:44 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-cifs

[-- Attachment #1: Type: text/plain, Size: 8552 bytes --]

Jeff,

Attaching two smb2 wireshark traces, you can see the incorrect Setinfo size
in the current code (existing setcifsacl wireshark trace).

Regards,

Shirish

On Sun, Aug 27, 2017 at 5:09 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>>
>> Instead of sending allocated buffer size of a security descriptor,
>> send the actual size of the security descriptor during set cifs acl
>> operation.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Tested-by: Paul van Schayck <polleke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  setcifsacl.c | 58 +++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/setcifsacl.c b/setcifsacl.c
>> index 7eeeaa6..0d4b15f 100644
>> --- a/setcifsacl.c
>> +++ b/setcifsacl.c
>> @@ -50,24 +50,33 @@ enum setcifsacl_actions {
>>  static void *plugin_handle;
>>  static bool plugin_loaded;
>>
>> -static void
>> +static int
>>  copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src)
>>  {
>> -     int i;
>> +     int i, size = 0;
>>
>>       dst->revision = src->revision;
>> +     size += sizeof(uint8_t);
>> +
>>       dst->num_subauth = src->num_subauth;
>> +     size += sizeof(uint8_t);
>> +
>>       for (i = 0; i < NUM_AUTHS; i++)
>>               dst->authority[i] = src->authority[i];
>> +     size += (sizeof(uint8_t) * NUM_AUTHS);
>> +
>>       for (i = 0; i < src->num_subauth; i++)
>>               dst->sub_auth[i] = src->sub_auth[i];
>> +     size += (sizeof(uint32_t) * src->num_subauth);
>> +
>> +     return size;
>>  }
>>
>>  static void
>>  copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>> -             int numaces, int acessize)
>> +             int numaces, int acessize, ssize_t *bufsize)
>
> A return pointer in a void return function is a little inefficient. How
> about making this function return ssize_t instead?
>
>>  {
>> -     int osidsoffset, gsidsoffset, dacloffset;
>> +     int size, osidsoffset, gsidsoffset, dacloffset;
>>       struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
>>       struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
>>       struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr;
>> @@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>>       gsidsoffset = le32toh(pntsd->gsidoffset);
>>       dacloffset = le32toh(pntsd->dacloffset);
>>
>> +     size = sizeof(struct cifs_ntsd);
>>       pnntsd->revision = pntsd->revision;
>>       pnntsd->type = pntsd->type;
>>       pnntsd->osidoffset = pntsd->osidoffset;
>>       pnntsd->gsidoffset = pntsd->gsidoffset;
>>       pnntsd->dacloffset = pntsd->dacloffset;
>> +     *bufsize = *bufsize + size;
>
> Here you're assuming that the size being passed in is always 0. Is that
> a safe assumption? Would it be better to have it just do this there?
>
>     *bufsize = size;
>
>>
>>       dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd + dacloffset);
>>       ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd + dacloffset);
>>
>> +     size = acessize + sizeof(struct cifs_ctrl_acl);
>>       ndacl_ptr->revision = dacl_ptr->revision;
>> -     ndacl_ptr->size = htole16(acessize + sizeof(struct cifs_ctrl_acl));
>> +     ndacl_ptr->size = htole16(size);
>>       ndacl_ptr->num_aces = htole32(numaces);
>> +     *bufsize = *bufsize + size;
>>
>>       /* copy owner sid */
>>       owner_sid_ptr = (struct cifs_sid *)((char *)pntsd + osidsoffset);
>>       nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd + osidsoffset);
>> -     copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
>> +     size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
>> +     *bufsize = *bufsize + size;
>>
>>       /* copy group sid */
>>       group_sid_ptr = (struct cifs_sid *)((char *)pntsd + gsidsoffset);
>>       ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd + gsidsoffset);
>> -     copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
>> +     size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
>> +     *bufsize = *bufsize + size;
>>
>>       return;
>>  }
>> @@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct cifs_ace *dace, int compflags)
>>  }
>>
>>  static int
>> -get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>> -                     int aces, ssize_t *bufsize, size_t *acesoffset)
>> +alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>> +                     int aces, size_t *acesoffset)
>>  {
>> -     unsigned int size, acessize, dacloffset;
>> +     unsigned int size, acessize, bufsize, dacloffset;
>>
>>       size = sizeof(struct cifs_ntsd) +
>>               2 * sizeof(struct cifs_sid) +
>> @@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd,
>>
>>       *acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl);
>>       acessize = aces * sizeof(struct cifs_ace);
>> -     *bufsize = size + acessize;
>> +     bufsize = size + acessize;
>>
>> -     *npntsd = malloc(*bufsize);
>> +     *npntsd = malloc(bufsize);
>>       if (!*npntsd) {
>>               printf("%s: Memory allocation failure", __func__);
>>               return errno;
>> @@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>       size_t acesoffset;
>>       char *acesptr;
>>
>> -     rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               acessize += size;
>>               acesptr += size;
>>       }
>> -     copy_sec_desc(pntsd, *npntsd, numcaces, acessize);
>> -     acesptr = (char *)*npntsd + acesoffset;
>>
>> +     copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>> @@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>       char *acesptr;
>>
>>       numaces = numfaces + numcaces;
>> -     rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               acesptr += size;
>>               acessize += size;
>>       }
>> -     copy_sec_desc(pntsd, *npntsd, numaces, acessize);
>> +
>> +     copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>> @@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               return -1;
>>       }
>>
>> -     rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               acessize += size;
>>       }
>>
>> -     copy_sec_desc(pntsd, *npntsd, numfaces, acessize);
>> +     copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>> @@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               return -1;
>>       }
>>
>> -     rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize, &acesoffset);
>> +     rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
>>       if (rc)
>>               return rc;
>>
>> @@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct cifs_ntsd **npntsd, ssize_t *bufsize,
>>               printf("%s: Nothing to delete\n", __func__);
>>               return 1;
>>       }
>> -     copy_sec_desc(pntsd, *npntsd, numaces, acessize);
>> +
>> +     copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
>>
>>       return 0;
>>  }
>
> Thanks for the patch. I'll plan to look more in detail tomorrow. Could
> you explain a little about this bug was manifested? Is there a public
> bug tracker link of some sort?
>
> (PS for Steve: we don't really have a stable branch for cifs-utils.
> Distros are on their own there.)
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

[-- Attachment #2: setcifsacl.existing.083017.1.pcapng --]
[-- Type: application/x-pcapng, Size: 42688 bytes --]

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

[parent not found: <CADT32eJdOx7z9jb41M=ZQmdKrOrGwH5YF-GxzSGx0Z52UvHcew-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]

* Re: [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size
       [not found]         ` <CADT32eJdOx7z9jb41M=ZQmdKrOrGwH5YF-GxzSGx0Z52UvHcew-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-08-28 11:58           ` Jeffrey Layton
  0 siblings, 0 replies; 6+ messages in thread
From: Jeffrey Layton @ 2017-08-28 11:58 UTC (permalink / raw)
  To: Shirish Pargaonkar; +Cc: linux-cifs

Got it, thanks. I see the thread now.

I think it'd be cleaner to make copy_set_desc return a size_t instead
of adding a return pointer there. That also means that we don't need to
worry about how that value is initialized since the caller is in charge
of it.

Also, could you add some more info the changelog about how the bug
exhibits itself? That can make it easier for folks to notice a fix when
the bug pops up.

Thanks,
Jeff

On Sun, 2017-08-27 at 22:44 -0500, Shirish Pargaonkar wrote:
> Jeff,
> 
> Attaching two smb2 wireshark traces, you can see the incorrect
> Setinfo size
> in the current code (existing setcifsacl wireshark trace).
> 
> Regards,
> 
> Shirish
> 
> On Sun, Aug 27, 2017 at 5:09 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> wrote:
> > On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> > wrote:
> > > From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > 
> > > Instead of sending allocated buffer size of a security
> > > descriptor,
> > > send the actual size of the security descriptor during set cifs
> > > acl
> > > operation.
> > > 
> > > 
> > > Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Tested-by: Paul van Schayck <polleke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  setcifsacl.c | 58 +++++++++++++++++++++++++++++++++++++---------
> > > ------------
> > >  1 file changed, 37 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/setcifsacl.c b/setcifsacl.c
> > > index 7eeeaa6..0d4b15f 100644
> > > --- a/setcifsacl.c
> > > +++ b/setcifsacl.c
> > > @@ -50,24 +50,33 @@ enum setcifsacl_actions {
> > >  static void *plugin_handle;
> > >  static bool plugin_loaded;
> > > 
> > > -static void
> > > +static int
> > >  copy_cifs_sid(struct cifs_sid *dst, const struct cifs_sid *src)
> > >  {
> > > -     int i;
> > > +     int i, size = 0;
> > > 
> > >       dst->revision = src->revision;
> > > +     size += sizeof(uint8_t);
> > > +
> > >       dst->num_subauth = src->num_subauth;
> > > +     size += sizeof(uint8_t);
> > > +
> > >       for (i = 0; i < NUM_AUTHS; i++)
> > >               dst->authority[i] = src->authority[i];
> > > +     size += (sizeof(uint8_t) * NUM_AUTHS);
> > > +
> > >       for (i = 0; i < src->num_subauth; i++)
> > >               dst->sub_auth[i] = src->sub_auth[i];
> > > +     size += (sizeof(uint32_t) * src->num_subauth);
> > > +
> > > +     return size;
> > >  }
> > > 
> > >  static void
> > >  copy_sec_desc(const struct cifs_ntsd *pntsd, struct cifs_ntsd
> > > *pnntsd,
> > > -             int numaces, int acessize)
> > > +             int numaces, int acessize, ssize_t *bufsize)
> > 
> > A return pointer in a void return function is a little inefficient.
> > How
> > about making this function return ssize_t instead?
> > 
> > >  {
> > > -     int osidsoffset, gsidsoffset, dacloffset;
> > > +     int size, osidsoffset, gsidsoffset, dacloffset;
> > >       struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
> > >       struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
> > >       struct cifs_ctrl_acl *dacl_ptr, *ndacl_ptr;
> > > @@ -77,28 +86,34 @@ copy_sec_desc(const struct cifs_ntsd *pntsd,
> > > struct cifs_ntsd *pnntsd,
> > >       gsidsoffset = le32toh(pntsd->gsidoffset);
> > >       dacloffset = le32toh(pntsd->dacloffset);
> > > 
> > > +     size = sizeof(struct cifs_ntsd);
> > >       pnntsd->revision = pntsd->revision;
> > >       pnntsd->type = pntsd->type;
> > >       pnntsd->osidoffset = pntsd->osidoffset;
> > >       pnntsd->gsidoffset = pntsd->gsidoffset;
> > >       pnntsd->dacloffset = pntsd->dacloffset;
> > > +     *bufsize = *bufsize + size;
> > 
> > Here you're assuming that the size being passed in is always 0. Is
> > that
> > a safe assumption? Would it be better to have it just do this
> > there?
> > 
> >     *bufsize = size;
> > 
> > > 
> > >       dacl_ptr = (struct cifs_ctrl_acl *)((char *)pntsd +
> > > dacloffset);
> > >       ndacl_ptr = (struct cifs_ctrl_acl *)((char *)pnntsd +
> > > dacloffset);
> > > 
> > > +     size = acessize + sizeof(struct cifs_ctrl_acl);
> > >       ndacl_ptr->revision = dacl_ptr->revision;
> > > -     ndacl_ptr->size = htole16(acessize + sizeof(struct
> > > cifs_ctrl_acl));
> > > +     ndacl_ptr->size = htole16(size);
> > >       ndacl_ptr->num_aces = htole32(numaces);
> > > +     *bufsize = *bufsize + size;
> > > 
> > >       /* copy owner sid */
> > >       owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
> > > osidsoffset);
> > >       nowner_sid_ptr = (struct cifs_sid *)((char *)pnntsd +
> > > osidsoffset);
> > > -     copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
> > > +     size = copy_cifs_sid(nowner_sid_ptr, owner_sid_ptr);
> > > +     *bufsize = *bufsize + size;
> > > 
> > >       /* copy group sid */
> > >       group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
> > > gsidsoffset);
> > >       ngroup_sid_ptr = (struct cifs_sid *)((char *)pnntsd +
> > > gsidsoffset);
> > > -     copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
> > > +     size = copy_cifs_sid(ngroup_sid_ptr, group_sid_ptr);
> > > +     *bufsize = *bufsize + size;
> > > 
> > >       return;
> > >  }
> > > @@ -156,10 +171,10 @@ compare_aces(struct cifs_ace *sace, struct
> > > cifs_ace *dace, int compflags)
> > >  }
> > > 
> > >  static int
> > > -get_sec_desc_size(struct cifs_ntsd *pntsd, struct cifs_ntsd
> > > **npntsd,
> > > -                     int aces, ssize_t *bufsize, size_t
> > > *acesoffset)
> > > +alloc_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd
> > > **npntsd,
> > > +                     int aces, size_t *acesoffset)
> > >  {
> > > -     unsigned int size, acessize, dacloffset;
> > > +     unsigned int size, acessize, bufsize, dacloffset;
> > > 
> > >       size = sizeof(struct cifs_ntsd) +
> > >               2 * sizeof(struct cifs_sid) +
> > > @@ -169,9 +184,9 @@ get_sec_desc_size(struct cifs_ntsd *pntsd,
> > > struct cifs_ntsd **npntsd,
> > > 
> > >       *acesoffset = dacloffset + sizeof(struct cifs_ctrl_acl);
> > >       acessize = aces * sizeof(struct cifs_ace);
> > > -     *bufsize = size + acessize;
> > > +     bufsize = size + acessize;
> > > 
> > > -     *npntsd = malloc(*bufsize);
> > > +     *npntsd = malloc(bufsize);
> > >       if (!*npntsd) {
> > >               printf("%s: Memory allocation failure", __func__);
> > >               return errno;
> > > @@ -188,7 +203,7 @@ ace_set(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >       size_t acesoffset;
> > >       char *acesptr;
> > > 
> > > -     rc = get_sec_desc_size(pntsd, npntsd, numcaces, bufsize,
> > > &acesoffset);
> > > +     rc = alloc_sec_desc(pntsd, npntsd, numcaces, &acesoffset);
> > >       if (rc)
> > >               return rc;
> > > 
> > > @@ -198,9 +213,8 @@ ace_set(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >               acessize += size;
> > >               acesptr += size;
> > >       }
> > > -     copy_sec_desc(pntsd, *npntsd, numcaces, acessize);
> > > -     acesptr = (char *)*npntsd + acesoffset;
> > > 
> > > +     copy_sec_desc(pntsd, *npntsd, numcaces, acessize, bufsize);
> > > 
> > >       return 0;
> > >  }
> > > @@ -215,7 +229,7 @@ ace_add(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >       char *acesptr;
> > > 
> > >       numaces = numfaces + numcaces;
> > > -     rc = get_sec_desc_size(pntsd, npntsd, numaces, bufsize,
> > > &acesoffset);
> > > +     rc = alloc_sec_desc(pntsd, npntsd, numaces, &acesoffset);
> > >       if (rc)
> > >               return rc;
> > > 
> > > @@ -230,7 +244,8 @@ ace_add(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >               acesptr += size;
> > >               acessize += size;
> > >       }
> > > -     copy_sec_desc(pntsd, *npntsd, numaces, acessize);
> > > +
> > > +     copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
> > > 
> > >       return 0;
> > >  }
> > > @@ -249,7 +264,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >               return -1;
> > >       }
> > > 
> > > -     rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize,
> > > &acesoffset);
> > > +     rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
> > >       if (rc)
> > >               return rc;
> > > 
> > > @@ -270,7 +285,7 @@ ace_modify(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >               acessize += size;
> > >       }
> > > 
> > > -     copy_sec_desc(pntsd, *npntsd, numfaces, acessize);
> > > +     copy_sec_desc(pntsd, *npntsd, numfaces, acessize, bufsize);
> > > 
> > >       return 0;
> > >  }
> > > @@ -294,7 +309,7 @@ ace_delete(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >               return -1;
> > >       }
> > > 
> > > -     rc = get_sec_desc_size(pntsd, npntsd, numfaces, bufsize,
> > > &acesoffset);
> > > +     rc = alloc_sec_desc(pntsd, npntsd, numfaces, &acesoffset);
> > >       if (rc)
> > >               return rc;
> > > 
> > > @@ -317,7 +332,8 @@ ace_delete(struct cifs_ntsd *pntsd, struct
> > > cifs_ntsd **npntsd, ssize_t *bufsize,
> > >               printf("%s: Nothing to delete\n", __func__);
> > >               return 1;
> > >       }
> > > -     copy_sec_desc(pntsd, *npntsd, numaces, acessize);
> > > +
> > > +     copy_sec_desc(pntsd, *npntsd, numaces, acessize, bufsize);
> > > 
> > >       return 0;
> > >  }
> > 
> > Thanks for the patch. I'll plan to look more in detail tomorrow.
> > Could
> > you explain a little about this bug was manifested? Is there a
> > public
> > bug tracker link of some sort?
> > 
> > (PS for Steve: we don't really have a stable branch for cifs-utils.
> > Distros are on their own there.)
> > --
> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

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

end of thread, other threads:[~2017-08-28 11:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-27  3:23 [PATCH] cifs: setcifsacl - Send the actual (security descriptor) buffer size instead of the pre-allocated size shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w
     [not found] ` <1503804209-5419-1-git-send-email-shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-08-27  3:40   ` Steve French
2017-08-27 10:09   ` Jeff Layton
     [not found]     ` <1503828595.4743.5.camel-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
2017-08-28  3:01       ` Shirish Pargaonkar
2017-08-28  3:44       ` Shirish Pargaonkar
     [not found]         ` <CADT32eJdOx7z9jb41M=ZQmdKrOrGwH5YF-GxzSGx0Z52UvHcew-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-08-28 11:58           ` Jeffrey Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox