| Message ID | 1503804209-5419-1-git-send-email-shirishpargaonkar@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
seems plausible to add cc: stable On Sat, Aug 26, 2017 at 10:23 PM, <shirishpargaonkar@gmail.com> wrote: > From: Shirish Pargaonkar <shirishpargaonkar@gmail.com> > > > 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@gmail.com> > Tested-by: Paul van Schayck <polleke@gmail.com> > --- > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar@gmail.com wrote: > From: Shirish Pargaonkar <shirishpargaonkar@gmail.com> > > > 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@gmail.com> > Tested-by: Paul van Schayck <polleke@gmail.com> > --- > 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, 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@samba.org> wrote: > On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar@gmail.com wrote: >> From: Shirish Pargaonkar <shirishpargaonkar@gmail.com> >> >> >> 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@gmail.com> >> Tested-by: Paul van Schayck <polleke@gmail.com> >> --- >> 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@samba.org> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@samba.org> wrote: > On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar@gmail.com wrote: >> From: Shirish Pargaonkar <shirishpargaonkar@gmail.com> >> >> >> 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@gmail.com> >> Tested-by: Paul van Schayck <polleke@gmail.com> >> --- >> 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@samba.org>
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@samba.org> > wrote: > > On Sat, 2017-08-26 at 22:23 -0500, shirishpargaonkar@gmail.com > > wrote: > > > From: Shirish Pargaonkar <shirishpargaonkar@gmail.com> > > > > > > > > > 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@gmail.com> > > > Tested-by: Paul van Schayck <polleke@gmail.com> > > > --- > > > 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@samba.org> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }