* [RFC PATCH] cifs: validate full SID length in security descriptors
@ 2026-05-19 2:36 Qihang
2026-05-19 16:28 ` Steve French
0 siblings, 1 reply; 2+ messages in thread
From: Qihang @ 2026-05-19 2:36 UTC (permalink / raw)
To: linux-cifs; +Cc: sfrench, smfrench, gregkh
parse_sid() only verified that the fixed SID header fit in the
returned security descriptor, but did not verify that the full SID
body described by num_subauth was present.
A malicious server can return a truncated owner or group SID whose
header lies within the descriptor buffer while sub_auth[] extends
past the end of the allocation, leading to an out-of-bounds read
when the client later parses or copies that SID.
Validate the full SID body in parse_sid(), centralize owner/group SID
lookup and bounds checking in sid_from_sd(), and use that validation
in parse_sec_desc(), build_sec_desc(), and copy_sec_desc() before
sub_auth[] is accessed.
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
fs/smb/client/cifsacl.c | 196 ++++++++++++++++++++++++++--------------
1 file changed, 129 insertions(+), 67 deletions(-)
diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index 786dbbc43c5b..42a3115359da 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -275,6 +275,73 @@ cifs_copy_sid(struct smb_sid *dst, const struct smb_sid *src)
return size;
}
+static int parse_sid(const struct smb_sid *psid, const char *end_of_acl)
+{
+ unsigned int sid_len;
+
+ /* SID must contain the fixed header before num_subauth is trusted. */
+ if (end_of_acl < (const char *)psid + CIFS_SID_BASE_SIZE) {
+ cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
+ return -EINVAL;
+ }
+ if (psid->num_subauth > SID_MAX_SUB_AUTHORITIES) {
+ cifs_dbg(VFS, "SID contains too many subauthorities %u\n",
+ psid->num_subauth);
+ return -EINVAL;
+ }
+
+ sid_len = CIFS_SID_BASE_SIZE + psid->num_subauth * sizeof(__le32);
+ if (end_of_acl < (const char *)psid + sid_len) {
+ cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
+ return -EINVAL;
+ }
+
+#ifdef CONFIG_CIFS_DEBUG2
+ if (psid->num_subauth) {
+ int i;
+
+ cifs_dbg(FYI, "SID revision %d num_auth %d\n",
+ psid->revision, psid->num_subauth);
+
+ for (i = 0; i < psid->num_subauth; i++) {
+ cifs_dbg(FYI, "SID sub_auth[%d]: 0x%x\n",
+ i, le32_to_cpu(psid->sub_auth[i]));
+ }
+
+ cifs_dbg(FYI, "RID 0x%x\n",
+ le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]));
+ }
+#endif
+
+ return 0;
+}
+
+static int sid_from_sd(const struct smb_ntsd *pntsd, __u32 secdesclen,
+ __u32 sid_offset, struct smb_sid **sid)
+{
+ struct smb_sid *psid;
+ char *end_of_acl;
+
+ if (secdesclen < sizeof(struct smb_ntsd)) {
+ cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
+ return -EINVAL;
+ }
+ end_of_acl = (char *)pntsd + secdesclen;
+
+ if (sid_offset < sizeof(struct smb_ntsd) ||
+ sid_offset > secdesclen - CIFS_SID_BASE_SIZE) {
+ cifs_dbg(VFS, "Server returned illegal SID offset\n");
+ return -EINVAL;
+ }
+
+ psid = (struct smb_sid *)((char *)pntsd + sid_offset);
+ if (parse_sid(psid, end_of_acl))
+ return -EINVAL;
+
+ *sid = psid;
+ return 0;
+}
+
static int
id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid)
{
@@ -515,14 +582,14 @@ exit_cifs_idmap(void)
}
/* copy ntsd, owner sid, and group sid from a security descriptor to another */
-static __u32 copy_sec_desc(const struct smb_ntsd *pntsd,
- struct smb_ntsd *pnntsd,
- __u32 sidsoffset,
- struct smb_sid *pownersid,
- struct smb_sid *pgrpsid)
+static int copy_sec_desc(const struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
+ __u32 sidsoffset, __u32 secdesclen,
+ __u32 *pnsecdesclen, struct smb_sid *pownersid,
+ struct smb_sid *pgrpsid)
{
struct smb_sid *owner_sid_ptr, *group_sid_ptr;
struct smb_sid *nowner_sid_ptr, *ngroup_sid_ptr;
+ int rc;
/* copy security descriptor control portion */
pnntsd->revision = pntsd->revision;
@@ -533,28 +600,34 @@ static __u32 copy_sec_desc(const struct smb_ntsd *pntsd,
pnntsd->gsidoffset = cpu_to_le32(sidsoffset + sizeof(struct smb_sid));
/* copy owner sid */
- if (pownersid)
+ if (pownersid) {
owner_sid_ptr = pownersid;
- else
- owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
- le32_to_cpu(pntsd->osidoffset));
+ } else {
+ rc = sid_from_sd(pntsd, secdesclen,
+ le32_to_cpu(pntsd->osidoffset), &owner_sid_ptr);
+ if (rc)
+ return rc;
+ }
nowner_sid_ptr = (struct smb_sid *)((char *)pnntsd + sidsoffset);
cifs_copy_sid(nowner_sid_ptr, owner_sid_ptr);
/* copy group sid */
- if (pgrpsid)
+ if (pgrpsid) {
group_sid_ptr = pgrpsid;
- else
- group_sid_ptr = (struct smb_sid *)((char *)pntsd +
- le32_to_cpu(pntsd->gsidoffset));
+ } else {
+ rc = sid_from_sd(pntsd, secdesclen,
+ le32_to_cpu(pntsd->gsidoffset), &group_sid_ptr);
+ if (rc)
+ return rc;
+ }
ngroup_sid_ptr = (struct smb_sid *)((char *)pnntsd + sidsoffset +
sizeof(struct smb_sid));
cifs_copy_sid(ngroup_sid_ptr, group_sid_ptr);
- return sidsoffset + (2 * sizeof(struct smb_sid));
+ *pnsecdesclen = sidsoffset + (2 * sizeof(struct smb_sid));
+ return 0;
}
-
/*
change posix mode to reflect permissions
pmode is the existing mode (we only want to overwrite part of this
@@ -1232,38 +1305,6 @@ static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,
return 0;
}
-static int parse_sid(struct smb_sid *psid, char *end_of_acl)
-{
- /* BB need to add parm so we can store the SID BB */
-
- /* validate that we do not go past end of ACL - sid must be at least 8
- bytes long (assuming no sub-auths - e.g. the null SID */
- if (end_of_acl < (char *)psid + 8) {
- cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
- return -EINVAL;
- }
-
-#ifdef CONFIG_CIFS_DEBUG2
- if (psid->num_subauth) {
- int i;
- cifs_dbg(FYI, "SID revision %d num_auth %d\n",
- psid->revision, psid->num_subauth);
-
- for (i = 0; i < psid->num_subauth; i++) {
- cifs_dbg(FYI, "SID sub_auth[%d]: 0x%x\n",
- i, le32_to_cpu(psid->sub_auth[i]));
- }
-
- /* BB add length check to make sure that we do not have huge
- num auths and therefore go off the end */
- cifs_dbg(FYI, "RID 0x%x\n",
- le32_to_cpu(psid->sub_auth[psid->num_subauth-1]));
- }
-#endif
-
- return 0;
-}
-
static bool dacl_offset_valid(unsigned int acl_len, __u32 dacloffset)
{
if (acl_len < sizeof(struct smb_acl))
@@ -1284,23 +1325,25 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
int rc = 0;
struct smb_sid *owner_sid_ptr, *group_sid_ptr;
struct smb_acl *dacl_ptr; /* no need for SACL ptr */
- char *end_of_acl = ((char *)pntsd) + acl_len;
- __u32 dacloffset;
+ char *end_of_acl;
+ __u32 dacloffset, osidoffset, gsidoffset;
if (pntsd == NULL)
return smb_EIO(smb_eio_trace_null_pointers);
+ if (acl_len < (int)sizeof(struct smb_ntsd)) {
+ cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
+ return -EINVAL;
+ }
+ end_of_acl = ((char *)pntsd) + acl_len;
- owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
- le32_to_cpu(pntsd->osidoffset));
- group_sid_ptr = (struct smb_sid *)((char *)pntsd +
- le32_to_cpu(pntsd->gsidoffset));
+ osidoffset = le32_to_cpu(pntsd->osidoffset);
+ gsidoffset = le32_to_cpu(pntsd->gsidoffset);
dacloffset = le32_to_cpu(pntsd->dacloffset);
cifs_dbg(NOISY, "revision %d type 0x%x ooffset 0x%x goffset 0x%x sacloffset 0x%x dacloffset 0x%x\n",
- pntsd->revision, pntsd->type, le32_to_cpu(pntsd->osidoffset),
- le32_to_cpu(pntsd->gsidoffset),
+ pntsd->revision, pntsd->type, osidoffset, gsidoffset,
le32_to_cpu(pntsd->sacloffset), dacloffset);
/* cifs_dump_mem("owner_sid: ", owner_sid_ptr, 64); */
- rc = parse_sid(owner_sid_ptr, end_of_acl);
+ rc = sid_from_sd(pntsd, acl_len, osidoffset, &owner_sid_ptr);
if (rc) {
cifs_dbg(FYI, "%s: Error %d parsing Owner SID\n", __func__, rc);
return rc;
@@ -1312,9 +1355,9 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
return rc;
}
- rc = parse_sid(group_sid_ptr, end_of_acl);
+ rc = sid_from_sd(pntsd, acl_len, gsidoffset, &group_sid_ptr);
if (rc) {
- cifs_dbg(FYI, "%s: Error %d mapping Owner SID to gid\n",
+ cifs_dbg(FYI, "%s: Error %d parsing Group SID\n",
__func__, rc);
return rc;
}
@@ -1354,8 +1397,15 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
struct smb_sid *nowner_sid_ptr = NULL, *ngroup_sid_ptr = NULL;
struct smb_acl *dacl_ptr = NULL; /* no need for SACL ptr */
struct smb_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
- char *end_of_acl = ((char *)pntsd) + secdesclen;
+ char *end_of_acl;
u16 size = 0;
+ __u32 osidoffset, gsidoffset;
+
+ if (secdesclen < sizeof(struct smb_ntsd)) {
+ cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
+ return -EINVAL;
+ }
+ end_of_acl = ((char *)pntsd) + secdesclen;
dacloffset = le32_to_cpu(pntsd->dacloffset);
if (dacloffset) {
@@ -1370,10 +1420,18 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
return rc;
}
- owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
- le32_to_cpu(pntsd->osidoffset));
- group_sid_ptr = (struct smb_sid *)((char *)pntsd +
- le32_to_cpu(pntsd->gsidoffset));
+ osidoffset = le32_to_cpu(pntsd->osidoffset);
+ gsidoffset = le32_to_cpu(pntsd->gsidoffset);
+ rc = sid_from_sd(pntsd, secdesclen, osidoffset, &owner_sid_ptr);
+ if (rc) {
+ cifs_dbg(FYI, "%s: Error %d parsing Owner SID\n", __func__, rc);
+ return rc;
+ }
+ rc = sid_from_sd(pntsd, secdesclen, gsidoffset, &group_sid_ptr);
+ if (rc) {
+ cifs_dbg(FYI, "%s: Error %d parsing Group SID\n", __func__, rc);
+ return rc;
+ }
if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
ndacloffset = sizeof(struct smb_ntsd);
@@ -1389,8 +1447,10 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
/* copy the non-dacl portion of secdesc */
- *pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
- NULL, NULL);
+ rc = copy_sec_desc(pntsd, pnntsd, sidsoffset, secdesclen,
+ pnsecdesclen, NULL, NULL);
+ if (rc)
+ return rc;
*aclflag |= CIFS_ACL_DACL;
} else {
@@ -1467,8 +1527,10 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
/* copy the non-dacl portion of secdesc */
- *pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
- nowner_sid_ptr, ngroup_sid_ptr);
+ rc = copy_sec_desc(pntsd, pnntsd, sidsoffset, secdesclen,
+ pnsecdesclen, nowner_sid_ptr, ngroup_sid_ptr);
+ if (rc)
+ goto chown_chgrp_exit;
chown_chgrp_exit:
/* errors could jump here. So make sure we return soon after this */
--
2.39.5 (Apple Git-154)
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [RFC PATCH] cifs: validate full SID length in security descriptors
2026-05-19 2:36 [RFC PATCH] cifs: validate full SID length in security descriptors Qihang
@ 2026-05-19 16:28 ` Steve French
0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2026-05-19 16:28 UTC (permalink / raw)
To: Qihang; +Cc: linux-cifs, sfrench, gregkh
merged into cifs-2.6.git for-next pending testing and additional review
On Mon, May 18, 2026 at 9:36 PM Qihang <q.h.hack.winter@gmail.com> wrote:
>
> parse_sid() only verified that the fixed SID header fit in the
> returned security descriptor, but did not verify that the full SID
> body described by num_subauth was present.
>
> A malicious server can return a truncated owner or group SID whose
> header lies within the descriptor buffer while sub_auth[] extends
> past the end of the allocation, leading to an out-of-bounds read
> when the client later parses or copies that SID.
>
> Validate the full SID body in parse_sid(), centralize owner/group SID
> lookup and bounds checking in sid_from_sd(), and use that validation
> in parse_sec_desc(), build_sec_desc(), and copy_sec_desc() before
> sub_auth[] is accessed.
>
> Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
> ---
> fs/smb/client/cifsacl.c | 196 ++++++++++++++++++++++++++--------------
> 1 file changed, 129 insertions(+), 67 deletions(-)
>
> diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
> index 786dbbc43c5b..42a3115359da 100644
> --- a/fs/smb/client/cifsacl.c
> +++ b/fs/smb/client/cifsacl.c
> @@ -275,6 +275,73 @@ cifs_copy_sid(struct smb_sid *dst, const struct smb_sid *src)
> return size;
> }
>
> +static int parse_sid(const struct smb_sid *psid, const char *end_of_acl)
> +{
> + unsigned int sid_len;
> +
> + /* SID must contain the fixed header before num_subauth is trusted. */
> + if (end_of_acl < (const char *)psid + CIFS_SID_BASE_SIZE) {
> + cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
> + return -EINVAL;
> + }
> + if (psid->num_subauth > SID_MAX_SUB_AUTHORITIES) {
> + cifs_dbg(VFS, "SID contains too many subauthorities %u\n",
> + psid->num_subauth);
> + return -EINVAL;
> + }
> +
> + sid_len = CIFS_SID_BASE_SIZE + psid->num_subauth * sizeof(__le32);
> + if (end_of_acl < (const char *)psid + sid_len) {
> + cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
> + return -EINVAL;
> + }
> +
> +#ifdef CONFIG_CIFS_DEBUG2
> + if (psid->num_subauth) {
> + int i;
> +
> + cifs_dbg(FYI, "SID revision %d num_auth %d\n",
> + psid->revision, psid->num_subauth);
> +
> + for (i = 0; i < psid->num_subauth; i++) {
> + cifs_dbg(FYI, "SID sub_auth[%d]: 0x%x\n",
> + i, le32_to_cpu(psid->sub_auth[i]));
> + }
> +
> + cifs_dbg(FYI, "RID 0x%x\n",
> + le32_to_cpu(psid->sub_auth[psid->num_subauth - 1]));
> + }
> +#endif
> +
> + return 0;
> +}
> +
> +static int sid_from_sd(const struct smb_ntsd *pntsd, __u32 secdesclen,
> + __u32 sid_offset, struct smb_sid **sid)
> +{
> + struct smb_sid *psid;
> + char *end_of_acl;
> +
> + if (secdesclen < sizeof(struct smb_ntsd)) {
> + cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
> + return -EINVAL;
> + }
> + end_of_acl = (char *)pntsd + secdesclen;
> +
> + if (sid_offset < sizeof(struct smb_ntsd) ||
> + sid_offset > secdesclen - CIFS_SID_BASE_SIZE) {
> + cifs_dbg(VFS, "Server returned illegal SID offset\n");
> + return -EINVAL;
> + }
> +
> + psid = (struct smb_sid *)((char *)pntsd + sid_offset);
> + if (parse_sid(psid, end_of_acl))
> + return -EINVAL;
> +
> + *sid = psid;
> + return 0;
> +}
> +
> static int
> id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid)
> {
> @@ -515,14 +582,14 @@ exit_cifs_idmap(void)
> }
>
> /* copy ntsd, owner sid, and group sid from a security descriptor to another */
> -static __u32 copy_sec_desc(const struct smb_ntsd *pntsd,
> - struct smb_ntsd *pnntsd,
> - __u32 sidsoffset,
> - struct smb_sid *pownersid,
> - struct smb_sid *pgrpsid)
> +static int copy_sec_desc(const struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
> + __u32 sidsoffset, __u32 secdesclen,
> + __u32 *pnsecdesclen, struct smb_sid *pownersid,
> + struct smb_sid *pgrpsid)
> {
> struct smb_sid *owner_sid_ptr, *group_sid_ptr;
> struct smb_sid *nowner_sid_ptr, *ngroup_sid_ptr;
> + int rc;
>
> /* copy security descriptor control portion */
> pnntsd->revision = pntsd->revision;
> @@ -533,28 +600,34 @@ static __u32 copy_sec_desc(const struct smb_ntsd *pntsd,
> pnntsd->gsidoffset = cpu_to_le32(sidsoffset + sizeof(struct smb_sid));
>
> /* copy owner sid */
> - if (pownersid)
> + if (pownersid) {
> owner_sid_ptr = pownersid;
> - else
> - owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
> - le32_to_cpu(pntsd->osidoffset));
> + } else {
> + rc = sid_from_sd(pntsd, secdesclen,
> + le32_to_cpu(pntsd->osidoffset), &owner_sid_ptr);
> + if (rc)
> + return rc;
> + }
> nowner_sid_ptr = (struct smb_sid *)((char *)pnntsd + sidsoffset);
> cifs_copy_sid(nowner_sid_ptr, owner_sid_ptr);
>
> /* copy group sid */
> - if (pgrpsid)
> + if (pgrpsid) {
> group_sid_ptr = pgrpsid;
> - else
> - group_sid_ptr = (struct smb_sid *)((char *)pntsd +
> - le32_to_cpu(pntsd->gsidoffset));
> + } else {
> + rc = sid_from_sd(pntsd, secdesclen,
> + le32_to_cpu(pntsd->gsidoffset), &group_sid_ptr);
> + if (rc)
> + return rc;
> + }
> ngroup_sid_ptr = (struct smb_sid *)((char *)pnntsd + sidsoffset +
> sizeof(struct smb_sid));
> cifs_copy_sid(ngroup_sid_ptr, group_sid_ptr);
>
> - return sidsoffset + (2 * sizeof(struct smb_sid));
> + *pnsecdesclen = sidsoffset + (2 * sizeof(struct smb_sid));
> + return 0;
> }
>
> -
> /*
> change posix mode to reflect permissions
> pmode is the existing mode (we only want to overwrite part of this
> @@ -1232,38 +1305,6 @@ static int set_chmod_dacl(struct smb_acl *pdacl, struct smb_acl *pndacl,
> return 0;
> }
>
> -static int parse_sid(struct smb_sid *psid, char *end_of_acl)
> -{
> - /* BB need to add parm so we can store the SID BB */
> -
> - /* validate that we do not go past end of ACL - sid must be at least 8
> - bytes long (assuming no sub-auths - e.g. the null SID */
> - if (end_of_acl < (char *)psid + 8) {
> - cifs_dbg(VFS, "ACL too small to parse SID %p\n", psid);
> - return -EINVAL;
> - }
> -
> -#ifdef CONFIG_CIFS_DEBUG2
> - if (psid->num_subauth) {
> - int i;
> - cifs_dbg(FYI, "SID revision %d num_auth %d\n",
> - psid->revision, psid->num_subauth);
> -
> - for (i = 0; i < psid->num_subauth; i++) {
> - cifs_dbg(FYI, "SID sub_auth[%d]: 0x%x\n",
> - i, le32_to_cpu(psid->sub_auth[i]));
> - }
> -
> - /* BB add length check to make sure that we do not have huge
> - num auths and therefore go off the end */
> - cifs_dbg(FYI, "RID 0x%x\n",
> - le32_to_cpu(psid->sub_auth[psid->num_subauth-1]));
> - }
> -#endif
> -
> - return 0;
> -}
> -
> static bool dacl_offset_valid(unsigned int acl_len, __u32 dacloffset)
> {
> if (acl_len < sizeof(struct smb_acl))
> @@ -1284,23 +1325,25 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
> int rc = 0;
> struct smb_sid *owner_sid_ptr, *group_sid_ptr;
> struct smb_acl *dacl_ptr; /* no need for SACL ptr */
> - char *end_of_acl = ((char *)pntsd) + acl_len;
> - __u32 dacloffset;
> + char *end_of_acl;
> + __u32 dacloffset, osidoffset, gsidoffset;
>
> if (pntsd == NULL)
> return smb_EIO(smb_eio_trace_null_pointers);
> + if (acl_len < (int)sizeof(struct smb_ntsd)) {
> + cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
> + return -EINVAL;
> + }
> + end_of_acl = ((char *)pntsd) + acl_len;
>
> - owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
> - le32_to_cpu(pntsd->osidoffset));
> - group_sid_ptr = (struct smb_sid *)((char *)pntsd +
> - le32_to_cpu(pntsd->gsidoffset));
> + osidoffset = le32_to_cpu(pntsd->osidoffset);
> + gsidoffset = le32_to_cpu(pntsd->gsidoffset);
> dacloffset = le32_to_cpu(pntsd->dacloffset);
> cifs_dbg(NOISY, "revision %d type 0x%x ooffset 0x%x goffset 0x%x sacloffset 0x%x dacloffset 0x%x\n",
> - pntsd->revision, pntsd->type, le32_to_cpu(pntsd->osidoffset),
> - le32_to_cpu(pntsd->gsidoffset),
> + pntsd->revision, pntsd->type, osidoffset, gsidoffset,
> le32_to_cpu(pntsd->sacloffset), dacloffset);
> /* cifs_dump_mem("owner_sid: ", owner_sid_ptr, 64); */
> - rc = parse_sid(owner_sid_ptr, end_of_acl);
> + rc = sid_from_sd(pntsd, acl_len, osidoffset, &owner_sid_ptr);
> if (rc) {
> cifs_dbg(FYI, "%s: Error %d parsing Owner SID\n", __func__, rc);
> return rc;
> @@ -1312,9 +1355,9 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
> return rc;
> }
>
> - rc = parse_sid(group_sid_ptr, end_of_acl);
> + rc = sid_from_sd(pntsd, acl_len, gsidoffset, &group_sid_ptr);
> if (rc) {
> - cifs_dbg(FYI, "%s: Error %d mapping Owner SID to gid\n",
> + cifs_dbg(FYI, "%s: Error %d parsing Group SID\n",
> __func__, rc);
> return rc;
> }
> @@ -1354,8 +1397,15 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
> struct smb_sid *nowner_sid_ptr = NULL, *ngroup_sid_ptr = NULL;
> struct smb_acl *dacl_ptr = NULL; /* no need for SACL ptr */
> struct smb_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
> - char *end_of_acl = ((char *)pntsd) + secdesclen;
> + char *end_of_acl;
> u16 size = 0;
> + __u32 osidoffset, gsidoffset;
> +
> + if (secdesclen < sizeof(struct smb_ntsd)) {
> + cifs_dbg(VFS, "ACL too small to parse security descriptor\n");
> + return -EINVAL;
> + }
> + end_of_acl = ((char *)pntsd) + secdesclen;
>
> dacloffset = le32_to_cpu(pntsd->dacloffset);
> if (dacloffset) {
> @@ -1370,10 +1420,18 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
> return rc;
> }
>
> - owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
> - le32_to_cpu(pntsd->osidoffset));
> - group_sid_ptr = (struct smb_sid *)((char *)pntsd +
> - le32_to_cpu(pntsd->gsidoffset));
> + osidoffset = le32_to_cpu(pntsd->osidoffset);
> + gsidoffset = le32_to_cpu(pntsd->gsidoffset);
> + rc = sid_from_sd(pntsd, secdesclen, osidoffset, &owner_sid_ptr);
> + if (rc) {
> + cifs_dbg(FYI, "%s: Error %d parsing Owner SID\n", __func__, rc);
> + return rc;
> + }
> + rc = sid_from_sd(pntsd, secdesclen, gsidoffset, &group_sid_ptr);
> + if (rc) {
> + cifs_dbg(FYI, "%s: Error %d parsing Group SID\n", __func__, rc);
> + return rc;
> + }
>
> if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
> ndacloffset = sizeof(struct smb_ntsd);
> @@ -1389,8 +1447,10 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
>
> sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
> /* copy the non-dacl portion of secdesc */
> - *pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
> - NULL, NULL);
> + rc = copy_sec_desc(pntsd, pnntsd, sidsoffset, secdesclen,
> + pnsecdesclen, NULL, NULL);
> + if (rc)
> + return rc;
>
> *aclflag |= CIFS_ACL_DACL;
> } else {
> @@ -1467,8 +1527,10 @@ static int build_sec_desc(struct smb_ntsd *pntsd, struct smb_ntsd *pnntsd,
>
> sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
> /* copy the non-dacl portion of secdesc */
> - *pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
> - nowner_sid_ptr, ngroup_sid_ptr);
> + rc = copy_sec_desc(pntsd, pnntsd, sidsoffset, secdesclen,
> + pnsecdesclen, nowner_sid_ptr, ngroup_sid_ptr);
> + if (rc)
> + goto chown_chgrp_exit;
>
> chown_chgrp_exit:
> /* errors could jump here. So make sure we return soon after this */
> --
> 2.39.5 (Apple Git-154)
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-05-19 16:28 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-19 2:36 [RFC PATCH] cifs: validate full SID length in security descriptors Qihang
2026-05-19 16:28 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox